239936 2022-04-30 21:30:21 -0700 [JSC] Add fast path to TypedArray.from 2022-08-05 01:21:31 -0700 1 1 1 Unclassified WebKit JavaScriptCore WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED https://bugs.webkit.org/show_bug.cgi?id=239891 https://bugs.webkit.org/show_bug.cgi?id=240290 InRadar P2 Normal --- 1 jarred ysuzuki andre.bargull saam webkit-bug-importer ysuzuki oldest_to_newest 1865688 0 458644 jarred 2022-04-30 21:30:21 -0700 Created attachment 458644 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 1867507 1 webkit-bug-importer 2022-05-07 21:31:12 -0700 <rdar://problem/92917413> 1867597 2 ysuzuki 2022-05-09 03:24:46 -0700 Pull request: https://github.com/WebKit/WebKit/pull/564 1867599 3 ysuzuki 2022-05-09 03:45:10 -0700 Right condition to take this fast path is actually subtle :) 1887464 4 ysuzuki 2022-07-28 15:18:58 -0700 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); 1887902 5 ews-feeder 2022-07-30 14:15:09 -0700 Committed 252976@main (1b440efcb4ae): <https://commits.webkit.org/252976@main> Reviewed commits have been landed. Closing PR #564 and removing active labels. 1889069 6 andre.bargull 2022-08-04 23:43:56 -0700 (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 1889074 7 ysuzuki 2022-08-05 00:49:44 -0700 (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. 1889078 8 ysuzuki 2022-08-05 01:21:31 -0700 (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! 458644 2022-04-30 21:30:21 -0700 2022-04-30 21:30:21 -0700 microbenchmark microbenchmark.js text/javascript 462 jarred Ly8gVGhpcyBzaG91bGQgcnVuIGluIGJvdGggbm9kZSBhbmQganNjIHNoZWxsCnZhciB0aW1lID0K ICAicHJlY2lzZVRpbWUiIGluIGdsb2JhbFRoaXMKICAgID8gKCkgPT4gZ2xvYmFsVGhpcy5wcmVj aXNlVGltZSgpICogMTAwMAogICAgOiBwZXJmb3JtYW5jZS5ub3cuYmluZChwZXJmb3JtYW5jZSk7 Cgp2YXIgbG9nID0gInByaW50IiBpbiBnbG9iYWxUaGlzID8gZ2xvYmFsVGhpcy5wcmludCA6IGNv bnNvbGUubG9nOwoKdmFyIGExID0gbmV3IFVpbnQ4QXJyYXkoMTAyNCAqIDEwMjQgKiAxKTsKYTEu ZmlsbCg5OSk7CmNvbnN0IHN0YXJ0ID0gdGltZSgpOwp2YXIgYTIgPSBVaW50OEFycmF5LmZyb20o YTEpOwpjb25zdCBlbGFwc2VkID0gdGltZSgpIC0gc3RhcnQ7CmxvZygiRWxhcHNlZDogIiArIGVs YXBzZWQudG9TdHJpbmcoKSArICJtcyIpOwoKbG9nKGEyWzBdKTsKbG9nKGEyW2EyLmxlbmd0aCAt IDFdKTsK