Bug 156404 – STP TypedArray.subarray 5x slowdown compared to 9.1

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Arthur Langereis]

In STP (and nightlies) TypedArray.subarray has slowed down by about 5x compared to Safari 9.1.

See the following test page (runs tests on page load):

http://zenmumbler.net/bugs/array_creation.html

On my 2013 iMac, I get the following (representative run, reproducible every time):

SAFARI 9.1
Iterations: 25, Items: 200000
F32: Min: 3.030, Max: 6.795, Avg: 3.951
Sub: Min: 10.705, Max: 14.020, Avg: 12.179
Poo: Min: 33.810, Max: 35.875, Avg: 34.790
Arr: Min: 2.115, Max: 5.265, Avg: 2.761

SAFARI TECHNOLOGY PREVIEW
Iterations: 25, Items: 200000
F32: Min: 2.225, Max: 4.155, Avg: 2.721
Sub: Min: 55.300, Max: 62.155, Avg: 58.000
Poo: Min: 48.705, Max: 49.750, Avg: 49.127
Arr: Min: 1.975, Max: 3.640, Avg: 2.390

So, while creating basic Float32Arrays has improved, using .subarray() has slowed down tremendously, which I feel directly in my 3D engine which deals a lot with binary data.
In general, TypedArrays are already much slower than normal arrays (.set() is also really slow), but this is a remarkable slowdown from one version to the next.

[Filip Pizlo]

Fascinating.  That's a pretty serious regression.  Keith, do you remember what we've done with subarray?

Arthur, are you OK with your http://zenmumbler.net/bugs/array_creation.html benchmark being included in our JSRegress suite?  It's a suite of miscellaneous benchmarks that we run regularly.  That way, after we fix this bug, this would help catch future regressions.

The easiest way to do this is if you uploaded it as an attachment to bugzilla.  That's the best kind of implicit approval that you're OK with this code being in WebKit.

[Keith Miller]

