New issue
Advanced search Search tips

Issue 649235 link

Starred by 2 users

Issue metadata

Status: WontFix
Owner:
Closed: Aug 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug-Regression

Blocked on:
issue 670316

Blocking:
issue 683999



Sign in to add a comment

2.1% regression in smoothness.tough_animation_cases at 419726:419788

Project Member Reported by toyoshim@chromium.org, Sep 22 2016

Issue description

See the link to graphs below.
 
All graphs for this bug:
  https://chromeperf.appspot.com/group_report?bug_id=649235

Original alerts at time of bug-filing:
  https://chromeperf.appspot.com/group_report?keys=agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgICgheXcvQoM


Bot(s) for this bug's original alert(s):

chromium-rel-win7-x64-dual
Project Member

Comment 3 by 42576172...@developer.gserviceaccount.com, Sep 22 2016

Cc: jfernan...@igalia.com
Owner: jfernan...@igalia.com

=== Auto-CCing suspected CL author jfernandez@igalia.com ===

Hi jfernandez@igalia.com, the bisect results pointed to your CL below as possibly
causing a regression. Please have a look at this info and see whether
your CL be related.


===== BISECT JOB RESULTS =====
Status: completed


===== SUSPECTED CL(s) =====
Subject : [css-align] Initial value of align-content should be 'stretch'.
Author  : jfernandez
Commit description:
  
We only allow the new CSS Box Alignment syntax when the Grid Layout
feature is enabled. Due to flexbox backward compatibility we have
implemented a different code path for the style initial/default values
assignment. However, we have incorrectly resolved both align-content
and justify-content to 'flex-start' when grid layout is disabled.

This patch changes the approach, so we set 'normal' (the value specified
by the new syntax) for both properties, but using the values defined in
the old syntax (Flexbox specification) at computed style resolution.

Since 'stretch' is the default value for the align-content property, this
issue implies that any flexbox line with an undefined height will be
laid out incorrectly, if not explicitly set via CSS, because flex items
can't use the available height, even though they use 'stretch' for their
'align-self' properties.

BUG= 647694 

