Issue metadata
Sign in to add a comment
|
2.1% regression in smoothness.tough_animation_cases at 419726:419788 |
||||||||||||||||||||||
Issue descriptionSee the link to graphs below.
,
Sep 22 2016
Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/9000845381509896864
,
Sep 22 2016
=== 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!
,
Sep 23 2016
I'll take a look.
,
Sep 27 2016
Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/9000363799165314000
,
Sep 27 2016
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.
,
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!
,
Oct 11 2016
Perf sheriff ping
,
Oct 14 2016
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.
,
Dec 3 2016
Are you looking for this?: https://chromium.googlesource.com/chromium/src/+/master/tools/perf/benchmarks/smoothness.py#164
,
Dec 3 2016
Yes, thanks.
,
Dec 13 2016
Ping. Any luck?
,
Dec 13 2016
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.
,
Dec 14 2016
I've started a new CL at https://crrev.com/2569113004 to try to solve this issue.
,
Dec 20 2016
I don't get any result, at least one I can access to, for the perf try jobs I launched; not sure why.
,
Dec 20 2016
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.
,
Dec 20 2016
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
,
Dec 20 2016
+simonhatch, do you know what went wrong with the perf try job in #17?
,
Dec 20 2016
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
,
Dec 20 2016
Error 403 when accessing the link.
,
Jan 12 2017
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.
,
Jan 20 2017
Quick update, html results should be fixed so please try again.
,
Jan 23 2017
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/
,
Jan 23 2017
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.
,
Aug 16 2017
Looks like it'll be hard to make progress at this point. |
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by toyoshim@chromium.org
, Sep 22 2016