GetByVal and PutByVal runtime operations shouldn't fall off a performance cliff when...
authorsbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 15 May 2020 06:55:08 +0000 (06:55 +0000)
committersbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 15 May 2020 06:55:08 +0000 (06:55 +0000)
https://bugs.webkit.org/show_bug.cgi?id=211935

Reviewed by Yusuke Suzuki and Mark Lam.

JSTests:

* microbenchmarks/get-and-put-by-val-double-index-dont-fall-off-a-cliff.js: Added.
(test):

Source/JavaScriptCore:

There were parts in the runtime for get_by_val that weren't properly handling
ints boxed as doubles along the fast path. This could lead to terrible
performance as we could go from double -> string -> int while converting the
subscript into a property to access.

This patch fixes that, and removes the duplicate code we had throughout the
codebase that does this conversion. I'm adding a new functions tryGetAsUint32Index
and tryGetAsInt32 which will handle the double to int conversion.

This is a 10x speedup on the microbenchmark get-and-put-by-val-double-index-dont-fall-off-a-cliff.js

* dfg/DFGOperations.cpp:
(JSC::DFG::putByValInternal):
* jit/JITOperations.cpp:
(JSC::getByVal):
* jsc.cpp:
(functionAsDoubleNumber):
* llint/LLIntSlowPaths.cpp:
(JSC::LLInt::getByVal):
(JSC::LLInt::LLINT_SLOW_PATH_DECL):
* runtime/JSCJSValue.h:
* runtime/JSCJSValueInlines.h:
(JSC::JSValue::tryGetAsUint32Index):
(JSC::JSValue::tryGetAsInt32):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@261731 268f45cc-cd09-0410-ab3c-d52691b4dbfc

JSTests/ChangeLog
JSTests/microbenchmarks/get-and-put-by-val-double-index-dont-fall-off-a-cliff.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/dfg/DFGOperations.cpp
Source/JavaScriptCore/jit/JITOperations.cpp
Source/JavaScriptCore/jsc.cpp
Source/JavaScriptCore/llint/LLIntSlowPaths.cpp
Source/JavaScriptCore/runtime/JSCJSValue.h
Source/JavaScriptCore/runtime/JSCJSValueInlines.h

index ce061f2..bb0d4b0 100644 (file)
@@ -1,3 +1,13 @@
+2020-05-14  Saam Barati  <sbarati@apple.com>
+
+        GetByVal and PutByVal runtime operations shouldn't fall off a performance cliff when the property is an integer boxed as a double
+        https://bugs.webkit.org/show_bug.cgi?id=211935
+
+        Reviewed by Yusuke Suzuki and Mark Lam.
+
+        * microbenchmarks/get-and-put-by-val-double-index-dont-fall-off-a-cliff.js: Added.
+        (test):
+
 2020-05-14  Devin Rousso  <drousso@apple.com>
 
         [ESNext] enable logical assignment operators by default
