Changeset 287800 in webkit

Timestamp:

Jan 7, 2022, 5:52:18 PM (5 years ago)

Author:

commit-queue@webkit.org

Message:

Expand the set of objects we take JSArray::fastSlice() path for
​https://bugs.webkit.org/show_bug.cgi?id=234539

Patch by Alexey Shvayka <​ashvayka@apple.com> on 2022-01-07
Reviewed by Yusuke Suzuki.

JSTests:

• microbenchmarks/array-slice-call-cloned-arguments.js: Added.
• stress/array-slice-beyond-length.js: Added.
• stress/array-slice-length-lookup.js: Added.

Source/JavaScriptCore:

Currently, Array.prototype's slice() / splice() methods take a fast path only for
JSArray source objects. With this change, gcSafeMemcpy-based path is taken for any
object with ordinary getOwnPropertySlotByIndex() method, which speeds up the common
case of [].slice.call(arguments) by 140% (in strict mode only, see ClonedArguments).

Also, once is ​https://webkit.org/b/234538 resolved, calling Array.prototype.slice()
on a static NodeList, which is a common idiom to acquire map() / filter() methods,
will become faster as well.

This patch was thoroughly evaluated to be spec-perfect and memory-safe:

• indexing mode check and holesMustForwardToPrototype() guarantee that there are no observable userland code to be invoked;
• fastSlice() signature is upgraded to uint64_t so nullptr is returned in case of large "length", resulting in a RangeError being thrown on the slow path;
• to handle the case of source array being shrinked after "length" lookup (see r175420), OOB read check is moved to JSArray::fastSlice() and refined to rely on vectorLength() so the double "length" lookup is avoided (added a test for this).

All this (and more) is well covered by the test262 suite.

This change improves Speedometer2/EmberJS-Debug-TodoMVC score by 0.5%: although the test
is slow on its own, [].slice.call(arguments) is performed ~56k times per run.

• runtime/ArrayPrototype.cpp:

(JSC::JSC_DEFINE_HOST_FUNCTION):

• runtime/JSArray.cpp:

(JSC::JSArray::fastSlice):

• runtime/JSArray.h:

Location:

trunk

Files:

• JSTests/ChangeLog (modified) (1 diff)
• JSTests/microbenchmarks/array-slice-call-cloned-arguments.js (added)
• JSTests/stress/array-slice-beyond-length.js (added)
• JSTests/stress/array-slice-length-lookup.js (added)
• Source/JavaScriptCore/ChangeLog (modified) (1 diff)
• Source/JavaScriptCore/runtime/ArrayPrototype.cpp (modified) (2 diffs)
• Source/JavaScriptCore/runtime/JSArray.cpp (modified) (3 diffs)
• Source/JavaScriptCore/runtime/JSArray.h (modified) (1 diff)

trunk/JSTests/ChangeLog

@@ +1 @@
+2022-01-07  Alexey Shvayka  <ashvayka@apple.com>
+
+        Expand the set of objects we take JSArray::fastSlice() path for
+        https://bugs.webkit.org/show_bug.cgi?id=234539
+
+        Reviewed by Yusuke Suzuki.
+
+        * microbenchmarks/array-slice-call-cloned-arguments.js: Added.
+        * stress/array-slice-beyond-length.js: Added.
+        * stress/array-slice-length-lookup.js: Added.
+
 2022-01-06  Saam Barati  <sbarati@apple.com>
 

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2022-01-07  Alexey Shvayka  <ashvayka@apple.com>
+
+        Expand the set of objects we take JSArray::fastSlice() path for
+        https://bugs.webkit.org/show_bug.cgi?id=234539
+
+        Reviewed by Yusuke Suzuki.
+
+        Currently, Array.prototype's slice() / splice() methods take a fast path only for
+        JSArray source objects. With this change, gcSafeMemcpy-based path is taken for any
+        object with ordinary getOwnPropertySlotByIndex() method, which speeds up the common
+        case of `[].slice.call(arguments)` by 140% (in strict mode only, see ClonedArguments).
+
+        Also, once is https://webkit.org/b/234538 resolved, calling Array.prototype.slice()
+        on a static NodeList, which is a common idiom to acquire map() / filter() methods,
+        will become faster as well.
+
+        This patch was thoroughly evaluated to be spec-perfect and memory-safe:
+
+          - indexing mode check and holesMustForwardToPrototype() guarantee that there
+            are no observable userland code to be invoked;
+          - fastSlice() signature is upgraded to uint64_t so `nullptr` is returned in case
+            of large "length", resulting in a RangeError being thrown on the slow path;
+          - to handle the case of source array being shrinked after "length" lookup (see r175420),
+            OOB read check is moved to JSArray::fastSlice() and refined to rely on vectorLength()
+            so the double "length" lookup is avoided (added a test for this).
+
+        All this (and more) is well covered by the test262 suite.
+
+        This change improves Speedometer2/EmberJS-Debug-TodoMVC score by 0.5%: although the test
+        is slow on its own, `[].slice.call(arguments)` is performed ~56k times per run.
+
+        * runtime/ArrayPrototype.cpp:
+        (JSC::JSC_DEFINE_HOST_FUNCTION):
+        * runtime/JSArray.cpp:
+        (JSC::JSArray::fastSlice):
+        * runtime/JSArray.h:
+
 2022-01-07  Tim Horton  <timothy_horton@apple.com>
 

