Issue metadata
Sign in to add a comment
|
49.6% regression in blink_perf.bindings at 481089:481103 |
||||||||||||||||||||
Issue descriptionSee the link to graphs below.
,
Jun 22 2017
Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/8976085095106274016
,
Jun 22 2017
Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/8976085080964057888
,
Jun 24 2017
=== Auto-CCing suspected CL author loorongjie@gmail.com === Hi loorongjie@gmail.com, 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 : Loo Rong Jie Commit : 0e4046ac0d81a6eb8c85c1a5bdb9f0cd51bb214b Date : Tue Jun 20 13:37:22 2017 Subject: Remove ~MaybeHandle and statically assert that handles are trivially copyable Bisect Details Configuration: android_webview_nexus6_aosp_perf_bisect Benchmark : blink_perf.bindings Metric : typed-array-construct-from-array/typed-array-construct-from-array Change : 49.60% | 150.613848534 -> 75.9110026882 Revision Result N chromium@481088 150.614 +- 1.57405 5 good chromium@481096 151.402 +- 1.27791 6 good chromium@481100 151.613 +- 2.46808 9 good chromium@481100,v8@b2b38f00c0 150.435 +- 2.17232 7 good chromium@481100,v8@f38f9dcd7e 151.154 +- 2.20495 7 good chromium@481100,v8@0e4046ac0d 76.0394 +- 0.509976 5 bad <-- chromium@481100,v8@6269b2be1e 76.1557 +- 0.977081 5 bad chromium@481100,v8@f6bc208864 75.988 +- 0.490482 5 bad chromium@481101 75.6592 +- 0.634203 5 bad chromium@481102 75.9054 +- 0.450708 4 bad chromium@481103 75.911 +- 0.838781 6 bad To Run This Test src/tools/perf/run_benchmark -v --browser=android-webview --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests blink_perf.bindings Debug Info https://chromeperf.appspot.com/buildbucket_job_status/8976085095106274016 Is this bisect wrong? https://chromeperf.appspot.com/bad_bisect?try_job_id=4555656809414656 | 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!
,
Jun 24 2017
=== BISECT JOB RESULTS === Bisect was unable to run to completion Error: TIMEOUT The bisect was able to narrow the range, you can try running with: good_revision: 06592e080fd1cf4a188265ed4f9bcf826952671a bad_revision : ef0e8704ab0f6dcfd1d88374ed4495f1c173aa04 If failures persist contact the team (see below) and report the error. Bisect Details Configuration: android_webview_nexus6_aosp_perf_bisect Benchmark : blink_perf.bindings Metric : typed-array-construct-from-array/typed-array-construct-from-array Change : 49.53% | 150.704760553 -> 76.0602413135 Revision Result N chromium@481088 150.705 +- 3.03814 6 good chromium@481096 150.709 +- 1.72394 6 good chromium@481103 76.0602 +- 0.81496 5 bad To Run This Test src/tools/perf/run_benchmark -v --browser=android-webview --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests blink_perf.bindings Debug Info https://chromeperf.appspot.com/buildbucket_job_status/8976085080964057888 Is this bisect wrong? https://chromeperf.appspot.com/bad_bisect?try_job_id=4942205074014208 | 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!
,
Jun 24 2017
Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/8975902079585851984
,
Jun 26 2017
=== BISECT JOB RESULTS === Bisect was unable to run to completion Error: TIMEOUT The bisect was able to narrow the range, you can try running with: good_revision: b2b38f00c05d6a6c2e2339af1eff0a1527c734f2 bad_revision : 6269b2be1eea8a5a9c5cfa9998575911c0121bea If failures persist contact the team (see below) and report the error. Bisect Details Configuration: android_webview_nexus6_aosp_perf_bisect Benchmark : blink_perf.bindings Metric : typed-array-construct-from-array/typed-array-construct-from-array Change : 49.89% | 151.419018539 -> 75.8705477739 Revision Result N chromium@481088 151.419 +- 2.3863 8 good chromium@481096 151.264 +- 3.18265 7 good chromium@481100 150.866 +- 2.34024 6 good chromium@481100,v8@b2b38f00c0 150.308 +- 2.04697 5 good chromium@481100,v8@6269b2be1e 76.0299 +- 1.02394 6 bad chromium@481100,v8@f6bc208864 76.1051 +- 0.562084 5 bad chromium@481101 75.8616 +- 0.327685 6 bad chromium@481102 75.8918 +- 1.09461 7 bad chromium@481103 75.8705 +- 0.650185 5 bad To Run This Test src/tools/perf/run_benchmark -v --browser=android-webview --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests blink_perf.bindings Debug Info https://chromeperf.appspot.com/buildbucket_job_status/8975902079585851984 Is this bisect wrong? https://chromeperf.appspot.com/bad_bisect?try_job_id=4942205074014208 | 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!
,
Jul 27 2017
Explictly assigning. A CL you landed tripped one of the speed metrics we measure in the lab. If this is the first time this has happened to one of your CLs, or if it's been a while, please read: https://chromium.googlesource.com/chromium/src/+/master/docs/speed/addressing_performance_regressions.md We're looking for one of the following: 1. Justification via explanation 2. Plan to revert or fix 3. Angry rage throwing of equipment at my head Just be aware that I'm trained in trumpet playing and First Aid and am not afraid to use it. Note: This was a bulk edit message and not very personal.
,
Jul 31 2017
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/1dfaec26477b9bfe2a2cb59d10fdcee82d5ccf09 commit 1dfaec26477b9bfe2a2cb59d10fdcee82d5ccf09 Author: Loo Rong Jie <loorongjie@gmail.com> Date: Mon Jul 31 08:46:37 2017 Partial revert of "Remove ~MaybeHandle and statically assert that handles are trivially copyable" Reason: cause Blink regression on Android Original CL: https://chromium-review.googlesource.com/c/538463/ Bug: chromium:735910 Change-Id: I405e71f6ffeaf9fa467036a6fafa0271a60de9d3 Reviewed-on: https://chromium-review.googlesource.com/593247 Reviewed-by: Yang Guo <yangguo@chromium.org> Commit-Queue: Loo Rong Jie <loorongjie@gmail.com> Cr-Commit-Position: refs/heads/master@{#46995} [modify] https://crrev.com/1dfaec26477b9bfe2a2cb59d10fdcee82d5ccf09/src/base/macros.h [modify] https://crrev.com/1dfaec26477b9bfe2a2cb59d10fdcee82d5ccf09/src/handles.cc [modify] https://crrev.com/1dfaec26477b9bfe2a2cb59d10fdcee82d5ccf09/src/handles.h
,
Aug 3 2017
Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/8972319135800746656
,
Aug 3 2017
Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/8972319124922909520
,
Aug 3 2017
Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/8972319118262724208
,
Aug 3 2017
Do we have any explanation how the original change could have caused the regressions? Also, I don't see the performance graph going back to the original value. Given the numbers, and the nature of the change, I actually vote for reverting the revert.
,
Aug 3 2017
A bit of background: rnk@ was the original CL owner (see [0] for intended performance improvement), but abandoned due to GCC compile error. I revived the CL with GCC workaround in [1]. After getting this Blink regression, I asked rnk@ for help (see [2]). I only have Windows machine so I can't debug Android bug. I reverted the CL just to see if the graph will improve, but unfortunately it doesn't. (I called it "partial revert" because [1] also contains drive-by fix for clang compile warning). I don't understand why Windows/Linux/Mac are not affected though. [0]: https://codereview.chromium.org/2632713003 [1]: https://chromium-review.googlesource.com/c/538463 [2]: https://bugs.chromium.org/p/chromium/issues/detail?id=457078#c97
,
Aug 3 2017
+jbroman, owner of blink_perf.bindings. Jeremy, I'm pretty stumped on this bug. Any ideas what could be going on here? Three bisects see a 50% regression to the blamed CL, but the revert (which also fixes a compile warning) does not bring the graphs back up. loorongjie: The biggest question I have is when was the revert CL rolled into chromium?
,
Aug 3 2017
The revert was contained in v8 version 6.2.100, and rolled into chromium with 6.2.104 in 97152cfcafc0f610b6642eff5eea8d56d1b9bc93. That was two days ago.
,
Aug 3 2017
The revert was actually included in V8 version 6.2.89, rolled into chromium at 6c5fc503dddf55d77a0fefb7bc3bfc0d91cd6987
,
Aug 3 2017
Here are some micro-benchmark regressions caused by the "partial revert" CL: https://bugs.chromium.org/p/chromium/issues/detail?id=752118
,
Aug 3 2017
=== Auto-CCing suspected CL author loorongjie@gmail.com === Hi loorongjie@gmail.com, 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 : Loo Rong Jie Commit : 0e4046ac0d81a6eb8c85c1a5bdb9f0cd51bb214b Date : Tue Jun 20 13:37:22 2017 Subject: Remove ~MaybeHandle and statically assert that handles are trivially copyable Bisect Details Configuration: android_webview_nexus6_aosp_perf_bisect Benchmark : blink_perf.bindings Metric : typed-array-construct-from-array/typed-array-construct-from-array Change : 50.01% | 151.5238271 -> 75.749507698 Revision Result N chromium@481088 151.524 +- 3.54778 8 good chromium@481096 151.279 +- 3.15016 7 good chromium@481100 150.957 +- 1.68914 7 good chromium@481100,v8@b2b38f00c0 150.643 +- 1.98411 6 good chromium@481100,v8@f38f9dcd7e 150.528 +- 3.329 8 good chromium@481100,v8@0e4046ac0d 76.2261 +- 0.668612 5 bad <-- chromium@481100,v8@6269b2be1e 75.9574 +- 0.712338 6 bad chromium@481100,v8@f6bc208864 76.0266 +- 0.342673 6 bad chromium@481101 75.9267 +- 0.498793 5 bad chromium@481102 75.9882 +- 0.315244 5 bad chromium@481103 75.7495 +- 0.780623 5 bad To Run This Test src/tools/perf/run_benchmark -v --browser=android-webview --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests blink_perf.bindings More information on addressing performance regressions: http://g.co/ChromePerformanceRegressions Debug information about this bisect: https://chromeperf.appspot.com/buildbucket_job_status/8972319135800746656 For feedback, file a bug with component Speed>Bisection
,
Aug 3 2017
Locally bisects on Nexus 5 consistently to that CL, and further to the removal of the user-provided destructor from MaybeHandle (as opposed to the other incidental changes).
,
Aug 3 2017
=== BISECT JOB RESULTS === NO Perf regression found, tests failed to produce values Bisect Details Configuration: android_nexus5_perf_bisect Benchmark : blink_perf.bindings Metric : typed-array-construct-from-array/typed-array-construct-from-array 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.bindings More information on addressing performance regressions: http://g.co/ChromePerformanceRegressions Debug information about this bisect: https://chromeperf.appspot.com/buildbucket_job_status/8972319124922909520 For feedback, file a bug with component Speed>Bisection
,
Aug 4 2017
=== BISECT JOB RESULTS === Bisect was unable to run to completion Error: INFRA_FAILURE The bisect was able to narrow the range, you can try running with: good_revision: b2b38f00c05d6a6c2e2339af1eff0a1527c734f2 bad_revision : 6269b2be1eea8a5a9c5cfa9998575911c0121bea If failures persist contact the team (see below) and report the error. Bisect Details Configuration: android_nexus6_perf_bisect Benchmark : blink_perf.bindings Metric : typed-array-construct-from-array/typed-array-construct-from-array Revision Result N chromium@481084 151.223 +- 1.55916 5 good chromium@481097 151.943 +- 0.978009 6 good chromium@481100 151.853 +- 2.84951 8 good chromium@481100,v8@b2b38f00c0 151.424 +- 1.62414 6 good chromium@481100,v8@6269b2be1e 76.4245 +- 0.622845 7 bad chromium@481100,v8@f6bc208864 76.5844 +- 0.42391 4 bad chromium@481101 76.8757 +- 0.549483 4 bad chromium@481102 76.3061 +- 0.632499 6 bad chromium@481103 76.6508 +- 1.18156 5 bad chromium@481109 76.5706 +- 0.93398 6 bad chromium@481133 76.6446 +- 1.19948 6 bad chromium@481182 76.5812 +- 1.1721 7 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.bindings More information on addressing performance regressions: http://g.co/ChromePerformanceRegressions Debug information about this bisect: https://chromeperf.appspot.com/buildbucket_job_status/8972319118262724208 For feedback, file a bug with component Speed>Bisection
,
Aug 4 2017
The benchmark we are talking about is a microbenchmark, creating a Uint8Array from an array of 1 million integers. To me this change in performance looks like a coincidence, maybe it changed some code alignment in v8 or something. Also, the revert had no effect on the performance. Any objections to reverting the revert?
,
Aug 5 2017
Bisected the point where restoring the destructor no longer restores performance that is: https://chromium.googlesource.com/v8/v8/+/41e79062fb7e0f53f4600277a38c508f727dc1a7 Adding and removing ": uint8_t" from InstanceType does the trick. It's not clear to me how this interacts with ~MaybeHandle, but it's consistent for me on my Nexus 5.
,
Aug 7 2017
Thanks for investigating! Looks like another hint that this is just about random memory changes. Both these changes should actually increase performance. We should just ignore this regression. Created a revert of the revert: https://chromium-review.googlesource.com/c/603797 Waiting for explicit LGTM before landing.
,
Aug 7 2017
I agree. It's not the first time I see unexpected regressions due to code moving around in the binary and especially hurting micro-benchmarks. We might see it recover miraculously in the future :) Let's reland for now.
,
Aug 8 2017
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/0ecdbeb026bcaf51715b227e94c8817092142e3b commit 0ecdbeb026bcaf51715b227e94c8817092142e3b Author: Clemens Hammacher <clemensh@chromium.org> Date: Tue Aug 08 08:17:47 2017 Revert "Partial revert of "Remove ~MaybeHandle and statically assert that handles are trivially copyable"" This reverts commit 1dfaec26477b9bfe2a2cb59d10fdcee82d5ccf09. Reason for revert: Does not fix the performance regression, see bug. Original change's description: > Partial revert of "Remove ~MaybeHandle and statically assert that handles are trivially copyable" > > Reason: cause Blink regression on Android > > Original CL: https://chromium-review.googlesource.com/c/538463/ > > Bug: chromium:735910 > Change-Id: I405e71f6ffeaf9fa467036a6fafa0271a60de9d3 > Reviewed-on: https://chromium-review.googlesource.com/593247 > Reviewed-by: Yang Guo <yangguo@chromium.org> > Commit-Queue: Loo Rong Jie <loorongjie@gmail.com> > Cr-Commit-Position: refs/heads/master@{#46995} R=yangguo@chromium.org,loorongjie@gmail.com,jbroman@chromium.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: chromium:735910 Change-Id: I65eecd575fb1b77471c6dd83a01df6c4e8a85214 Reviewed-on: https://chromium-review.googlesource.com/603797 Reviewed-by: Yang Guo <yangguo@chromium.org> Reviewed-by: Clemens Hammacher <clemensh@chromium.org> Commit-Queue: Clemens Hammacher <clemensh@chromium.org> Cr-Commit-Position: refs/heads/master@{#47213} [modify] https://crrev.com/0ecdbeb026bcaf51715b227e94c8817092142e3b/src/base/macros.h [modify] https://crrev.com/0ecdbeb026bcaf51715b227e94c8817092142e3b/src/handles.cc [modify] https://crrev.com/0ecdbeb026bcaf51715b227e94c8817092142e3b/src/handles.h
,
Aug 8 2017
Marking WontFix, since we don't try to restore the original performance, but just ignore this spurious regression. |
|||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||
Comment 1 by kraynov@chromium.org
, Jun 22 2017