(In reply to comment #1)
> Fascinating.  That's a pretty serious regression.  Keith, do you remember
> what we've done with subarray?
> 

This regression is not particularly surprising. The ES6 spec changes the behavior of the subarray function substantially. I expect that issue here is that the subarray function now uses species constructors, which require several property lookups. Namely, the constructor and the Symbol.species properties. Since the subarray function is in C++ (where we don't cache the property lookups) and the subarrays you are making are very small (4 elements). The cost of the species construction will dominate the cost of the element copying.

[Geoffrey Garen]

(In reply to comment #2)
> (In reply to comment #1)
> > Fascinating.  That's a pretty serious regression.  Keith, do you remember
> > what we've done with subarray?
> > 
> 
> This regression is not particularly surprising. The ES6 spec changes the
> behavior of the subarray function substantially. I expect that issue here is
> that the subarray function now uses species constructors, which require
> several property lookups. Namely, the constructor and the Symbol.species
> properties. Since the subarray function is in C++ (where we don't cache the
> property lookups) and the subarrays you are making are very small (4
> elements). The cost of the species construction will dominate the cost of
> the element copying.

Why not perform the property accesses in JavaScript and then call a helper function to do the allocation?

I don't think we want to consider a 5X regression unsurprising.

[Keith Miller]

(In reply to comment #3)
> (In reply to comment #2)
> > (In reply to comment #1)
> > > Fascinating.  That's a pretty serious regression.  Keith, do you remember
> > > what we've done with subarray?
> > > 
> > 
> > This regression is not particularly surprising. The ES6 spec changes the
> > behavior of the subarray function substantially. I expect that issue here is
> > that the subarray function now uses species constructors, which require
> > several property lookups. Namely, the constructor and the Symbol.species
> > properties. Since the subarray function is in C++ (where we don't cache the
> > property lookups) and the subarrays you are making are very small (4
> > elements). The cost of the species construction will dominate the cost of
> > the element copying.
> 
> Why not perform the property accesses in JavaScript and then call a helper
> function to do the allocation?
> 
> I don't think we want to consider a 5X regression unsurprising.

I agree, unsurprising was the wrong word choice. I think the a JS wrapper would work. I just didn't bother implementing it as I was unsure how popular the subarray function was in practice. If the subarray function is popular then creating a wrapper is something we should consider doing.

[Arthur Langereis]

Created attachment 276133 [details]
Original Testcase

I added the original testcase as requested by Filip. I am fine with this being included in any repository.

The Pool subtest was an experiment to see if I pre-created empty TypedArrays and then initialised them when needed was to test where the bottleneck lies. In Webkit this is a bit faster than the subarray method but e.g. in Firefox it is essentially as fast as plain array filling (with a large up-front cost but that is fine in my case.)

Up to and including 9.1, Webkit is faster or comparable in all tests to Firefox and Chrome but now it is slower in most cases except for plain JS arrays.

For Keith: the reason I'm using subarray a lot is to have a sort-of C++-like array_view where I can give a function an object that can be sub-indexed. In my framework I create interleaved vertex/index buffers that are then indexed with views. I could reorganise this that I don't pass per-vector views but it made the interface a bit simpler.

Basically, I'm doing vertex stream building and manipulation in the browser and I naively assumed that array sub-view creation would be a very lightweight operation but after your comment I understand the overhead associated with it better now.

Thanks for your efforts.

[Arthur Langereis]

Created attachment 276134 [details]
Indexing R/W speed comparison

Adding this file as well as it's directly related: this second test shows how slow TypedArray.set() is as well.

I was surprised that copying data into a new JS array, modifying it and copying the data back into the TypedArray was much faster than either .subarray() -> sub-indexeing or using .set(newValues, offset)

Another issue: bug 120813 referenced the reason why, but it's now 3 years old.

This test can also be run from my server:
http://zenmumbler.net/bugs/array_indexing.html

[Arthur Langereis]

I saw the "NeedsRadar" keyword added, so I took the step to create a copy of this bug in Apple's bugreporter.

The id is 26493032 so I believe the radar link to be rdar://26493032

Do I now change the keyword from NeedsRadar to InRadar?

[Brady Eidson]

(In reply to comment #7)
> I saw the "NeedsRadar" keyword added, so I took the step to create a copy of
> this bug in Apple's bugreporter.
> 
> The id is 26493032 so I believe the radar link to be rdar://26493032
> 
> Do I now change the keyword from NeedsRadar to InRadar?

Thanks for taking action.

That said, NeedsRadar is generally for Apple folks' reference. It's better for us to import a bugzilla to radar directly for a number of reasons.

[Brady Eidson]

<rdar://problem/26493032>

[Keith Miller]

Created attachment 281915 [details]
Patch

[Keith Miller]

Created attachment 282172 [details]
Patch

[Keith Miller]

Created attachment 282174 [details]
micro-benchmark results

[Keith Miller]

Created attachment 282175 [details]
Patch

[Build Bot]

Comment on attachment 282175 [details]
Patch

Attachment 282175 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1582243

New failing tests:
webgl/1.0.2/conformance/rendering/draw-elements-out-of-bounds.html
webgl/1.0.3/conformance/extensions/angle-instanced-arrays-out-of-bounds.html
fast/canvas/webgl/array-unit-tests.html
webgl/1.0.2/conformance/typedarrays/array-unit-tests.html
fast/canvas/webgl/draw-elements-out-of-bounds.html
fast/canvas/webgl/angle-instanced-arrays-out-of-bounds.html
js/regress/typed-array-subarray.html

[Build Bot]

Created attachment 282180 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 282175 [details]
Patch

Attachment 282175 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1582263

New failing tests:
webgl/1.0.2/conformance/rendering/draw-elements-out-of-bounds.html
webgl/1.0.3/conformance/extensions/angle-instanced-arrays-out-of-bounds.html
fast/canvas/webgl/array-unit-tests.html
webgl/1.0.2/conformance/typedarrays/array-unit-tests.html
fast/canvas/webgl/draw-elements-out-of-bounds.html
fast/canvas/webgl/angle-instanced-arrays-out-of-bounds.html
js/regress/typed-array-subarray.html

[Build Bot]

Created attachment 282182 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 282175 [details]
Patch

Attachment 282175 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1582271

New failing tests:
js/regress/typed-array-subarray.html

[Build Bot]

Created attachment 282183 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.4

[Build Bot]

Comment on attachment 282175 [details]
Patch

Attachment 282175 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1582295

New failing tests:
webgl/1.0.2/conformance/rendering/draw-elements-out-of-bounds.html
webgl/1.0.3/conformance/extensions/angle-instanced-arrays-out-of-bounds.html
fast/canvas/webgl/array-unit-tests.html
webgl/1.0.2/conformance/typedarrays/array-unit-tests.html
fast/canvas/webgl/draw-elements-out-of-bounds.html
fast/canvas/webgl/angle-instanced-arrays-out-of-bounds.html
js/regress/typed-array-subarray.html

[Build Bot]

Created attachment 282184 [details]
Archive of layout-test-results from ews112 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Keith Miller]

Created attachment 283322 [details]
Patch

[Keith Miller]

Created attachment 283324 [details]
Patch

[Geoffrey Garen]

Comment on attachment 283324 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=283324&action=review

r=me

> Source/JavaScriptCore/builtins/TypedArrayPrototype.js:31
> +// to lookup their default constructor, which is expensive. If we used the

look up

> Source/JavaScriptCore/builtins/TypedArrayPrototype.js:32
> +// normal speciesConstructor helper we would need to lookup the default

look up

> Source/JavaScriptCore/builtins/TypedArrayPrototype.js:50
> +    // FIXME: This should check @isConstructor(constructor) but it slows things down
> +    // substantially. The lack of a check here is not observable, however, because
> +    // the first thing we will do with the value is attempt to construct the result with it.

If this behavior is observable, you should file a bug with a test case and remove the FIXME.

If this behavior is not observable, it is correct, so you should remove the FIXME.

[Keith Miller]

Created attachment 283326 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 283326 [details]
Patch for landing

Rejecting attachment 283326 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'build', '--no-clean', '--no-update', '--build-style=release', '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
/EWS/WebKit/Source/JavaScriptCore/llint/LLIntThunks.cpp -o /Volumes/Data/EWS/WebKit/WebKitBuild/JavaScriptCore.build/Release/JavaScriptCore.build/Objects-normal/x86_64/LLIntThunks.o

** BUILD FAILED **


The following build commands failed:
	CompileC /Volumes/Data/EWS/WebKit/WebKitBuild/JavaScriptCore.build/Release/JavaScriptCore.build/Objects-normal/x86_64/JSTypedArrayViewPrototype.o runtime/JSTypedArrayViewPrototype.cpp normal x86_64 c++ com.apple.compilers.llvm.clang.1_0.compiler
(1 failure)

Full output: http://webkit-queues.webkit.org/results/1664270

[Keith Miller]

Created attachment 283327 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 283327 [details]
Patch for landing

Clearing flags on attachment: 283327

Committed r203076: <http://trac.webkit.org/changeset/203076>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Arthur Langereis]

Wonderful! Thank you, Keith, for your efforts.