Issue metadata
Sign in to add a comment
|
12.3% regression in blink_perf.parser at 469616:469654 |
||||||||||||||||||||
Issue descriptionSee the link to graphs below.
,
May 8 2017
Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/8980151090452639472
,
May 8 2017
Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/8980151076860542848
,
May 9 2017
=== Auto-CCing suspected CL author gab@chromium.org === Hi gab@chromium.org, the bisect results pointed to your CL, please take a look at the results. === BISECT JOB RESULTS === Perf regression found with culprit Suspected Commit Author : gab Commit : 7af9dc076d2349ade19a95e24f6ee6811e939765 Date : Fri May 05 13:38:54 2017 Subject: Make nesting/running states a RunLoop rather than a MessageLoop concept. Bisect Details Configuration: android_nexus5_perf_bisect Benchmark : blink_perf.parser Metric : textarea-parsing/textarea-parsing Change : 12.63% | 140.057941767 -> 122.370515435 Revision Result N chromium@469615 140.058 +- 1.53271 6 good chromium@469635 139.256 +- 1.83633 6 good chromium@469636 121.583 +- 2.56063 6 bad <-- chromium@469637 122.592 +- 2.30603 6 bad chromium@469638 122.214 +- 1.19008 6 bad chromium@469640 122.254 +- 2.51235 6 bad chromium@469645 122.406 +- 1.43699 6 bad chromium@469654 122.371 +- 0.418935 6 bad To Run This Test src/tools/perf/run_benchmark -v --browser=android-chromium --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests blink_perf.parser Debug Info https://chromeperf.appspot.com/buildbucket_job_status/8980151090452639472 Is this bisect wrong? https://chromeperf.appspot.com/bad_bisect?try_job_id=5806290208555008 | 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 Speed>Bisection. Thank you!
,
May 9 2017
=== BISECT JOB RESULTS === Perf regression found with culprit Suspected Commit Author : gab Commit : 7af9dc076d2349ade19a95e24f6ee6811e939765 Date : Fri May 05 13:38:54 2017 Subject: Make nesting/running states a RunLoop rather than a MessageLoop concept. Bisect Details Configuration: android_nexus5_perf_bisect Benchmark : blink_perf.parser Metric : textarea-parsing/textarea-parsing Change : 12.70% | 140.368400688 -> 122.545366133 Revision Result N chromium@469615 140.368 +- 1.82076 6 good chromium@469635 139.93 +- 1.79148 6 good chromium@469636 121.578 +- 2.93752 6 bad <-- chromium@469637 121.918 +- 2.10799 6 bad chromium@469638 121.65 +- 1.47114 6 bad chromium@469640 121.303 +- 0.83739 6 bad chromium@469645 121.461 +- 1.95554 5 bad chromium@469654 122.545 +- 2.47184 6 bad To Run This Test src/tools/perf/run_benchmark -v --browser=android-chromium --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests blink_perf.parser Debug Info https://chromeperf.appspot.com/buildbucket_job_status/8980151076860542848 Is this bisect wrong? https://chromeperf.appspot.com/bad_bisect?try_job_id=5276914282921984 | 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 Speed>Bisection. Thank you!
,
May 10 2017
I have a theory that this is the change to third_party/WebKit/Source/platform/scheduler/child/scheduler_tqm_delegate_impl.cc in https://codereview.chromium.org/2818533003 which replaces a member method call with a mandatory TLS call. And since IsNested() is called in the Blink scheduler's work loop [1] this could have a performance impact? [1] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc?rcl=41d0368bc22967ae30b2b5097d887f6552efff83&l=286 I have an idea on how to solve it through the step that always going to follow this CL (RunLoop::Delegate), so I'll go ahead and implement that instead of reverting this CL.
,
May 10 2017
@skyostil: does #6 make sense to you?
,
May 11 2017
Please tag with applicable OSs. Thanks.
,
May 11 2017
,
May 11 2017
Hmm, TLS should be ~free on Android so I'm not sure what's going on here. Is it possible we're falsely thinking that we're nested when we're not? The scheduler turns off most optimizations when it's nested.
,
May 18 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/27355196d32f75606b3e43b54bd0d03ef42b4579 commit 27355196d32f75606b3e43b54bd0d03ef42b4579 Author: gab <gab@chromium.org> Date: Thu May 18 06:01:10 2017 Introduce RunLoop::Delegate splitting RunLoop/MessageLoop some more. Separates RunLoop/MessageLoop further. Still some interdependencies to address but this drops friend class RunLoop; from MessageLoop :). It also moves the relevant state from TLS back onto the MessageLoop instance (hidden as private data in RunLoop::Delegate). This is easier to manage and ensures cleanup when MessageLoop goes away (particularly important in unit tests). BUG= 703346 , 719530 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel TBR=phajdan.jr@chromium.org for content\public\test\cache_test_util.h fwd-decl nit Review-Url: https://codereview.chromium.org/2880453003 Cr-Commit-Position: refs/heads/master@{#472695} [modify] https://crrev.com/27355196d32f75606b3e43b54bd0d03ef42b4579/base/message_loop/message_loop.cc [modify] https://crrev.com/27355196d32f75606b3e43b54bd0d03ef42b4579/base/message_loop/message_loop.h [modify] https://crrev.com/27355196d32f75606b3e43b54bd0d03ef42b4579/base/run_loop.cc [modify] https://crrev.com/27355196d32f75606b3e43b54bd0d03ef42b4579/base/run_loop.h [modify] https://crrev.com/27355196d32f75606b3e43b54bd0d03ef42b4579/components/download/internal/scheduler/network_listener_unittest.cc [modify] https://crrev.com/27355196d32f75606b3e43b54bd0d03ef42b4579/content/public/test/cache_test_util.h
,
May 19 2017
Fixed by r472695 (IsNested() is still done from TLS directly so that wasn't it). My theory is that this regression was mostly a hoax, maybe because the test is constructing/destroying MessageLoops in a loop or something (I think previous destruction handling was a bit heavier -- the state now lives privately inside MessageLoop instead of in TLS). Oh well... new design is better for other reasons anyways :) |
|||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||
Comment 1 by kraynov@chromium.org
, May 8 2017