Issue metadata
Sign in to add a comment
|
12.7% regression in blink_perf.dom at 397648:397675 |
||||||||||||||||||||
Issue descriptionSee the link to graphs below.
,
Jun 13 2016
re-kicked failed bisect
,
Jun 13 2016
=== Auto-CCing suspected CL author sigbjornf@opera.com === Hi sigbjornf@opera.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 : Shrink weak hash tables when adding elements, if needed. Author : sigbjornf Commit description: Hash tables containing weak references tend to be asymmetrically handled -- Blink "user code" will add elements to the hash table, with the garbage collector taking care of removing references to elements that have no other strong references to keep them alive. The weak processing of hash tables isn't capable of shrinking and allocate a new hash table backing store while running, hence the table entries are only cleared. Blink code will rarely do manual removals from these collections, which gives the hash table no opportunity to actually shrink the capacity of the backing store. This can lead to hash tables with a very low load factor, the majority of the entries be deleted and empty slots. To allow for shrinking to happen over hash tables with weak references, add() will check if shrinking is required. R= BUG= Review-Url: https://codereview.chromium.org/2034883002 Cr-Commit-Position: refs/heads/master@{#397667} Commit : c9eb1766e3fa7c5b221ceca2c30ca402bb204014 Date : Fri Jun 03 08:47:37 2016 ===== TESTED REVISIONS ===== Revision Mean Std Dev N Good? chromium@397647 346.831 4.85742 5 good chromium@397661 345.585 8.5689 5 good chromium@397665 335.783 2.94408 5 good chromium@397666 331.976 5.58648 5 good chromium@397667 304.293 3.38246 5 bad <-- chromium@397668 308.077 3.42185 5 bad chromium@397675 302.718 3.53339 5 bad Bisect job ran on: android_nexus7_perf_bisect Bug ID: 618670 Test Command: src/tools/perf/run_benchmark -v --browser=android-chromium --output-format=chartjson --upload-results --also-run-disabled-tests blink_perf.dom Test Metric: select-single-add/select-single-add Relative Change: 12.72% Score: 99.9 Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/android_nexus7_perf_bisect/builds/3010 Job details: https://chromeperf.appspot.com/buildbucket_job_status/9009963227496900144 Not what you expected? We'll investigate and get back to you! https://chromeperf.appspot.com/bad_bisect?try_job_id=5824899331915776 | 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!
,
Jun 14 2016
I don't quite understand the overlap, as the code path added to add() over hash tables by that CL isn't exercised by select-single-add.html's appendChild() loop.
,
Jun 14 2016
Could just be a bad bisect. That happens from time to time though the numbers look pretty solid. That said, the most recent data points from the perf bot looks like it may have recovered. I'll give it a bit more time to be sure before closing.
,
Jun 14 2016
Thanks, I'll do so too - the dip looks clear enough, but I couldn't see it reproducing locally (non-Android) nor the code hitting.
,
Jun 24 2016
Trying another bisect, but the numbers actually look pretty clear to me.
,
Jul 6 2016
Moving this nonessential bug to the next milestone. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jul 12 2016
Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/9007321854762888608
,
Jul 12 2016
===== BISECT JOB RESULTS ===== Status: completed ===== SUSPECTED CL(s) ===== Subject : Shrink weak hash tables when adding elements, if needed. Author : sigbjornf Commit description: Hash tables containing weak references tend to be asymmetrically handled -- Blink "user code" will add elements to the hash table, with the garbage collector taking care of removing references to elements that have no other strong references to keep them alive. The weak processing of hash tables isn't capable of shrinking and allocate a new hash table backing store while running, hence the table entries are only cleared. Blink code will rarely do manual removals from these collections, which gives the hash table no opportunity to actually shrink the capacity of the backing store. This can lead to hash tables with a very low load factor, the majority of the entries be deleted and empty slots. To allow for shrinking to happen over hash tables with weak references, add() will check if shrinking is required. R= BUG= Review-Url: https://codereview.chromium.org/2034883002 Cr-Commit-Position: refs/heads/master@{#397667} Commit : c9eb1766e3fa7c5b221ceca2c30ca402bb204014 Date : Fri Jun 03 08:47:37 2016 ===== TESTED REVISIONS ===== Revision Mean Std Dev N Good? chromium@397647 356.301 5.8408 5 good chromium@397661 351.97 2.89637 5 good chromium@397665 341.375 3.40182 5 good chromium@397666 340.253 4.68277 5 good chromium@397667 311.746 4.83991 5 bad <-- chromium@397668 308.48 2.76669 5 bad chromium@397675 313.802 2.71273 5 bad Bisect job ran on: android_nexus7_perf_bisect Bug ID: 618670 Test Command: src/tools/perf/run_benchmark -v --browser=android-chromium --output-format=chartjson --upload-results --also-run-disabled-tests blink_perf.dom Test Metric: select-single-add/select-single-add Relative Change: 11.93% Score: 99.9 Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/android_nexus7_perf_bisect/builds/3070 Job details: https://chromeperf.appspot.com/buildbucket_job_status/9007321854762888608 Not what you expected? We'll investigate and get back to you! https://chromeperf.appspot.com/bad_bisect?try_job_id=5279168149848064 | 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!
,
Jul 22 2016
Seems to be legit. sigbjornf, can you investigate and/or revert.
,
Jul 25 2016
I reckon this is/was due to secondary effects on a single target, as the perf tests really doesn't exercise the codepath of that change. And given that there's been subsequent changes to select element handling that drops the runs/s to about a third of what it was, it doesn't seem like a valid thing to focus on.
,
Jul 27 2016
Agreed. I close this issue as WontFix. |
|||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||
Comment 1 by pmeenan@chromium.org
, Jun 9 2016