Bug 239936 – [JSC] Add fast path to TypedArray.from

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

[Jarred Sumner]

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

[Radar WebKit Bug Importer]

<rdar://problem/92917413>

[Yusuke Suzuki]

Pull request: https://github.com/WebKit/WebKit/pull/564

[Yusuke Suzuki]

Right condition to take this fast path is actually subtle :)

[Yusuke Suzuki]

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);

[EWS]

Committed 252976@main (1b440efcb4ae): <https://commits.webkit.org/252976@main>

Reviewed commits have been landed. Closing PR #564 and removing active labels.

[André Bargull]

(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

[Yusuke Suzuki]

(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.

[Yusuke Suzuki]

(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!