Review-Url: https://codereview.chromium.org/2348273002
Cr-Commit-Position: refs/heads/master@{#419737}
Commit  : eeac3e6a187e362d796fa01489927e773be705ac
Date    : Tue Sep 20 13:19:31 2016


===== TESTED REVISIONS =====
Revision         Mean     Std Dev    N  Good?
chromium@419725  18.5728  0.013689   5  good
chromium@419733  18.555   0.0179708  5  good
chromium@419735  18.5596  0.014516   5  good
chromium@419736  18.5638  0.0165809  5  good
chromium@419737  18.905   0.0181797  5  bad    <--
chromium@419741  18.8953  0.0148596  5  bad
chromium@419757  18.8651  0.0121081  5  bad
chromium@419788  18.8915  0.0221865  5  bad

Bisect job ran on: win_x64_perf_bisect
Bug ID: 649235

Test Command: src/tools/perf/run_benchmark -v --browser=release_x64 --output-format=chartjson --upload-results --also-run-disabled-tests smoothness.tough_animation_cases
Test Metric: frame_times/frame_times
Relative Change: 1.72%
Score: 99.9

Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/win_x64_perf_bisect/builds/1478
Job details: https://chromeperf.appspot.com/buildbucket_job_status/9000845381509896864


Not what you expected? We'll investigate and get back to you!
  https://chromeperf.appspot.com/bad_bisect?try_job_id=5802396649783296

| O O | Visit http://www.chromium.org/developers/speed-infra/perf-bug-faq
|  X  | for more information addressing perf regression bugs. For feedback,
| / \ | file a bug with component Tests>AutoBisect.  Thank you!
I'll take a look.
I've been investigating this for some time but I couldn't found out why my patch could have cause such regression. I've sent several try jobs in all win platform bots reverting the suspicious patch, but I haven't got results showing improvements. 

https://codereview.chromium.org/2362893003/

Of course I could be misunderstanding the results, so I'd appreciate additional feedback on this issue.
Project Member

Comment 7 by 42576172...@developer.gserviceaccount.com, Sep 28 2016


===== BISECT JOB RESULTS =====
Status: completed


===== SUSPECTED CL(s) =====
Subject : [css-align] Initial value of align-content should be 'stretch'.
Author  : jfernandez
Commit description:
  
We only allow the new CSS Box Alignment syntax when the Grid Layout
feature is enabled. Due to flexbox backward compatibility we have
implemented a different code path for the style initial/default values
assignment. However, we have incorrectly resolved both align-content
and justify-content to 'flex-start' when grid layout is disabled.

This patch changes the approach, so we set 'normal' (the value specified
by the new syntax) for both properties, but using the values defined in
the old syntax (Flexbox specification) at computed style resolution.

Since 'stretch' is the default value for the align-content property, this
issue implies that any flexbox line with an undefined height will be
laid out incorrectly, if not explicitly set via CSS, because flex items
can't use the available height, even though they use 'stretch' for their
'align-self' properties.

BUG= 647694 

Review-Url: https://codereview.chromium.org/2348273002
Cr-Commit-Position: refs/heads/master@{#419737}
Commit  : eeac3e6a187e362d796fa01489927e773be705ac
Date    : Tue Sep 20 13:19:31 2016


===== TESTED REVISIONS =====
Revision         Mean     Std Dev    N  Good?
chromium@419725  18.5503  0.0271764  5  good
chromium@419733  18.5573  0.0187236  5  good
chromium@419735  18.538   0.015797   5  good
chromium@419736  18.5488  0.0171056  5  good
chromium@419737  18.8664  0.0236445  5  bad    <--
chromium@419741  18.887   0.0282794  5  bad
chromium@419757  18.881   0.0211168  5  bad
chromium@419788  18.8914  0.0066199  5  bad

Bisect job ran on: win_x64_perf_bisect
Bug ID: 649235

Test Command: src/tools/perf/run_benchmark -v --browser=release_x64 --output-format=chartjson --upload-results --also-run-disabled-tests smoothness.tough_animation_cases
Test Metric: frame_times/frame_times
Relative Change: 1.84%
Score: 99.9

Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/win_x64_perf_bisect/builds/1481
Job details: https://chromeperf.appspot.com/buildbucket_job_status/9000363799165314000


Not what you expected? We'll investigate and get back to you!
  https://chromeperf.appspot.com/bad_bisect?try_job_id=5895184275996672

| O O | Visit http://www.chromium.org/developers/speed-infra/perf-bug-faq
|  X  | for more information addressing perf regression bugs. For feedback,
| / \ | file a bug with component Tests>AutoBisect.  Thank you!
Perf sheriff ping
I can't find the smoothness.tough_animation_cases. Could anybody show me where I could find it ? I'd need to analyze how the test cases are defined and how a change in CSS Alignment properties could affect. 

I'd be very useful if someone could provide some insight about why the regression happens only on win platforms and only in these test cases. The suspected change should affect many other performance tests as well.  
Cc: alancutter@chromium.org
Are you looking for this?:
https://chromium.googlesource.com/chromium/src/+/master/tools/perf/benchmarks/smoothness.py#164
Yes, thanks.
Ping.  Any luck?
Sorry, didn't find the time to look at this deeply. I reverted the suspected patch some time ago, and sent several peft try jobs: 

https://codereview.chromium.org/2362893003/

However, I didn't get promising results reverting the patch. It's pretty strange that this change affects the animation tests, since it only affects how we resolve the computed style (I doubt it has any impact on animation) and how we initialize the align-content property of ComputedStyle class. This could, somehow, affect webs where align-content is used since we will have to resolve the "normal" value.

I'd need more time to be really sure about the root cause and propose a solution.
I've started a new CL at https://crrev.com/2569113004 to try to solve this issue.
I don't get any result, at least one I can access to, for the perf try jobs I launched; not sure why. 
If you look at the buildbot status page (example: https://build.chromium.org/p/tryserver.chromium.perf/builders/winx64nvidia_perf_bisect/builds/1879) you'll see a "HTML Results" link: http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-09-27_08-10-00

There should be one for each try job.
Umm, yes, that's what I expected, but for some reason I don't find the "HTML Results" link on any of the perf try jobs I launched in the mentioned issue.

https://build.chromium.org/p/tryserver.chromium.perf/builders/winx64nvidia_perf_bisect/builds/1981
Cc: simonhatch@chromium.org
+simonhatch, do you know what went wrong with the perf try job in #17?
Blockedon: 670316
Perf Try jobs are pretty broken at the moment,  crbug.com/670316 

Pulled this from the logs of "result" step: https://console.developers.google.com/m/cloudstorage/b/chrome-telemetry-output/o/html-results/results-2016-12-19_12-56-23

Error 403 when accessing the link.
I'm still unable to get any meaningful report from the perf try job I launched for the win platform:

https://codereview.chromium.org/2592653004

There use to be an HTML report with the performance analysis, but for some reason I don't get it now.
Quick update, html results should be fixed so please try again.
I'm not sure if I'm doing something wrong, but I'm still unable to get the HTML results of the performance try jobs. Please, take a look at the jobs submitted for the following issue to help me figure out what's wrong with them:

https://codereview.chromium.org/2569113004/


Blocking: 683999
Ah so sorry, a bug popped up yesterday that prevented things running properly on the windows bots :( The fix is in flight, and will land shortly at which point you can retry.
Status: WontFix (was: Assigned)
Looks like it'll be hard to make progress at this point.

Sign in to add a comment