Expand the set of objects we take JSArray::fastSlice() path for

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Alexey Shvayka]

Expand the set of objects we take JSArray::fastSlice() path for

[Alexey Shvayka]

Created attachment 447669 [details]
Patch

[Alexey Shvayka]

(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

[Alexey Shvayka]

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.

[Saam Barati]

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?

[Alexey Shvayka]

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

[Alexey Shvayka]

Created attachment 447749 [details]
Patch

Get source structure after ensureWritable() to fix ASSERT failure and explicitly downcast |count| to fix 'watch' build.

[Yusuke Suzuki]

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?

[Radar WebKit Bug Importer]

<rdar://problem/86946541>

[Alexey Shvayka]

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

[Alexey Shvayka]

Created attachment 448648 [details]
Patch for landing

[EWS]

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