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

Issue 619491 link

Starred by 2 users

Issue metadata

Status: WontFix
Owner:
Closed: Aug 2016
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug-Regression



Sign in to add a comment

27.1% regression in blink_perf.bindings at 398489:398501

Project Member Reported by oth@chromium.org, Jun 13 2016

Issue description

Comment 1 by oth@chromium.org, Jun 13 2016

All graphs for this bug:
  https://chromeperf.appspot.com/group_report?bug_id=619491

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


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

android-galaxy-s5
Project Member

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

Cc: mlippautz@chromium.org
Owner: mlippautz@chromium.org

=== Auto-CCing suspected CL author mlippautz@chromium.org ===

Hi mlippautz@chromium.org, 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 : Track based on JSArrayBuffer addresses on pages instead of the attached
Author  : mlippautz
Commit description:
  backing store.

Details of tracking:
- Scavenge: New space pages are processes in bulk on the main thread
- MC: Unswept pages are processed in bulk in parallel. All other pages
  are processed by the sweeper concurrently.

BUG= chromium:611688 
LOG=N
TEST=cctest/test-array-buffer-tracker/*
CQ_EXTRA_TRYBOTS=tryserver.v8:v8_linux_arm64_gc_stress_dbg,v8_linux_gc_stress_dbg,v8_mac_gc_stress_dbg,v8_linux64_tsan_rel,v8_mac64_asan_rel

Review-Url: https://codereview.chromium.org/2036643002
Cr-Commit-Position: refs/heads/master@{#36798}
Commit  : 839f3fd406426a221d74eb7a33a72794c3c7a548
Date    : Tue Jun 07 17:29:35 2016


===== TESTED REVISIONS =====
Revision                       Mean     Std Dev  N  Good?
chromium@398488                334.453  15.9733  5  good
chromium@398492                354.044  2.78508  5  good
chromium@398494                355.054  4.19312  5  good
chromium@398494,v8@1f2eaa1c14  361.283  2.06264  5  good
chromium@398494,v8@ba3703db61  360.449  1.99176  5  good
chromium@398494,v8@839f3fd406  251.749  2.31745  5  bad    <--
chromium@398494,v8@67af060318  255.025  2.40741  5  bad
chromium@398495                249.427  3.20648  5  bad
chromium@398501                248.483  6.95451  5  bad

Bisect job ran on: android_s5_perf_bisect
Bug ID: 619491

Test Command: src/tools/perf/run_benchmark -v --browser=android-chromium --output-format=chartjson --upload-results --also-run-disabled-tests blink_perf.bindings
Test Metric: typed-array-construct-from-same-type/typed-array-construct-from-same-type
Relative Change: 25.70%
Score: 99.9

Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/android_s5_perf_bisect/builds/702
Job details: https://chromeperf.appspot.com/buildbucket_job_status/9009984205473863024


Not what you expected? We'll investigate and get back to you!
  https://chromeperf.appspot.com/bad_bisect?try_job_id=5787664784031744

| 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!
Project Member

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


===== BISECT JOB RESULTS =====
Status: completed


===== SUSPECTED CL(s) =====
Subject : Track based on JSArrayBuffer addresses on pages instead of the attached
Author  : mlippautz
Commit description:
  backing store.

Details of tracking:
- Scavenge: New space pages are processes in bulk on the main thread
- MC: Unswept pages are processed in bulk in parallel. All other pages
  are processed by the sweeper concurrently.

BUG= chromium:611688 
LOG=N
TEST=cctest/test-array-buffer-tracker/*
CQ_EXTRA_TRYBOTS=tryserver.v8:v8_linux_arm64_gc_stress_dbg,v8_linux_gc_stress_dbg,v8_mac_gc_stress_dbg,v8_linux64_tsan_rel,v8_mac64_asan_rel

Review-Url: https://codereview.chromium.org/2036643002
Cr-Commit-Position: refs/heads/master@{#36798}
Commit  : 839f3fd406426a221d74eb7a33a72794c3c7a548
Date    : Tue Jun 07 17:29:35 2016


===== TESTED REVISIONS =====
Revision                       Mean     Std Dev  N  Good?
chromium@398488                316.851  4.48086  5  good
chromium@398492                331.532  4.38671  5  good
chromium@398494                333.214  7.75391  5  good
chromium@398494,v8@1f2eaa1c14  333.502  5.68011  5  good
chromium@398494,v8@ba3703db61  331.675  1.17657  5  good
chromium@398494,v8@839f3fd406  234.988  3.7618   5  bad    <--
chromium@398494,v8@67af060318  236.899  3.26636  5  bad
chromium@398495                233.138  3.82313  5  bad
chromium@398501                234.57   3.78646  5  bad

Bisect job ran on: android_s5_perf_bisect
Bug ID: 619491

Test Command: src/tools/perf/run_benchmark -v --browser=android-chromium --output-format=chartjson --upload-results --also-run-disabled-tests blink_perf.bindings
Test Metric: typed-array-construct-from-same-type/typed-array-construct-from-same-type
Relative Change: 25.97%
Score: 99.9

Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/android_s5_perf_bisect/builds/708
Job details: https://chromeperf.appspot.com/buildbucket_job_status/9009958215201970416


Not what you expected? We'll investigate and get back to you!
  https://chromeperf.appspot.com/bad_bisect?try_job_id=5812704577585152

| 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!
Cc: -mlippautz@chromium.org hpayer@chromium.org u...@chromium.org
Status: Started (was: Assigned)
Alright, I could also repro on x64 locally. 

The benchmarks harness a fast path for array buffer registration which needs to be properly inlined. On x64 the numbers recover with better inlining so I will prepare a CL for that. In general I am not a big fan of those microbenchmarks with super-tight loops though.
Project Member

Comment 6 by bugdroid1@chromium.org, Jun 17 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/849d6b45ef6358cedf2451a56a6179f380c95cf3

commit 849d6b45ef6358cedf2451a56a6179f380c95cf3
Author: mlippautz <mlippautz@chromium.org>
Date: Fri Jun 17 10:26:18 2016

[heap] Preallocate ArrayBufferTracker for new space pages

BUG= chromium:619491 
LOG=N

Review-Url: https://codereview.chromium.org/2076093002
Cr-Commit-Position: refs/heads/master@{#37063}

[modify] https://crrev.com/849d6b45ef6358cedf2451a56a6179f380c95cf3/src/heap/spaces-inl.h

Doesn't seem to have moved the benchmark at all. Was it supposed to recover some of the loss?
Actually, yes. I checked locally on a N5 (which showed a similar regression) that it not only recovered but improved this benchmark. Will check again once I have access to HW (sometime this week).
Project Member

Comment 9 by bugdroid1@chromium.org, Jun 28 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/8d2ae27808f047ca8b8f90e63a9c8735321d2ad0

commit 8d2ae27808f047ca8b8f90e63a9c8735321d2ad0
Author: mlippautz <mlippautz@chromium.org>
Date: Tue Jun 28 08:36:33 2016

[heap] Optimize ArrayBuffer tracking

With the current approach we only need to track using an unordered set as we can
still access the backing store pointer and length by the time we free the
backing store.

BUG= chromium:619491 ,  chromium:611688 
LOG=N
R=ulan@chromium.org

Review-Url: https://codereview.chromium.org/2107443002
Cr-Commit-Position: refs/heads/master@{#37318}

[modify] https://crrev.com/8d2ae27808f047ca8b8f90e63a9c8735321d2ad0/src/heap/array-buffer-tracker-inl.h
[modify] https://crrev.com/8d2ae27808f047ca8b8f90e63a9c8735321d2ad0/src/heap/array-buffer-tracker.cc
[modify] https://crrev.com/8d2ae27808f047ca8b8f90e63a9c8735321d2ad0/src/heap/array-buffer-tracker.h

Project Member

Comment 10 by bugdroid1@chromium.org, Jun 29 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/0e1eaec71d5c6d05169308a71eaa51571e7c4f70

commit 0e1eaec71d5c6d05169308a71eaa51571e7c4f70
Author: mlippautz <mlippautz@chromium.org>
Date: Wed Jun 29 05:29:24 2016

Revert of [heap] Optimize ArrayBuffer tracking (patchset #5 id:80001 of https://codereview.chromium.org/2107443002/ )

Reason for revert:
Seems to break GPU bots

Original issue's description:
> [heap] Optimize ArrayBuffer tracking
>
> With the current approach we only need to track using an unordered set as we can
> still access the backing store pointer and length by the time we free the
> backing store.
>
> BUG= chromium:619491 ,  chromium:611688 
> LOG=N
> R=ulan@chromium.org
>
> Committed: https://crrev.com/8d2ae27808f047ca8b8f90e63a9c8735321d2ad0
> Cr-Commit-Position: refs/heads/master@{#37318}

TBR=ulan@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= chromium:619491 ,  chromium:611688 

Review-Url: https://codereview.chromium.org/2105273002
Cr-Commit-Position: refs/heads/master@{#37358}

[modify] https://crrev.com/0e1eaec71d5c6d05169308a71eaa51571e7c4f70/src/heap/array-buffer-tracker-inl.h
[modify] https://crrev.com/0e1eaec71d5c6d05169308a71eaa51571e7c4f70/src/heap/array-buffer-tracker.cc
[modify] https://crrev.com/0e1eaec71d5c6d05169308a71eaa51571e7c4f70/src/heap/array-buffer-tracker.h

Project Member

Comment 11 by bugdroid1@chromium.org, Jun 29 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/f58dd088f0886f838f35a38613cd7b1a66f0cd63

commit f58dd088f0886f838f35a38613cd7b1a66f0cd63
Author: mlippautz <mlippautz@chromium.org>
Date: Wed Jun 29 14:53:34 2016

Reland "[heap] Optimize ArrayBuffer tracking"

With the current approach we only need to track using an unordered set as we can
still access the backing store pointer and length by the time we free the
backing store.

Reland:
The issue was fixed in 67b5a501db179412479e827e51c43515e4196b27.

BUG= chromium:619491 ,  chromium:611688 
LOG=N
R=ulan@chromium.org

This reverts commit 0e1eaec71d5c6d05169308a71eaa51571e7c4f70.

Review-Url: https://codereview.chromium.org/2109913003
Cr-Commit-Position: refs/heads/master@{#37399}

[modify] https://crrev.com/f58dd088f0886f838f35a38613cd7b1a66f0cd63/src/heap/array-buffer-tracker-inl.h
[modify] https://crrev.com/f58dd088f0886f838f35a38613cd7b1a66f0cd63/src/heap/array-buffer-tracker.cc
[modify] https://crrev.com/f58dd088f0886f838f35a38613cd7b1a66f0cd63/src/heap/array-buffer-tracker.h

Project Member

Comment 12 by sheriffbot@chromium.org, Jul 5 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
This doesn't seem to have recovered.

mlippautz@, any thoughts?
Agreed, not seeing any recovery here. Any other ideas?
Friendly perf-sheriff ping, any update on this?
I don't see a short-term fix for this regression. However, the concept of the new tracker is needed to make progress in other directions that enable optimizations on real-world websites.

Medium-term we'd like to not rely on full GCs in this case, but have eager scavenges kick in (based on amount of external memory allocated). 

I'll keep this bug open and update it once this lands.
Perf sheriff ping:

mlippautz@, any updates on eager scavenges? Is there another bug that this bug can be blocked on?
Status: WontFix (was: Started)
Will happen at some point, not immediately though. 

The regression in this micro benchmark is expected since we just plainly copy more memory in new space. Marking as WontFix for now.

Sign in to add a comment