diff --git a/JSTests/microbenchmarks/get-and-put-by-val-double-index-dont-fall-off-a-cliff.js b/JSTests/microbenchmarks/get-and-put-by-val-double-index-dont-fall-off-a-cliff.js
new file mode 100644 (file)
index 0000000..a7afd3b
--- /dev/null
@@ -0,0 +1,47 @@
+const numPixels = 24000000;
+let source = new Uint8Array(numPixels);
+let target = new Uint8Array(numPixels);
+
+for (let i = 0; i < source.length; ++i) {
+    if (Math.random() > 0.35)
+        source[i] = 0;
+    else
+        source[i] = 1;
+}
+
+const area = {
+    x: asDoubleNumber(1.0),
+    y: asDoubleNumber(1.0),
+    x2: asDoubleNumber(1001.0),
+    y2: asDoubleNumber(1001.0),
+    width: asDoubleNumber(1000.0),
+    height: asDoubleNumber(1000.0),
+    segmentationWidth: asDoubleNumber(6000.0),
+    edgesCacheWidth: asDoubleNumber(6000.0),
+};
+
+function test(source, target, area) {
+    // fast implementation that can't handle edges of segmentation area
+    const segmentationWidth = area.segmentationWidth;
+    const edgesCacheWidth = area.edgesCacheWidth;
+    const {x2, y2} = area;
+
+    for (let y = area.y; y < y2; ++y) {
+        for (let x = area.x; x < x2; ++x) {
+            const sourceIndex = (y * segmentationWidth) + x;
+            const value = source[sourceIndex];
+
+            if (value !== source[sourceIndex - 1] ||
+                value !== source[sourceIndex + 1] ||
+                value !== source[sourceIndex - segmentationWidth] ||
+                value !== source[sourceIndex + segmentationWidth])
+            {
+                const targetIndex = (y * edgesCacheWidth) + x;
+                target[targetIndex] = 1;
+            }
+        }
+    }
+}
+noInline(test);
+
+test(source, target, area);
index 7cdb5bb..f38cb2e 100644 (file)
@@ -1,3 +1,35 @@
+2020-05-14  Saam Barati  <sbarati@apple.com>
+
+        GetByVal and PutByVal runtime operations shouldn't fall off a performance cliff when the property is an integer boxed as a double
+        https://bugs.webkit.org/show_bug.cgi?id=211935
+
+        Reviewed by Yusuke Suzuki and Mark Lam.
+
+        There were parts in the runtime for get_by_val that weren't properly handling
+        ints boxed as doubles along the fast path. This could lead to terrible
+        performance as we could go from double -> string -> int while converting the
+        subscript into a property to access.
+        
+        This patch fixes that, and removes the duplicate code we had throughout the
+        codebase that does this conversion. I'm adding a new functions tryGetAsUint32Index
+        and tryGetAsInt32 which will handle the double to int conversion.
+        
+        This is a 10x speedup on the microbenchmark get-and-put-by-val-double-index-dont-fall-off-a-cliff.js
+
+        * dfg/DFGOperations.cpp:
+        (JSC::DFG::putByValInternal):
+        * jit/JITOperations.cpp:
+        (JSC::getByVal):
+        * jsc.cpp:
+        (functionAsDoubleNumber):
+        * llint/LLIntSlowPaths.cpp:
+        (JSC::LLInt::getByVal):
+        (JSC::LLInt::LLINT_SLOW_PATH_DECL):
+        * runtime/JSCJSValue.h:
+        * runtime/JSCJSValueInlines.h:
+        (JSC::JSValue::tryGetAsUint32Index):
+        (JSC::JSValue::tryGetAsInt32):
+
 2020-05-14  Devin Rousso  <drousso@apple.com>
 
         [ESNext] enable logical assignment operators by default