trunk/Source/JavaScriptCore/runtime/ArrayPrototype.cpp

@@ -1140 +1140 @@
         return { };
 
-    bool okToDoFastPath = speciesResult.first == SpeciesConstructResult::FastPath && isJSArray(thisObj) && length == toLength(globalObject, thisObj);
-    RETURN_IF_EXCEPTION(scope, { });
-    if (LIKELY(okToDoFastPath)) {
-        if (JSArray* result = asArray(thisObj)->fastSlice(globalObject, static_cast<uint32_t>(begin), static_cast<uint32_t>(end - begin)))
+    if (LIKELY(speciesResult.first == SpeciesConstructResult::FastPath)) {
+        if (JSArray* result = JSArray::fastSlice(globalObject, thisObj, begin, end - begin))
             return JSValue::encode(result);
     }
@@ -1236 +1234 @@
 
     JSObject* result = nullptr;
-    bool okToDoFastPath = speciesResult.first == SpeciesConstructResult::FastPath && isJSArray(thisObj) && length == toLength(globalObject, thisObj);
-    RETURN_IF_EXCEPTION(scope, encodedJSValue());
-    if (LIKELY(okToDoFastPath))
-        result = asArray(thisObj)->fastSlice(globalObject, static_cast<uint32_t>(actualStart), static_cast<uint32_t>(actualDeleteCount));
+    if (LIKELY(speciesResult.first == SpeciesConstructResult::FastPath))
+        result = JSArray::fastSlice(globalObject, thisObj, actualStart, actualDeleteCount);
 
     if (!result) {

trunk/Source/JavaScriptCore/runtime/JSArray.cpp

@@ -726 +726 @@
 }
 
-JSArray* JSArray::fastSlice(JSGlobalObject* globalObject, unsigned startIndex, unsigned count)
-{
-    VM& vm = globalObject->vm();
-
-    ensureWritable(vm);
-
-    auto arrayType = indexingMode();
+JSArray* JSArray::fastSlice(JSGlobalObject* globalObject, JSObject* source, uint64_t startIndex, uint64_t count)
+{
+    VM& vm = globalObject->vm();
+
+    // FIXME: Avoid converting the source from CoW since we aren't modifying it.
+    // https://bugs.webkit.org/show_bug.cgi?id=234990
+    source->ensureWritable(vm);
+
+    Structure* sourceStructure = source->structure(vm);
+    if (sourceStructure->typeInfo().interceptsGetOwnPropertySlotByIndexEvenWhenLengthIsNotZero())
+        return nullptr;
+
+    auto arrayType = source->indexingMode() | IsArray;
     switch (arrayType) {
     case ArrayWithDouble:
     case ArrayWithInt32:
     case ArrayWithContiguous: {
-        if (count >= MIN_SPARSE_ARRAY_INDEX || structure(vm)->holesMustForwardToPrototype(vm, this))
+        if (count >= MIN_SPARSE_ARRAY_INDEX || sourceStructure->holesMustForwardToPrototype(vm, source))
+            return nullptr;
+
+        if (startIndex + count > source->butterfly()->vectorLength())
             return nullptr;
 
@@ -746 +755 @@
         ASSERT(!globalObject->isHavingABadTime());
         ObjectInitializationScope scope(vm);
-        JSArray* resultArray = JSArray::tryCreateUninitializedRestricted(scope, resultStructure, count);
+        JSArray* resultArray = JSArray::tryCreateUninitializedRestricted(scope, resultStructure, static_cast<uint32_t>(count));
         if (UNLIKELY(!resultArray))
             return nullptr;
@@ -752 +761 @@
         auto& resultButterfly = *resultArray->butterfly();
         if (arrayType == ArrayWithDouble)
-            gcSafeMemcpy(resultButterfly.contiguousDouble().data(), butterfly()->contiguousDouble().data() + startIndex, sizeof(JSValue) * count);
+            gcSafeMemcpy(resultButterfly.contiguousDouble().data(), source->butterfly()->contiguousDouble().data() + startIndex, sizeof(JSValue) * static_cast<uint32_t>(count));
         else
-            gcSafeMemcpy(resultButterfly.contiguous().data(), butterfly()->contiguous().data() + startIndex, sizeof(JSValue) * count);
+            gcSafeMemcpy(resultButterfly.contiguous().data(), source->butterfly()->contiguous().data() + startIndex, sizeof(JSValue) * static_cast<uint32_t>(count));
 
         ASSERT(resultButterfly.publicLength() == count);

trunk/Source/JavaScriptCore/runtime/JSArray.h

@@ -105 +105 @@
     JS_EXPORT_PRIVATE JSValue pop(JSGlobalObject*);
 
-    JSArray* fastSlice(JSGlobalObject*, unsigned startIndex, unsigned count);
+    static JSArray* fastSlice(JSGlobalObject*, JSObject* source, uint64_t startIndex, uint64_t count);
 
     bool canFastCopy(VM&, JSArray* otherArray);