Issue metadata
Sign in to add a comment
|
9.9%-16.9% regression in thread_times.simple_mobile_sites etc at 465577:465890 |
||||||||||||||||||||||||
Issue descriptionSee the link to graphs below.
,
Apr 25 2017
Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/8981387458398743168
,
Apr 25 2017
=== Auto-CCing suspected CL author nhiroki@chromium.org === Hi nhiroki@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 : nhiroki Commit : d56a35fb25dc388a0506174b9e3fda67337313ce Date : Thu Apr 20 01:05:33 2017 Subject: Worker: Introduce per-global-scope task scheduler Bisect Details Configuration: android_nexus6_perf_bisect Benchmark : thread_times.simple_mobile_sites Metric : thread_renderer_compositor_cpu_time_per_frame/thread_renderer_compositor_cpu_time_per_frame Change : 16.27% | 1.67927791147 -> 1.95254814414 Revision Result N chromium@465743 1.67928 +- 0.0322643 6 good chromium@465816 1.68214 +- 0.0264551 6 good chromium@465835 1.69899 +- 0.032187 6 good chromium@465840 1.68025 +- 0.0264842 6 good chromium@465841 1.943 +- 0.0343885 6 bad <-- chromium@465842 1.94132 +- 0.0515649 6 bad chromium@465844 1.95359 +- 0.0438042 6 bad chromium@465853 1.93266 +- 0.0254546 6 bad chromium@465889 1.95255 +- 0.0456112 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 thread_times.simple_mobile_sites Debug Info https://chromeperf.appspot.com/buildbucket_job_status/8981387458398743168 Is this bisect wrong? https://chromeperf.appspot.com/bad_bisect?try_job_id=5778184798208000 | 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!
,
Apr 25 2017
Yes, my global-scope-scheduler patch should be the culprit. See also comments in this review: https://codereview.chromium.org/2806623004/diff/260001/third_party/WebKit/Source/platform/scheduler/child/compositor_worker_scheduler.cc#newcode18 skyostil@: Do you think this can be optimized by task cancellation in base::MessageLoop mentioned in the comments? Do you have any other ideas to improve it?
,
Apr 25 2017
I considered some options... Option 1) Reverting the global-scope-scheduler patch: I'd prefer to avoid this option if possible because this is the prerequisite patch to proceed with further optimization/refactoring projects like off-main-thread worker start and the worker scheduler ( issue 670534 ). Option 2) Optimizing the scheduler implementation: This would be an ideal option, but for now I don't have ideas how to optimize it other than the task cancellation optimization. Option 3) Migrating AnimationWorklet from the compositor thread to a dedicated worker thread and reverting changes in CompositorWorkerScheduler (see issue 666032 ): If we don't run AnimationWorklet on the compositor thread, changes in CompositorWorkerScheduler are not necessary. Reverting the changes would fix the regression, but I'm not sure if we can easily migrate it (I feel it's not easy as far as I can read the issue description).
,
Apr 25 2017
FYI: +haraken@ for perf regression in bindings tests, +flackr@ for perf regression in compositor tests and AnimationWorklet
,
Apr 26 2017
Right, I think this is due to the lack of cancellation support in base::MessageLoop. It shouldn't be too hard to fix.
,
Apr 26 2017
+dcheng@, can you share the status of cancellation support in base::MessageLoop? I heard you have a patch for that. skyostil@, thank you for your input.
,
Apr 27 2017
I have a patch, but there's an issue with thread-safety in a watch dog test. If this is urgent (i.e. for M59), I'll try to remote into my Windows machine later and upload the CL so someone else can take over it.
,
Apr 27 2017
IIUC, the Hiroki's change switches compositor worker from bare base::MessageLoop to the blink task queue. As we've already use MessageLoop in the prev code, I think the cancellation support doesn't contribute to the regression. Maybe, it's the overhead of the scheduler itself?
,
Apr 27 2017
dcheng@, thank you for the quick reply in spite of OOO. My scheduler patch was landed in M60, so this is not urgent and there is no need to hurry :)
,
Apr 27 2017
,
Apr 27 2017
#10: It's true that the MessageLoop was already there and didn't support cancellation, but IIRC the problem is exacerbated when the Blink scheduler sits on top of the MessageLoop and the compositor triggers an unfortunate pattern for posting and cancelling the BeginFrame deadline task. Alex, was that right?
,
Apr 28 2017
Issue 714155 has been merged into this issue.
,
May 2 2017
FYI: I'll be ooo May 3-7. Maybe I can have time to check this issue during the period. Plan: - dcheng@ will land the MessageLoop cancellation patch. - If it doesn't mitigate the perf regression, I'll consider reverting (a part of) the global-scope-scheduler patch to make it for M60 branch.
,
May 2 2017
alexclarke@, any thoughts on c#13?
,
May 2 2017
RE #13 Yes I spent most of December / January refactoring the scheduler to reduce the overhead. The problem is Scheduler::ScheduleBeginImplFrameDeadline which tries to post a task to run at a particular time. The issue with that is it samples now as does postTask and they don't quite agree, typically this task gets posted and cancelled a number of times which results in a lot of pointless delayed tasks. There are three ways of fixing this: 1. Add a PostTaskAt - we had such an api before but it got removed 2. Support cancellation in the base messageloop (+dchang@'s patch) 3. Get the blinks scheduler to talk to the base message pump directly because MessagePump::ScheduleDelayedWork supports cancellation. This used to be my favoured solution but then luck luke project started and it now feels like rowing the wrong direction.
,
May 8 2017
I'm back (but will be ooo again May 15-19, sorry) alexclarke@, thank you for the clarification! dcheng@, I'd like to know the current status of the patch. Do you think it would be possible to land it in M60? As c#15, I'm thinking of reverting my changes if it's difficult to fix it in M60.
,
May 11 2017
UPDATE: dcheng@ is now rebasing the patch and running tests (thank you!)
,
May 23 2017
Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/8978810233224879152
,
May 23 2017
=== BISECT JOB RESULTS === Bisect failed for unknown reasons Please contact the team (see below) and report the error. Bisect Details Configuration: mac_air_perf_bisect Benchmark : blink_perf.bindings Metric : dom-attribute-on-prototoype/dom-attribute-on-prototoype To Run This Test src/tools/perf/run_benchmark -v --browser=release --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests blink_perf.bindings Debug Info https://chromeperf.appspot.com/buildbucket_job_status/8978810233224879152 Is this bisect wrong? https://chromeperf.appspot.com/bad_bisect?try_job_id=6374238933483520 | 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 23 2017
Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/8978809540736792512
,
May 23 2017
Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/8978809512469204656
,
May 23 2017
=== BISECT JOB RESULTS === Bisect failed for unknown reasons Please contact the team (see below) and report the error. Bisect Details Configuration: mac_air_perf_bisect Benchmark : blink_perf.bindings Metric : dom-attribute-on-prototoype/dom-attribute-on-prototoype To Run This Test src/tools/perf/run_benchmark -v --browser=release --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests blink_perf.bindings Debug Info https://chromeperf.appspot.com/buildbucket_job_status/8978809540736792512 Is this bisect wrong? https://chromeperf.appspot.com/bad_bisect?try_job_id=5263806009180160 | 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 23 2017
Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/8978808733867271344
,
May 23 2017
Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/8978808729679629648
,
May 23 2017
=== BISECT JOB RESULTS === Bisect failed for unknown reasons Please contact the team (see below) and report the error. Bisect Details Configuration: mac_air_perf_bisect Benchmark : blink_perf.bindings Metric : dom-attribute-on-prototoype/dom-attribute-on-prototoype To Run This Test src/tools/perf/run_benchmark -v --browser=release --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests blink_perf.bindings Debug Info https://chromeperf.appspot.com/buildbucket_job_status/8978808733867271344 Is this bisect wrong? https://chromeperf.appspot.com/bad_bisect?try_job_id=5263806009180160 | 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 23 2017
=== BISECT JOB RESULTS === Bisect failed for unknown reasons Please contact the team (see below) and report the error. Bisect Details Configuration: mac_air_perf_bisect Benchmark : blink_perf.bindings Metric : dom-attribute-on-prototoype/dom-attribute-on-prototoype To Run This Test src/tools/perf/run_benchmark -v --browser=release --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests blink_perf.bindings Debug Info https://chromeperf.appspot.com/buildbucket_job_status/8978808729679629648 Is this bisect wrong? https://chromeperf.appspot.com/bad_bisect?try_job_id=6374238933483520 | 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 23 2017
Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/8978802104008750448
,
May 23 2017
=== BISECT JOB RESULTS === Perf regression found with culprit Suspected Commit Author : lpy Commit : 0462cff6ac0ddd51d88e74eb2e356061bbe792b0 Date : Wed Apr 19 22:44:42 2017 Subject: Node.prototype.baseURI should not be nullable Bisect Details Configuration: mac_10_12_perf_bisect Benchmark : blink_perf.bindings Metric : dom-attribute-on-prototoype/dom-attribute-on-prototoype Change : 19.51% | 179.59967201 -> 144.555302621 Revision Result N chromium@465751 179.6 +- 3.59202 6 good chromium@465787 177.79 +- 5.16381 6 good chromium@465790 175.891 +- 8.22447 5 good chromium@465791 152.665 +- 9.25344 6 bad <-- chromium@465792 148.987 +- 4.16039 6 bad chromium@465796 146.193 +- 10.0622 6 bad chromium@465804 149.355 +- 6.74099 6 bad chromium@465821 141.575 +- 17.2138 6 bad chromium@465890 144.555 +- 7.08732 6 bad To Run This Test src/tools/perf/run_benchmark -v --browser=release --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests blink_perf.bindings Debug Info https://chromeperf.appspot.com/buildbucket_job_status/8978809512469204656 Is this bisect wrong? https://chromeperf.appspot.com/bad_bisect?try_job_id=6384774219825152 | 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 23 2017
=== BISECT JOB RESULTS === Perf regression found with culprit Suspected Commit Author : lpy Commit : 0462cff6ac0ddd51d88e74eb2e356061bbe792b0 Date : Wed Apr 19 22:44:42 2017 Subject: Node.prototype.baseURI should not be nullable Bisect Details Configuration: mac_10_12_perf_bisect Benchmark : blink_perf.bindings Metric : dom-attribute-on-prototoype/dom-attribute-on-prototoype Change : 18.51% | 178.573524128 -> 145.521544527 Revision Result N chromium@465751 178.574 +- 6.53725 6 good chromium@465787 176.913 +- 5.58004 6 good chromium@465790 179.79 +- 1.62553 6 good chromium@465791 153.667 +- 5.74673 6 bad <-- chromium@465792 149.431 +- 9.28853 6 bad chromium@465796 150.344 +- 8.04228 6 bad chromium@465804 150.989 +- 8.75031 6 bad chromium@465821 144.371 +- 9.12846 6 bad chromium@465890 145.522 +- 8.66176 6 bad To Run This Test src/tools/perf/run_benchmark -v --browser=release --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests blink_perf.bindings Debug Info https://chromeperf.appspot.com/buildbucket_job_status/8978802104008750448 Is this bisect wrong? https://chromeperf.appspot.com/bad_bisect?try_job_id=5570676657750016 | 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 24 2017
I inspected the perf graphs again, and found that following regressions were not relevant to my changes and they were already fixed. - typed-array-construct-from-array - peak_event_size_rate_avg - peak_event_rate_avg - http___rawgit.com_WebKit_webkit_master_PerformanceTests_Animometer_developer.html? In addition, bisect jobs reported that "dom-attribute-on-prototype" was caused by issue 714901 . So, the real regression caused by my changes is only "thread_renderer_compositor_cpu_time_per_frame". It looks like the diff would be subtle and we might not have to revert the changes, I'm now testing a revert CL[1] just in case though. [1] https://codereview.chromium.org/2877103002/
,
May 26 2017
re: c#32, I tested the revert CL but didn't improve the results. The diff would be quite small so I'll keep my changes. Please feel free to reopen this if you think this is not an ignorable regression. FYI: dcheng@'s promising optimization patch is now under review: https://chromium-review.googlesource.com/c/505274/ |
|||||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||||
Comment 1 by rsch...@chromium.org
, Apr 25 2017