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

Issue 777390 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Last visit 15 days ago
Closed: Jan 2018
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug-Regression



Sign in to add a comment

10.1%-18.8% regression in blink_perf.bindings at 510088:510301

Project Member Reported by pmeenan@chromium.org, Oct 23 2017

Issue description

See the link to graphs below.
 
Project Member

Comment 1 by 42576172...@developer.gserviceaccount.com, Oct 23 2017

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

(For debugging:) Original alerts at time of bug-filing:
  https://chromeperf.appspot.com/group_report?sid=2f18aefa6566e2849a57caed195df0c5714a07e5eb551bb5ff48a5c5ee6f10c2


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

android-nexus5X
android-nexus6
android-one
android-webview-nexus6
chromium-rel-mac-retina
chromium-rel-mac11
chromium-rel-mac11-air
chromium-rel-mac12
chromium-rel-mac12-mini-8gb
chromium-rel-win10
chromium-rel-win7-dual
chromium-rel-win7-gpu-ati
chromium-rel-win7-gpu-intel
chromium-rel-win7-gpu-nvidia
chromium-rel-win7-x64-dual
chromium-rel-win8-dual
linux-release
Project Member

Comment 3 by 42576172...@developer.gserviceaccount.com, Oct 23 2017

Cc: cwhan.t...@gmail.com
Owner: cwhan.t...@gmail.com
Status: Assigned (was: Untriaged)

=== Auto-CCing suspected CL author cwhan.tunz@gmail.com ===

Hi cwhan.tunz@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 : Choongwoo Han
  Commit : 501127995efd9e8ff4be11e2e6927a3e10597fd1
  Date   : Thu Oct 19 08:25:52 2017
  Subject: [typedarrays] Reduce overheads of TA.p.set

Bisect Details
  Configuration: mac_10_12_perf_bisect
  Benchmark    : blink_perf.bindings
  Metric       : typed-array-construct-from-array/typed-array-construct-from-array
  Change       : 10.63% | 510.192308428 -> 455.971164954

Revision                           Result                  N
chromium@510185                    510.192 +- 5.46377      6      good
chromium@510189                    503.123 +- 6.63641      6      good
chromium@510189,v8@501127995e      455.429 +- 3.54785      6      bad       <--
chromium@510189,v8@7a540779ca      455.965 +- 4.15196      6      bad
chromium@510189,v8@25b78853e2      456.018 +- 2.30265      6      bad
chromium@510189,v8@e39629c7f2      453.267 +- 9.54483      6      bad
chromium@510190                    453.594 +- 5.88303      6      bad
chromium@510191                    453.374 +- 5.34411      6      bad
chromium@510193                    452.892 +- 6.54953      6      bad
chromium@510200                    453.378 +- 2.61339      6      bad
chromium@510214                    452.526 +- 7.87786      6      bad
chromium@510245                    453.74 +- 4.23023       6      bad
chromium@510301                    455.971 +- 2.53262      6      bad

Please refer to the following doc on diagnosing blink_perf regressions:
  https://chromium.googlesource.com/chromium/src/+/master/docs/speed/benchmark_harnesses/blink_perf.md

To Run This Test
  src/tools/perf/run_benchmark -v --browser=release --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/8964940552347266672


For feedback, file a bug with component Speed>Bisection
Cc: petermarshall@chromium.org
Adding an offset to each set operation makes it slow.
maybe we can create a dummy JSTypedArray that has adjusted byte offset, instead of adding offset for each set operation. 
example code: https://chromium-review.googlesource.com/#/c/v8/v8/+/735102 

I'm not sure that this is the better way.
What about if we just start the for-loop at offset? 
e.g.

for (uint32_t index = offset; index < length + offset; index++) {
  ...
  dest->set(index, dest->from(int_value));
get operations also use the index, which do not use the offset.
Hmm yep. I'll take a closer look. We might just be able to accept the regression if fixing it will be too messy.
So I don't know the exact reason, but if I change all of the arithmetic in TryCopyElementsHandleFastNumber to use int32_t rather than uint32_t, then it only regresses ~6% instead of ~13%.
wow.. I didn't expect that type castings make such regression. then the solution seems acceptable.

Comment 13 Deleted

What don't we just change index types of FixedTypedArray?

CL: https://chromium-review.googlesource.com/c/v8/v8/+/743581

Test	                                before	        after      	%
TypedArrays/ConstructAllTypedArrays	11973.3	        12176.7  	1.7
TypedArrays/ConstructArrayLike	        1145226.7	1150760.0	0.5
TypedArrays/ConstructBySameTypedArray	284863.3	284826.7	
TypedArrays/ConstructByTypedArray	58407.7	        63623.3	        8.9
TypedArrays/ConstructWithBuffer    	2401350.0	2406936.7	0.2
TypedArrays/Constructor	                2538280.0	2512280.0	-1.0
TypedArrays/CopyWithin	                133490.0	134283.3	0.6
TypedArrays/SetFromArrayLike      	1282810.0	1342126.7	4.6
TypedArrays/SetFromDifferentType	35223.3	        35050.0	        -0.5
TypedArrays/SetFromSameType	        3393953.3	3362230.0	-0.9
TypedArrays/Sort	                10906.7	        12170.0    	11.6
Status: Fixed (was: Assigned)
petermarshall: It looks like the timeseries have recovered, so I'll mark this Fixed. Please re-open if you haven't yet and still want to start the loop at the offset and change the index type.

Sign in to add a comment