Bug 239936

Summary: [JSC] Add fast path to TypedArray.from
Product: WebKit Reporter: Jarred Sumner <jarred>
Component: JavaScriptCoreAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: andre.bargull, saam, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=239891
https://bugs.webkit.org/show_bug.cgi?id=240290
Attachments:
Description Flags
microbenchmark none

Description Jarred Sumner 2022-04-30 21:30:21 PDT 
Created attachment 458644 [details]
microbenchmark

TypedArray.from(otherTypedArrayView) uses Symbol.iterator when it could use memmove() or possibly memcpy.

A microbenchmark is attached that runs in both JSC shell and node.

On macOS 12.3 aarch64 M1X

For 1 MB:
- JSC: 22ms
- Node : 0.13ms

For 32 MB:
- JSC: 274ms
- Node: 4ms


If TypedArray.prototype.set[0] is used in TypedArrayConstructor.js when !mapFn && @isTypedArrayView(arrayLike), that becomes:

For 1 MB:
- JSC: 22ms
- JSC (modified): 0.4ms
- v8 9.6 (Node 17.7.1): 0.13ms

For 32 MB:
- JSC: 274ms
- JSC (modified): 5ms
- v8 9.6 (Node 17.7.1): 4ms


It isn't precisely correct to use TypedArray.prototype.set, but it shows roughly what the numbers would look like after this is fixed. It does seem that V8 is consistently slightly faster after this change though, which means there is still something more to do here. Maybe it could allocate uninitialized memory because it will copy directly from the other typed array view?

V8's optimization for TypedArray.from: https://github.com/v8/v8/blob/f32335fea75b7bde495e0800d7f7349253f81a7c/src/builtins/typed-array-from.tq#L167

[0]: https://gist.github.com/Jarred-Sumner/543b94142de9f17a9ec86e9dac5cf171#file-typedarrayconstructor-js
Comment 1 Radar WebKit Bug Importer 2022-05-07 21:31:12 PDT 
<rdar://problem/92917413>
Comment 2 Yusuke Suzuki 2022-05-09 03:24:46 PDT 
Pull request: https://github.com/WebKit/WebKit/pull/564
Comment 3 Yusuke Suzuki 2022-05-09 03:45:10 PDT 
Right condition to take this fast path is actually subtle :)
Comment 4 Yusuke Suzuki 2022-07-28 15:18:58 PDT 
BTW, it seems that SpiderMonkey and V8 optimize this function incorrectly. The following test pass only in JSC (and from the spec, JSC's behavior is correct since we use ArrayIterator, which uses "length" property, not TypedArrayLength).

function shouldBe(actual, expected) {
    if (actual !== expected)
        throw new Error('bad value: ' + actual);
}

var array = new Uint8Array(128);
Reflect.defineProperty(array, 'length', {
    value: 42
});
var result = Uint8Array.from(array);
shouldBe(result.length, 42);
Comment 5 EWS 2022-07-30 14:15:09 PDT 
Committed 252976@main (1b440efcb4ae): <https://commits.webkit.org/252976@main>

Reviewed commits have been landed. Closing PR #564 and removing active labels.
Comment 6 André Bargull 2022-08-04 23:43:56 PDT 
(In reply to Yusuke Suzuki from comment #4)
> BTW, it seems that SpiderMonkey and V8 optimize this function incorrectly.
> The following test pass only in JSC (and from the spec, JSC's behavior is
> correct since we use ArrayIterator, which uses "length" property, not
> TypedArrayLength).

It's actually the other way around. JSC is incorrectly using the "length" [1] property, whereas the spec mandates that for objects with a [[TypedArrayName]] internal slot, the [[ArrayLength]] internal slot is read [2]. :-)

[1] https://github.com/WebKit/WebKit/blob/3f019cf4b5d2b381db5af9d2751583f7871ba8bf/Source/JavaScriptCore/builtins/ArrayIteratorPrototype.js#L53
[2] https://tc39.es/ecma262/#sec-createarrayiterator
Comment 7 Yusuke Suzuki 2022-08-05 00:49:44 PDT 
(In reply to André Bargull from comment #6)
> (In reply to Yusuke Suzuki from comment #4)
> > BTW, it seems that SpiderMonkey and V8 optimize this function incorrectly.
> > The following test pass only in JSC (and from the spec, JSC's behavior is
> > correct since we use ArrayIterator, which uses "length" property, not
> > TypedArrayLength).
> 
> It's actually the other way around. JSC is incorrectly using the "length"
> [1] property, whereas the spec mandates that for objects with a
> [[TypedArrayName]] internal slot, the [[ArrayLength]] internal slot is read
> [2]. :-)
> 
> [1]
> https://github.com/WebKit/WebKit/blob/
> 3f019cf4b5d2b381db5af9d2751583f7871ba8bf/Source/JavaScriptCore/builtins/
> ArrayIteratorPrototype.js#L53
> [2] https://tc39.es/ecma262/#sec-createarrayiterator

Oh, interesting. We can make the implementation simpler.
Comment 8 Yusuke Suzuki 2022-08-05 01:21:31 PDT 
(In reply to Yusuke Suzuki from comment #7)
> (In reply to André Bargull from comment #6)
> > (In reply to Yusuke Suzuki from comment #4)
> > > BTW, it seems that SpiderMonkey and V8 optimize this function incorrectly.
> > > The following test pass only in JSC (and from the spec, JSC's behavior is
> > > correct since we use ArrayIterator, which uses "length" property, not
> > > TypedArrayLength).
> > 
> > It's actually the other way around. JSC is incorrectly using the "length"
> > [1] property, whereas the spec mandates that for objects with a
> > [[TypedArrayName]] internal slot, the [[ArrayLength]] internal slot is read
> > [2]. :-)
> > 
> > [1]
> > https://github.com/WebKit/WebKit/blob/
> > 3f019cf4b5d2b381db5af9d2751583f7871ba8bf/Source/JavaScriptCore/builtins/
> > ArrayIteratorPrototype.js#L53
> > [2] https://tc39.es/ecma262/#sec-createarrayiterator
> 
> Oh, interesting. We can make the implementation simpler.

Will be fixed in https://github.com/WebKit/WebKit/pull/3038, it makes implementation simpler!