Expand the set of objects we take JSArray::fastSlice() path for
Created attachment 447669 [details] Patch
(In reply to Alexey Shvayka from comment #1) > Created attachment 447669 [details] > Patch --outer 50: r287094 patch array-slice-call-cloned-arguments 103.0680+-0.1483 ^ 42.6947+-0.1334 ^ definitely 2.4141x faster
Created attachment 447670 [details] Patch for running Speedometer2/EmberJS-Debug-TodoMVC locally Speedometer2/EmberJS-Debug-TodoMVC score === r287094: 62.99 pt patch: 63.33 pt (0.5% better) Arithmetic mean of 6 runs of `Tools/Scripts/run-perf-tests --repeat=4 --test-runner-count=6 PerformanceTests/Speedometer --no-build --release --no-show-results` with "emberjs-debug-todomvc-tests.patch" applied.
Comment on attachment 447669 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=447669&action=review > Source/JavaScriptCore/runtime/ArrayPrototype.cpp:1080 > + if (LIKELY(speciesResult.first == SpeciesConstructResult::FastPath)) { Why do we not need to check length==toLength any more?
(In reply to Saam Barati from comment #4) > Comment on attachment 447669 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=447669&action=review > > > Source/JavaScriptCore/runtime/ArrayPrototype.cpp:1080 > > + if (LIKELY(speciesResult.first == SpeciesConstructResult::FastPath)) { > > Why do we not need to check length==toLength any more? We are not very interested if "length" has changed, yet we still do need to ensure we don't read OOB. This is now done via `if (startIndex + count > source->butterfly()->vectorLength())`.
Created attachment 447749 [details] Patch Get source structure after ensureWritable() to fix ASSERT failure and explicitly downcast |count| to fix 'watch' build.
Comment on attachment 447749 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=447749&action=review r=me, nice. > JSTests/microbenchmarks/array-slice-call-cloned-arguments.js:14 > +})(); Can we also have a test which performs, array.slice(0, number-exceeding-length-of-array) > Source/JavaScriptCore/runtime/JSArray.cpp:732 > + source->ensureWritable(vm); I think, in a subsequent patch, it is nice if we can avoid making the input as non-CoW since we can create a slice of array from CoW array. Can we have FIXME with bugzilla URL here?
<rdar://problem/86946541>
(In reply to Yusuke Suzuki from comment #7) > r=me, nice. Thank you for review! > Can we also have a test which performs, > > array.slice(0, number-exceeding-length-of-array) Added: JSTests/stress/array-slice-beyond-length.js (first test). However, start / end exceeding length of array is handled by argumentClampedIndexFromStartOrEnd(), so no crash if if (startIndex + count > source->butterfly()->vectorLength()) return nullptr; is removed. I expected the test, added in r175420, to crash if "length" check is removed, no luck either. However, JSTests/test262/test/built-ins/Array/prototype/splice/S15.4.4.12_A3_T1.js did start failing / crashing, so I ported it for slice(): please see the second test in JSTests/stress/array-slice-beyond-length.js. > > Source/JavaScriptCore/runtime/JSArray.cpp:732 > > + source->ensureWritable(vm); > > I think, in a subsequent patch, it is nice if we can avoid making the input > as non-CoW since we can create a slice of array from CoW array. > Can we have FIXME with bugzilla URL here? Added: https://bugs.webkit.org/show_bug.cgi?id=234990.
Created attachment 448648 [details] Patch for landing
Committed r287800 (245853@main): <https://commits.webkit.org/245853@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 448648 [details].