index 944d9c6..93f5956 100644 (file)
@@ -129,24 +129,12 @@ ALWAYS_INLINE static void putByValInternal(JSGlobalObject* globalObject, VM& vm,
     JSValue property = JSValue::decode(encodedProperty);
     JSValue value = JSValue::decode(encodedValue);
 
-    if (LIKELY(property.isUInt32())) {
-        // Despite its name, JSValue::isUInt32 will return true only for positive boxed int32_t; all those values are valid array indices.
-        ASSERT(isIndex(property.asUInt32()));
+    if (Optional<uint32_t> index = property.tryGetAsUint32Index()) {
         scope.release();
-        putByVal<strict, direct>(globalObject, vm, baseValue, property.asUInt32(), value);
+        putByVal<strict, direct>(globalObject, vm, baseValue, *index, value);
         return;
     }
 
-    if (property.isDouble()) {
-        double propertyAsDouble = property.asDouble();
-        uint32_t propertyAsUInt32 = static_cast<uint32_t>(propertyAsDouble);
-        if (propertyAsDouble == propertyAsUInt32 && isIndex(propertyAsUInt32)) {
-            scope.release();
-            putByVal<strict, direct>(globalObject, vm, baseValue, propertyAsUInt32, value);
-            return;
-        }
-    }
-
     // Don't put to an object if toString throws an exception.
     auto propertyName = property.toPropertyKey(globalObject);
     RETURN_IF_EXCEPTION(scope, void());
@@ -668,16 +656,10 @@ EncodedJSValue JIT_OPERATION operationGetByValCell(JSGlobalObject* globalObject,
 
     JSValue property = JSValue::decode(encodedProperty);
 
-    if (property.isUInt32())
-        RELEASE_AND_RETURN(scope, getByValWithIndex(globalObject, base, property.asUInt32()));
-
-    if (property.isDouble()) {
-        double propertyAsDouble = property.asDouble();
-        uint32_t propertyAsUInt32 = static_cast<uint32_t>(propertyAsDouble);
-        if (propertyAsUInt32 == propertyAsDouble)
-            RELEASE_AND_RETURN(scope, getByValWithIndex(globalObject, base, propertyAsUInt32));
+    if (Optional<uint32_t> index = property.tryGetAsUint32Index())
+        RELEASE_AND_RETURN(scope, getByValWithIndex(globalObject, base, *index));
 
-    } else if (property.isString()) {
+    if (property.isString()) {
         Structure& structure = *base->structure(vm);
         if (JSCell::canUseFastGetOwnProperty(structure)) {
             RefPtr<AtomStringImpl> existingAtomString = asString(property)->toExistingAtomString(globalObject);
@@ -1571,8 +1553,8 @@ EncodedJSValue JIT_OPERATION operationGetByValWithThis(JSGlobalObject* globalObj
     }
     
     PropertySlot slot(thisVal, PropertySlot::PropertySlot::InternalMethodType::Get);
-    if (subscript.isUInt32()) {
-        uint32_t i = subscript.asUInt32();
+    if (Optional<uint32_t> index = subscript.tryGetAsUint32Index()) {
+        uint32_t i = *index;
         if (isJSString(baseValue) && asString(baseValue)->canGetIndex(i))
             return JSValue::encode(asString(baseValue)->getIndex(globalObject, i));
         
index 33aa8c0..6440905 100644 (file)
@@ -715,9 +715,9 @@ static void putByVal(JSGlobalObject* globalObject, JSValue baseValue, JSValue su
 {
     VM& vm = globalObject->vm();
     auto scope = DECLARE_THROW_SCOPE(vm);
-    if (LIKELY(subscript.isUInt32())) {
+    if (Optional<uint32_t> index = subscript.tryGetAsUint32Index()) {
         byValInfo->tookSlowPath = true;
-        uint32_t i = subscript.asUInt32();
+        uint32_t i = *index;
         if (baseValue.isObject()) {
             JSObject* object = asObject(baseValue);
             if (object->canSetIndexQuickly(i, value)) {
@@ -734,7 +734,8 @@ static void putByVal(JSGlobalObject* globalObject, JSValue baseValue, JSValue su
         scope.release();
         baseValue.putByIndex(globalObject, i, value, ecmaMode.isStrict());
         return;
-    } else if (subscript.isInt32()) {
+    } 
+    if (subscript.isInt32()) {
         byValInfo->tookSlowPath = true;
         if (baseValue.isObject())
             byValInfo->arrayProfile->setOutOfBounds();
@@ -757,11 +758,9 @@ static void directPutByVal(JSGlobalObject* globalObject, JSObject* baseObject, J
     VM& vm = globalObject->vm();
     auto scope = DECLARE_THROW_SCOPE(vm);
 
-    if (LIKELY(subscript.isUInt32())) {
-        // Despite its name, JSValue::isUInt32 will return true only for positive boxed int32_t; all those values are valid array indices.
+    if (Optional<uint32_t> maybeIndex = subscript.tryGetAsUint32Index()) {
         byValInfo->tookSlowPath = true;
-        uint32_t index = subscript.asUInt32();
-        ASSERT(isIndex(index));
+        uint32_t index = *maybeIndex;
 
         switch (baseObject->indexingType()) {
         case ALL_INT32_INDEXING_TYPES:
@@ -781,17 +780,6 @@ static void directPutByVal(JSGlobalObject* globalObject, JSObject* baseObject, J
         return;
     }
 
-    if (subscript.isDouble()) {
-        double subscriptAsDouble = subscript.asDouble();
-        uint32_t subscriptAsUInt32 = static_cast<uint32_t>(subscriptAsDouble);
-        if (subscriptAsDouble == subscriptAsUInt32 && isIndex(subscriptAsUInt32)) {
-            byValInfo->tookSlowPath = true;
-            scope.release();
-            baseObject->putDirectIndex(globalObject, subscriptAsUInt32, value, 0, ecmaMode.isStrict() ? PutDirectIndexShouldThrow : PutDirectIndexShouldNotThrow);
-            return;
-        }
-    }
-
     // Don't put to an object if toString threw an exception.
     auto property = subscript.toPropertyKey(globalObject);
     RETURN_IF_EXCEPTION(scope, void());
@@ -1970,8 +1958,8 @@ ALWAYS_INLINE static JSValue getByVal(JSGlobalObject* globalObject, CallFrame* c
         }
     }
 
-    if (subscript.isInt32()) {
-        int32_t i = subscript.asInt32();
+    if (Optional<int32_t> index = subscript.tryGetAsInt32()) {
+        int32_t i = *index;
         if (isJSString(baseValue)) {
             if (i >= 0 && asString(baseValue)->canGetIndex(i))
                 RELEASE_AND_RETURN(scope, asString(baseValue)->getIndex(globalObject, i));
@@ -2083,15 +2071,10 @@ EncodedJSValue JIT_OPERATION operationGetByVal(JSGlobalObject* globalObject, Enc
     if (LIKELY(baseValue.isCell())) {
         JSCell* base = baseValue.asCell();
 
-        if (property.isUInt32())
-            RELEASE_AND_RETURN(scope, getByValWithIndex(globalObject, base, property.asUInt32()));
+        if (Optional<uint32_t> index = property.tryGetAsUint32Index())
+            RELEASE_AND_RETURN(scope, getByValWithIndex(globalObject, base, *index));
 
-        if (property.isDouble()) {
-            double propertyAsDouble = property.asDouble();
-            uint32_t propertyAsUInt32 = static_cast<uint32_t>(propertyAsDouble);
-            if (propertyAsUInt32 == propertyAsDouble && isIndex(propertyAsUInt32))
-                RELEASE_AND_RETURN(scope, getByValWithIndex(globalObject, base, propertyAsUInt32));
-        } else if (property.isString()) {
+        if (property.isString()) {
             Structure& structure = *base->structure(vm);
             if (JSCell::canUseFastGetOwnProperty(structure)) {
                 RefPtr<AtomStringImpl> existingAtomString = asString(property)->toExistingAtomString(globalObject);
index 8d98b2a..5154c2a 100644 (file)
@@ -373,6 +373,7 @@ static EncodedJSValue JSC_HOST_CALL functionMallocInALoop(JSGlobalObject*, CallF
 static EncodedJSValue JSC_HOST_CALL functionTotalCompileTime(JSGlobalObject*, CallFrame*);
 
 static EncodedJSValue JSC_HOST_CALL functionSetUnhandledRejectionCallback(JSGlobalObject*, CallFrame*);
+static EncodedJSValue JSC_HOST_CALL functionAsDoubleNumber(JSGlobalObject*, CallFrame*);
 
 struct Script {
     enum class StrictMode {
@@ -641,6 +642,8 @@ private:
         addFunction(vm, "totalCompileTime", functionTotalCompileTime, 0);
 
         addFunction(vm, "setUnhandledRejectionCallback", functionSetUnhandledRejectionCallback, 1);
+
+        addFunction(vm, "asDoubleNumber", functionAsDoubleNumber, 1);
     }
     
     void addFunction(VM& vm, JSObject* object, const char* name, NativeFunction function, unsigned arguments)
@@ -2492,6 +2495,15 @@ EncodedJSValue JSC_HOST_CALL functionSetUnhandledRejectionCallback(JSGlobalObjec
     return JSValue::encode(jsUndefined());
 }
 
+EncodedJSValue JSC_HOST_CALL functionAsDoubleNumber(JSGlobalObject* globalObject, CallFrame* callFrame)
+{
+    VM& vm = globalObject->vm();
+    auto scope = DECLARE_THROW_SCOPE(vm);
+    double num = callFrame->argument(0).toNumber(globalObject);
+    RETURN_IF_EXCEPTION(scope, encodedJSValue());
+    return JSValue::encode(jsDoubleNumber(num));
+}
+
 // Use SEH for Release builds only to get rid of the crash report dialog
 // (luckily the same tests fail in Release and Debug builds so far). Need to
 // be in a separate main function because the jscmain function requires object
index a1e1d33..e713cc2 100644 (file)
@@ -1012,8 +1012,8 @@ static ALWAYS_INLINE JSValue getByVal(VM& vm, JSGlobalObject* globalObject, Code
         }
     }
     
-    if (subscript.isUInt32()) {
-        uint32_t i = subscript.asUInt32();
+    if (Optional<uint32_t> index = subscript.tryGetAsUint32Index()) {
+        uint32_t i = *index;
         auto& metadata = bytecode.metadata(codeBlock);
         ArrayProfile* arrayProfile = &metadata.m_arrayProfile;
 
@@ -1091,8 +1091,8 @@ LLINT_SLOW_PATH_DECL(slow_path_put_by_val)
     JSValue value = getOperand(callFrame, bytecode.m_value);
     bool isStrictMode = bytecode.m_ecmaMode.isStrict();
     
-    if (LIKELY(subscript.isUInt32())) {
-        uint32_t i = subscript.asUInt32();
+    if (Optional<uint32_t> index = subscript.tryGetAsUint32Index()) {
+        uint32_t i = *index;
         if (baseValue.isObject()) {
             JSObject* object = asObject(baseValue);
             if (object->canSetIndexQuickly(i, value))
@@ -1123,22 +1123,11 @@ LLINT_SLOW_PATH_DECL(slow_path_put_by_val_direct)
     RELEASE_ASSERT(baseValue.isObject());
     JSObject* baseObject = asObject(baseValue);
     bool isStrictMode = bytecode.m_ecmaMode.isStrict();
-    if (LIKELY(subscript.isUInt32())) {
-        // Despite its name, JSValue::isUInt32 will return true only for positive boxed int32_t; all those values are valid array indices.
-        ASSERT(isIndex(subscript.asUInt32()));
-        baseObject->putDirectIndex(globalObject, subscript.asUInt32(), value, 0, isStrictMode ? PutDirectIndexShouldThrow : PutDirectIndexShouldNotThrow);
+    if (Optional<uint32_t> index = subscript.tryGetAsUint32Index()) {
+        baseObject->putDirectIndex(globalObject, *index, value, 0, isStrictMode ? PutDirectIndexShouldThrow : PutDirectIndexShouldNotThrow);
         LLINT_END();
     }
 
-    if (subscript.isDouble()) {
-        double subscriptAsDouble = subscript.asDouble();
-        uint32_t subscriptAsUInt32 = static_cast<uint32_t>(subscriptAsDouble);
-        if (subscriptAsDouble == subscriptAsUInt32 && isIndex(subscriptAsUInt32)) {
-            baseObject->putDirectIndex(globalObject, subscriptAsUInt32, value, 0, isStrictMode ? PutDirectIndexShouldThrow : PutDirectIndexShouldNotThrow);
-            LLINT_END();
-        }
-    }
-
     // Don't put to an object if toString threw an exception.
     auto property = subscript.toPropertyKey(globalObject);
     if (UNLIKELY(throwScope.exception()))
index 7e31759..cc2cc10 100644 (file)
@@ -216,6 +216,8 @@ public:
 
     int32_t asInt32() const;
     uint32_t asUInt32() const;
+    Optional<uint32_t> tryGetAsUint32Index();
+    Optional<int32_t> tryGetAsInt32();
     int64_t asAnyInt() const;
     uint32_t asUInt32AsAnyInt() const;
     int32_t asInt32AsAnyInt() const;
index ddc1f24..011d58b 100644 (file)
@@ -96,6 +96,34 @@ inline double JSValue::asNumber() const
     return isInt32() ? asInt32() : asDouble();
 }
 
+inline Optional<uint32_t> JSValue::tryGetAsUint32Index()
+{
+    if (isUInt32()) {
+        ASSERT(isIndex(asUInt32()));
+        return asUInt32();
+    }
+    if (isNumber()) {
+        double number = asNumber();
+        uint32_t asUint = static_cast<uint32_t>(number);
+        if (static_cast<double>(asUint) == number && isIndex(asUint))
+            return asUint;
+    }
+    return WTF::nullopt;
+}
+
+inline Optional<int32_t> JSValue::tryGetAsInt32()
+{
+    if (isInt32())
+        return asInt32();
+    if (isNumber()) {
+        double number = asNumber();
+        int32_t asInt = static_cast<int32_t>(number);
+        if (static_cast<double>(asInt) == number)
+            return asInt;
+    }
+    return WTF::nullopt;
+}
+
 inline JSValue jsNaN()
 {
     return JSValue(JSValue::EncodeAsDouble, PNaN);