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

Issue 700835 link

Starred by 5 users

Issue metadata

Status: Fixed
Merged: issue 700364
Owner:
OOO until 2019-02-10
Closed: May 2017
Cc:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 1
Type: Bug-Regression

Blocked on:
issue 713570

Blocking:
issue 698746


Participants' hotlists:
I-TF-Launch


Sign in to add a comment

11.5%-26.6% regression in blink_perf.bindings at 455716:455725

Project Member Reported by hablich@chromium.org, Mar 13 2017

Issue description

See the link to graphs below.
 
Blocking: 698746
Project Member

Comment 5 by 42576172...@developer.gserviceaccount.com, Mar 13 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: 772559ed7f79de8395dfb24eae582172a1840655
  bad_revision : 5e921f820bc0a86566e25b279019ba965535230e

If failures persist contact the team (see below) and report the error.


Bisect Details
  Configuration: android_nexus6_perf_bisect
  Benchmark    : blink_perf.bindings
  Metric       : structured-clone-json-deserialize/structured-clone-json-deserialize

Revision             Result                   N
chromium@455715      81.3517 +- 0.708819      6      good
chromium@455718      81.4549 +- 1.19308       6      good
chromium@455719      80.8302 +- 0.40859       6      good
chromium@455720      94.3986 +- 0.871051      6      bad
chromium@455725      92.1439 +- 2.2167        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 blink_perf.bindings

Debug Info
  https://chromeperf.appspot.com/buildbucket_job_status/8985243928494724064

Is this bisect wrong?
  https://chromeperf.appspot.com/bad_bisect?try_job_id=5210034348228608


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

Comment 6 by 42576172...@developer.gserviceaccount.com, Mar 14 2017

Mergedinto: 700364
Status: Duplicate (was: Untriaged)

=== BISECT JOB RESULTS ===
Perf regression found with culprit

Suspected Commit
  Author : Michael Hablich
  Commit : 7c3936c67a6361371d9a00d83e2c03ecc603ce76
  Date   : Thu Mar 09 08:05:42 2017
  Subject: Version 5.9.34.1 (Turn on I+TF)

Bisect Details
  Configuration: android_nexus6_perf_bisect
  Benchmark    : blink_perf.bindings
  Metric       : typed-array-set-from-typed/typed-array-set-from-typed
  Change       : 25.73% | 276.556918259 -> 205.398933222

Revision                           Result                  N
chromium@455715                    276.557 +- 4.15412      6      good
chromium@455718                    276.617 +- 4.27439      6      good
chromium@455719                    274.987 +- 2.77965      6      good
chromium@455719,v8@7c3936c67a      205.251 +- 1.08145      6      bad       <--
chromium@455719,v8@fbffc377e3      205.64 +- 1.31726       5      bad
chromium@455720                    205.034 +- 1.20953      6      bad
chromium@455725                    205.399 +- 1.64622      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 blink_perf.bindings

Debug Info
  https://chromeperf.appspot.com/buildbucket_job_status/8985225427223778144

Is this bisect wrong?
  https://chromeperf.appspot.com/bad_bisect?try_job_id=6752079206416384


| 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!
Status: Available (was: Duplicate)
Owner: petermarshall@chromium.org
There are two typedArray regressions in the results too.

Will extract the cloning one later.
Status: Assigned (was: Available)
Status: Started (was: Assigned)
Project Member

Comment 12 by 42576172...@developer.gserviceaccount.com, Apr 11 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: 772559ed7f79de8395dfb24eae582172a1840655
  bad_revision : 5e921f820bc0a86566e25b279019ba965535230e

If failures persist contact the team (see below) and report the error.


Bisect Details
  Configuration: android_nexus6_perf_bisect
  Benchmark    : blink_perf.bindings
  Metric       : structured-clone-json-deserialize/structured-clone-json-deserialize

Revision             Result                  N
chromium@455715      81.0188 +- 2.00742      6      good
chromium@455718      80.662 +- 2.08641       6      good
chromium@455719      80.5502 +- 1.53068      6      good
chromium@455720      95.0625 +- 2.77313      6      bad
chromium@455725      93.3168 +- 1.23053      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 blink_perf.bindings

Debug Info
  https://chromeperf.appspot.com/buildbucket_job_status/8982651851751278928

Is this bisect wrong?
  https://chromeperf.appspot.com/bad_bisect?try_job_id=5210034348228608


| 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!
Labels: Merge-Review-58
Peter, do you have any updates on this?
There are 3 benchmarks here:
1. structured-clone-json-deserialize: Back to normal now.

2. typed-array-construct-from-typed: still slower than before, but we are still doing work on these. Realistically, these were implemented in JS before, meaning that if you ever construct more than a few types of typed array, you'll get into megamorphic cases and slowdown considerably, so these aren't really a great measurement anyway. I have a CL underway which might improve the score a little bit more, but this regression is "OK" for now.

3. typed-array-set-from-typed: I don't know why this regressed - we haven't touched it recently. It shares some logic with splice which was recently re-implemented in C++, so it might be an easy target for porting.
Please add applicable OSs.  Thanks!
Labels: -Pri-2 ReleaseBlock-Stable OS-All Pri-1
Is there a fix to merge in for M58? I don't see anything actually committed. 
Labels: -Merge-Review-58
Nope.
Labels: Merge-Request-59
One fix here which should help from-typed and other cases that are similar, which needs to be merged:

https://chromium.googlesource.com/v8/v8/+/356e9246b2f9653f1ef3bf47b3e801391b1824c3


Labels: -Merge-Request-59 Merge-Approved-59
Please merge your change to M59 branch #3071 latest before 4:00 PM PT, Monday (04/24) so we can take it for next week last M59 dev release. Thank you.
Blockedon: 713570
Labels: -Merge-Approved-59
Actually, there is an outstanding perf issue with the CL so we won't merge it back. https://bugs.chromium.org/p/chromium/issues/detail?id=713570
Project Member

Comment 24 by bugdroid1@chromium.org, Apr 27 2017

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

commit c1699fdeefbd290dc068d097a9c5a420c4bb8978
Author: Peter Marshall <petermarshall@chromium.org>
Date: Thu Apr 27 07:37:44 2017

[builtins] Copy TypedArray elements with the elements accessor in Set.

Performance regressed for this with the I+TF switch. This speeds up
the simple case by using optimizations in the elements accessor.

Bug:  chromium:700835 
Change-Id: Iaba30951b93daefa0fb32acd6656ac705cdc73ed
Reviewed-on: https://chromium-review.googlesource.com/483341
Commit-Queue: Peter Marshall <petermarshall@chromium.org>
Reviewed-by: Franziska Hinkelmann <franzih@chromium.org>
Reviewed-by: Yang Guo <yangguo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#44913}
[modify] https://crrev.com/c1699fdeefbd290dc068d097a9c5a420c4bb8978/src/elements.cc
[modify] https://crrev.com/c1699fdeefbd290dc068d097a9c5a420c4bb8978/src/js/typedarray.js

Status: Fixed (was: Started)
Set is now faster than before this regression.
Labels: Merge-TBD
[Auto-generated comment by a script] We noticed that this issue is targeted for M-59; it appears the fix may have landed after branch point, meaning a merge might be required. Please confirm if a merge is required here - if so add Merge-Request-59 label, otherwise remove Merge-TBD label. Thanks.
Labels: -Merge-TBD

Sign in to add a comment