New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 618670 link

Starred by 2 users

Issue metadata

Status: WontFix
Owner:
Email to this user bounced
Closed: Jul 2016
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug-Regression



Sign in to add a comment

12.7% regression in blink_perf.dom at 397648:397675

Project Member Reported by pmeenan@chromium.org, Jun 9 2016

Issue description

See the link to graphs below.
 
All graphs for this bug:
  https://chromeperf.appspot.com/group_report?bug_id=618670

Original alerts at time of bug-filing:
  https://chromeperf.appspot.com/group_report?keys=agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgICg3MmkvQkM


Bot(s) for this bug's original alert(s):

android-nexus7v2
re-kicked failed bisect
Project Member

Comment 3 by 42576172...@developer.gserviceaccount.com, Jun 13 2016

Cc: sigbjo...@opera.com
Owner: sigbjo...@opera.com

=== 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!

Comment 4 by sigbjo...@opera.com, Jun 14 2016

Cc: -sigbjo...@opera.com haraken@chromium.org yukishiino@chromium.org bashi@chromium.org
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.
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.

Comment 6 by sigbjo...@opera.com, 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.
Trying another bisect, but the numbers actually look pretty clear to me.
Project Member

Comment 8 by sheriffbot@chromium.org, Jul 6 2016

Labels: -M-53 M-54 MovedFrom-53
Moving this nonessential bug to the next milestone.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 10 by 42576172...@developer.gserviceaccount.com, 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!
Seems to be legit. sigbjornf, can you investigate and/or revert.
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.
Status: WontFix (was: Assigned)
Agreed.  I close this issue as WontFix.

Sign in to add a comment