Don't mark an array profile out of bounds for the cases where the DFG will convert...
authorsbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 20 Feb 2018 02:00:39 +0000 (02:00 +0000)
committersbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 20 Feb 2018 02:00:39 +0000 (02:00 +0000)
https://bugs.webkit.org/show_bug.cgi?id=182912
<rdar://problem/37685083>

Reviewed by Keith Miller.

In the baseline JIT and LLInt, when we loading a hole from an original array,
with the array prototype chain being normal, we end up marking the ArrayProfile
for that GetByVal as out of bounds. However, the DFG knows exactly how to
optimize this case by returning undefined when loading from a hole. Currently,
it only does this for Contiguous arrays (and sometimes Double arrays).
This patch just makes sure to not mark the ArrayProfile as out of bounds
in this scenario for Contiguous arrays, since the DFG will always optimize
this case.

However, we should extend this by profiling when a GetByVal loads a hole. By
doing so, we can optimize this for Int32, ArrayStorage, and maybe even Double
arrays. That work will happen in:
https://bugs.webkit.org/show_bug.cgi?id=182940

This patch is a 30-50%  speedup on JetStream's hash-map test. This patch
speeds up JetStream by 1% when testing on my iMac.

* dfg/DFGArrayMode.cpp:
(JSC::DFG::ArrayMode::refine const):
* dfg/DFGFixupPhase.cpp:
(JSC::DFG::FixupPhase::fixupNode):
* jit/JITOperations.cpp:
(JSC::getByVal):
(JSC::canAccessArgumentIndexQuickly): Deleted.
* llint/LLIntSlowPaths.cpp:
(JSC::LLInt::getByVal):
(JSC::LLInt::LLINT_SLOW_PATH_DECL):
* llint/LowLevelInterpreter32_64.asm:
* llint/LowLevelInterpreter64.asm:
* runtime/CommonSlowPaths.h:
(JSC::CommonSlowPaths::canAccessArgumentIndexQuickly):

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

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/dfg/DFGArrayMode.cpp
Source/JavaScriptCore/dfg/DFGFixupPhase.cpp
Source/JavaScriptCore/jit/JITOperations.cpp
Source/JavaScriptCore/llint/LLIntSlowPaths.cpp
Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm
Source/JavaScriptCore/llint/LowLevelInterpreter64.asm
Source/JavaScriptCore/runtime/CommonSlowPaths.h

index d31a1af..7ceddb0 100644 (file)
@@ -1,3 +1,43 @@
+2018-02-19  Saam Barati  <sbarati@apple.com>
+
+        Don't mark an array profile out of bounds for the cases where the DFG will convert the access to SaneChain
+        https://bugs.webkit.org/show_bug.cgi?id=182912
+        <rdar://problem/37685083>
+
+        Reviewed by Keith Miller.
+
+        In the baseline JIT and LLInt, when we loading a hole from an original array,
+        with the array prototype chain being normal, we end up marking the ArrayProfile
+        for that GetByVal as out of bounds. However, the DFG knows exactly how to
+        optimize this case by returning undefined when loading from a hole. Currently,
+        it only does this for Contiguous arrays (and sometimes Double arrays).
+        This patch just makes sure to not mark the ArrayProfile as out of bounds
+        in this scenario for Contiguous arrays, since the DFG will always optimize
+        this case.
+        
+        However, we should extend this by profiling when a GetByVal loads a hole. By
+        doing so, we can optimize this for Int32, ArrayStorage, and maybe even Double
+        arrays. That work will happen in:
+        https://bugs.webkit.org/show_bug.cgi?id=182940
+        
+        This patch is a 30-50%  speedup on JetStream's hash-map test. This patch
+        speeds up JetStream by 1% when testing on my iMac.
+
+        * dfg/DFGArrayMode.cpp:
+        (JSC::DFG::ArrayMode::refine const):
+        * dfg/DFGFixupPhase.cpp:
+        (JSC::DFG::FixupPhase::fixupNode):
+        * jit/JITOperations.cpp:
+        (JSC::getByVal):
+        (JSC::canAccessArgumentIndexQuickly): Deleted.
+        * llint/LLIntSlowPaths.cpp:
+        (JSC::LLInt::getByVal):
+        (JSC::LLInt::LLINT_SLOW_PATH_DECL):
+        * llint/LowLevelInterpreter32_64.asm:
+        * llint/LowLevelInterpreter64.asm:
+        * runtime/CommonSlowPaths.h:
+        (JSC::CommonSlowPaths::canAccessArgumentIndexQuickly):
+
 2018-02-17  Filip Pizlo  <fpizlo@apple.com>
 
         GetArrayMask should support constant folding
index 19da3e4..3075bdf 100644 (file)
@@ -210,12 +210,16 @@ ArrayMode ArrayMode::refine(
         // If we have an OriginalArray and the JSArray prototype chain is sane,
         // any indexed access always return undefined. We have a fast path for that.
         JSGlobalObject* globalObject = graph.globalObjectFor(node->origin.semantic);
+        Structure* arrayPrototypeStructure = globalObject->arrayPrototype()->structure();
+        Structure* objectPrototypeStructure = globalObject->objectPrototype()->structure();
         if ((node->op() == GetByVal || canBecomeGetArrayLength(graph, node))
             && arrayClass() == Array::OriginalArray
-            && globalObject->arrayPrototypeChainIsSane()
-            && !graph.hasExitSite(node->origin.semantic, OutOfBounds)) {
-            graph.registerAndWatchStructureTransition(globalObject->arrayPrototype()->structure());
-            graph.registerAndWatchStructureTransition(globalObject->objectPrototype()->structure());
+            && !graph.hasExitSite(node->origin.semantic, OutOfBounds)
+            && arrayPrototypeStructure->transitionWatchpointSetIsStillValid()
+            && objectPrototypeStructure->transitionWatchpointSetIsStillValid()
+            && globalObject->arrayPrototypeChainIsSane()) {
+            graph.registerAndWatchStructureTransition(arrayPrototypeStructure);
+            graph.registerAndWatchStructureTransition(objectPrototypeStructure);
             if (globalObject->arrayPrototypeChainIsSane())
                 return withSpeculation(Array::SaneChain);
         }
index 87c67dc..de799ff 100644 (file)
@@ -722,55 +722,58 @@ private:
             case Array::Double:
                 if (arrayMode.arrayClass() == Array::OriginalArray
                     && arrayMode.speculation() == Array::InBounds) {
-                    JSGlobalObject* globalObject = m_graph.globalObjectFor(node->origin.semantic);
-                    if (globalObject->arrayPrototypeChainIsSane()) {
-                        // Check if SaneChain will work on a per-type basis. Note that:
-                        //
-                        // 1) We don't want double arrays to sometimes return undefined, since
-                        // that would require a change to the return type and it would pessimise
-                        // things a lot. So, we'd only want to do that if we actually had
-                        // evidence that we could read from a hole. That's pretty annoying.
-                        // Likely the best way to handle that case is with an equivalent of
-                        // SaneChain for OutOfBounds. For now we just detect when Undefined and
-                        // NaN are indistinguishable according to backwards propagation, and just
-                        // use SaneChain in that case. This happens to catch a lot of cases.
-                        //
-                        // 2) We don't want int32 array loads to have to do a hole check just to
-                        // coerce to Undefined, since that would mean twice the checks.
-                        //
-                        // This has two implications. First, we have to do more checks than we'd
-                        // like. It's unfortunate that we have to do the hole check. Second,
-                        // some accesses that hit a hole will now need to take the full-blown
-                        // out-of-bounds slow path. We can fix that with:
-                        // https://bugs.webkit.org/show_bug.cgi?id=144668
+                    // Check if SaneChain will work on a per-type basis. Note that:
+                    //
+                    // 1) We don't want double arrays to sometimes return undefined, since
+                    // that would require a change to the return type and it would pessimise
+                    // things a lot. So, we'd only want to do that if we actually had
+                    // evidence that we could read from a hole. That's pretty annoying.
+                    // Likely the best way to handle that case is with an equivalent of
+                    // SaneChain for OutOfBounds. For now we just detect when Undefined and
+                    // NaN are indistinguishable according to backwards propagation, and just
+                    // use SaneChain in that case. This happens to catch a lot of cases.
+                    //
+                    // 2) We don't want int32 array loads to have to do a hole check just to
+                    // coerce to Undefined, since that would mean twice the checks.
+                    //
+                    // This has two implications. First, we have to do more checks than we'd
+                    // like. It's unfortunate that we have to do the hole check. Second,
+                    // some accesses that hit a hole will now need to take the full-blown
+                    // out-of-bounds slow path. We can fix that with:
+                    // https://bugs.webkit.org/show_bug.cgi?id=144668
+                    
+                    bool canDoSaneChain = false;
+                    switch (arrayMode.type()) {
+                    case Array::Contiguous:
+                        // This is happens to be entirely natural. We already would have
+                        // returned any JSValue, and now we'll return Undefined. We still do
+                        // the check but it doesn't require taking any kind of slow path.
+                        canDoSaneChain = true;
+                        break;
                         
-                        bool canDoSaneChain = false;
-                        switch (arrayMode.type()) {
-                        case Array::Contiguous:
-                            // This is happens to be entirely natural. We already would have
-                            // returned any JSValue, and now we'll return Undefined. We still do
-                            // the check but it doesn't require taking any kind of slow path.
+                    case Array::Double:
+                        if (!(node->flags() & NodeBytecodeUsesAsOther)) {
+                            // Holes look like NaN already, so if the user doesn't care
+                            // about the difference between Undefined and NaN then we can
+                            // do this.
                             canDoSaneChain = true;
-                            break;
-                            
-                        case Array::Double:
-                            if (!(node->flags() & NodeBytecodeUsesAsOther)) {
-                                // Holes look like NaN already, so if the user doesn't care
-                                // about the difference between Undefined and NaN then we can
-                                // do this.
-                                canDoSaneChain = true;
-                            }
-                            break;
-                            
-                        default:
-                            break;
                         }
+                        break;
                         
-                        if (canDoSaneChain) {
-                            m_graph.registerAndWatchStructureTransition(globalObject->arrayPrototype()->structure());
-                            m_graph.registerAndWatchStructureTransition(globalObject->objectPrototype()->structure());
-                            if (globalObject->arrayPrototypeChainIsSane())
-                                node->setArrayMode(arrayMode.withSpeculation(Array::SaneChain));
+                    default:
+                        break;
+                    }
+                    
+                    if (canDoSaneChain) {
+                        JSGlobalObject* globalObject = m_graph.globalObjectFor(node->origin.semantic);
+                        Structure* arrayPrototypeStructure = globalObject->arrayPrototype()->structure();
+                        Structure* objectPrototypeStructure = globalObject->objectPrototype()->structure();
+                        if (arrayPrototypeStructure->transitionWatchpointSetIsStillValid()
+                            && objectPrototypeStructure->transitionWatchpointSetIsStillValid()
+                            && globalObject->arrayPrototypeChainIsSane()) {
+                            m_graph.registerAndWatchStructureTransition(arrayPrototypeStructure);
+                            m_graph.registerAndWatchStructureTransition(objectPrototypeStructure);
+                            node->setArrayMode(arrayMode.withSpeculation(Array::SaneChain));
                         }
                     }
                 }
index 057c108..b5de023 100644 (file)
@@ -1724,27 +1724,6 @@ int32_t JIT_OPERATION operationInstanceOfCustom(ExecState* exec, EncodedJSValue
 
 }
 
-static bool canAccessArgumentIndexQuickly(JSObject& object, uint32_t index)
-{
-    switch (object.structure()->typeInfo().type()) {
-    case DirectArgumentsType: {
-        DirectArguments* directArguments = jsCast<DirectArguments*>(&object);
-        if (directArguments->isMappedArgumentInDFG(index))
-            return true;
-        break;
-    }
-    case ScopedArgumentsType: {
-        ScopedArguments* scopedArguments = jsCast<ScopedArguments*>(&object);
-        if (scopedArguments->isMappedArgumentInDFG(index))
-            return true;
-        break;
-    }
-    default:
-        break;
-    }
-    return false;
-}
-
 static JSValue getByVal(ExecState* exec, JSValue baseValue, JSValue subscript, ByValInfo* byValInfo, ReturnAddressPtr returnAddress)
 {
     VM& vm = exec->vm();
@@ -1781,7 +1760,16 @@ static JSValue getByVal(ExecState* exec, JSValue baseValue, JSValue subscript, B
             if (object->canGetIndexQuickly(i))
                 return object->getIndexQuickly(i);
 
-            if (!canAccessArgumentIndexQuickly(*object, i)) {
+            bool skipMarkingOutOfBounds = false;
+
+            if (object->indexingType() == ArrayWithContiguous && i < object->butterfly()->publicLength()) {
+                // FIXME: expand this to ArrayStorage, Int32, and maybe Double:
+                // https://bugs.webkit.org/show_bug.cgi?id=182940
+                auto* globalObject = object->globalObject();
+                skipMarkingOutOfBounds = globalObject->isOriginalArrayStructure(object->structure()) && globalObject->arrayPrototypeChainIsSane();
+            }
+
+            if (!skipMarkingOutOfBounds && !CommonSlowPaths::canAccessArgumentIndexQuickly(*object, i)) {
                 // FIXME: This will make us think that in-bounds typed array accesses are actually
                 // out-of-bounds.
                 // https://bugs.webkit.org/show_bug.cgi?id=149886
@@ -1950,7 +1938,7 @@ EncodedJSValue JIT_OPERATION operationHasIndexedPropertyDefault(ExecState* exec,
     if (object->canGetIndexQuickly(index))
         return JSValue::encode(JSValue(JSValue::JSTrue));
 
-    if (!canAccessArgumentIndexQuickly(*object, index)) {
+    if (!CommonSlowPaths::canAccessArgumentIndexQuickly(*object, index)) {
         // FIXME: This will make us think that in-bounds typed array accesses are actually
         // out-of-bounds.
         // https://bugs.webkit.org/show_bug.cgi?id=149886
@@ -1974,7 +1962,7 @@ EncodedJSValue JIT_OPERATION operationHasIndexedPropertyGeneric(ExecState* exec,
     if (object->canGetIndexQuickly(index))
         return JSValue::encode(JSValue(JSValue::JSTrue));
 
-    if (!canAccessArgumentIndexQuickly(*object, index)) {
+    if (!CommonSlowPaths::canAccessArgumentIndexQuickly(*object, index)) {
         // FIXME: This will make us think that in-bounds typed array accesses are actually
         // out-of-bounds.
         // https://bugs.webkit.org/show_bug.cgi?id=149886
index 632bfdb..7b3601c 100644 (file)
@@ -836,7 +836,7 @@ LLINT_SLOW_PATH_DECL(slow_path_del_by_id)
     LLINT_RETURN(jsBoolean(couldDelete));
 }
 
-static ALWAYS_INLINE JSValue getByVal(VM& vm, ExecState* exec, JSValue baseValue, JSValue subscript)
+static ALWAYS_INLINE JSValue getByVal(VM& vm, ExecState* exec, Instruction* pc, JSValue baseValue, JSValue subscript)
 {
     auto scope = DECLARE_THROW_SCOPE(vm);
 
@@ -852,10 +852,32 @@ static ALWAYS_INLINE JSValue getByVal(VM& vm, ExecState* exec, JSValue baseValue
     
     if (subscript.isUInt32()) {
         uint32_t i = subscript.asUInt32();
-        if (isJSString(baseValue) && asString(baseValue)->canGetIndex(i)) {
-            scope.release();
-            return asString(baseValue)->getIndex(exec, i);
+        ArrayProfile* arrayProfile = pc[4].u.arrayProfile;
+
+        if (isJSString(baseValue)) {
+            if (asString(baseValue)->canGetIndex(i)) {
+                scope.release();
+                return asString(baseValue)->getIndex(exec, i);
+            }
+            arrayProfile->setOutOfBounds();
+        } else if (baseValue.isObject()) {
+            JSObject* object = asObject(baseValue);
+            if (object->canGetIndexQuickly(i))
+                return object->getIndexQuickly(i);
+
+            bool skipMarkingOutOfBounds = false;
+
+            if (object->indexingType() == ArrayWithContiguous && i < object->butterfly()->publicLength()) {
+                // FIXME: expand this to ArrayStorage, Int32, and maybe Double:
+                // https://bugs.webkit.org/show_bug.cgi?id=182940
+                auto* globalObject = object->globalObject();
+                skipMarkingOutOfBounds = globalObject->isOriginalArrayStructure(object->structure()) && globalObject->arrayPrototypeChainIsSane();
+            }
+
+            if (!skipMarkingOutOfBounds && !CommonSlowPaths::canAccessArgumentIndexQuickly(*object, i))
+                arrayProfile->setOutOfBounds();
         }
+
         scope.release();
         return baseValue.get(exec, i);
     }
@@ -871,7 +893,7 @@ static ALWAYS_INLINE JSValue getByVal(VM& vm, ExecState* exec, JSValue baseValue
 LLINT_SLOW_PATH_DECL(slow_path_get_by_val)
 {
     LLINT_BEGIN();
-    LLINT_RETURN_PROFILED(op_get_by_val, getByVal(vm, exec, LLINT_OP_C(2).jsValue(), LLINT_OP_C(3).jsValue()));
+    LLINT_RETURN_PROFILED(op_get_by_val, getByVal(vm, exec, pc, LLINT_OP_C(2).jsValue(), LLINT_OP_C(3).jsValue()));
 }
 
 LLINT_SLOW_PATH_DECL(slow_path_put_by_val)
index 0e17ab1..223590a 100644 (file)
@@ -1583,7 +1583,7 @@ _llint_op_get_by_val:
     bineq t2, ContiguousShape, .opGetByValNotContiguous
 
 .opGetByValIsContiguous:
-    biaeq t1, -sizeof IndexingHeader + IndexingHeader::u.lengths.publicLength[t3], .opGetByValOutOfBounds
+    biaeq t1, -sizeof IndexingHeader + IndexingHeader::u.lengths.publicLength[t3], .opGetByValSlow
     andi JSObject::m_butterflyIndexingMask[t0], t1
     loadi TagOffset[t3, t1, 8], t2
     loadi PayloadOffset[t3, t1, 8], t1
@@ -1591,7 +1591,7 @@ _llint_op_get_by_val:
 
 .opGetByValNotContiguous:
     bineq t2, DoubleShape, .opGetByValNotDouble
-    biaeq t1, -sizeof IndexingHeader + IndexingHeader::u.lengths.publicLength[t3], .opGetByValOutOfBounds
+    biaeq t1, -sizeof IndexingHeader + IndexingHeader::u.lengths.publicLength[t3], .opGetByValSlow
     andi JSObject::m_butterflyIndexingMask[t0], t1
     loadd [t3, t1, 8], ft0
     bdnequn ft0, ft0, .opGetByValSlow
@@ -1603,23 +1603,20 @@ _llint_op_get_by_val:
 .opGetByValNotDouble:
     subi ArrayStorageShape, t2
     bia t2, SlowPutArrayStorageShape - ArrayStorageShape, .opGetByValSlow
-    biaeq t1, -sizeof IndexingHeader + IndexingHeader::u.lengths.vectorLength[t3], .opGetByValOutOfBounds
+    biaeq t1, -sizeof IndexingHeader + IndexingHeader::u.lengths.vectorLength[t3], .opGetByValSlow
     andi JSObject::m_butterflyIndexingMask[t0], t1
     loadi ArrayStorage::m_vector + TagOffset[t3, t1, 8], t2
     loadi ArrayStorage::m_vector + PayloadOffset[t3, t1, 8], t1
 
 .opGetByValDone:
     loadi 4[PC], t0
-    bieq t2, EmptyValueTag, .opGetByValOutOfBounds
+    bieq t2, EmptyValueTag, .opGetByValSlow
 .opGetByValNotEmpty:
     storei t2, TagOffset[cfr, t0, 8]
     storei t1, PayloadOffset[cfr, t0, 8]
     valueProfile(t2, t1, 20, t0)
     dispatch(constexpr op_get_by_val_length)
 
-.opGetByValOutOfBounds:
-    loadpFromInstruction(4, t0)
-    storeb 1, ArrayProfile::m_outOfBounds[t0]
 .opGetByValSlow:
     callOpcodeSlowPath(_llint_slow_path_get_by_val)
     dispatch(constexpr op_get_by_val_length)
index 8648f50..c7f4701 100644 (file)
@@ -1514,20 +1514,20 @@ _llint_op_get_by_val:
     bineq t2, ContiguousShape, .opGetByValNotContiguous
 
 .opGetByValIsContiguous:
-    biaeq t1, -sizeof IndexingHeader + IndexingHeader::u.lengths.publicLength[t3], .opGetByValOutOfBounds
+    biaeq t1, -sizeof IndexingHeader + IndexingHeader::u.lengths.publicLength[t3], .opGetByValSlow
     andi JSObject::m_butterflyIndexingMask[t0], t1
     loadisFromInstruction(1, t0)
     loadq [t3, t1, 8], t2
-    btqz t2, .opGetByValOutOfBounds
+    btqz t2, .opGetByValSlow
     jmp .opGetByValDone
 
 .opGetByValNotContiguous:
     bineq t2, DoubleShape, .opGetByValNotDouble
-    biaeq t1, -sizeof IndexingHeader + IndexingHeader::u.lengths.publicLength[t3], .opGetByValOutOfBounds
+    biaeq t1, -sizeof IndexingHeader + IndexingHeader::u.lengths.publicLength[t3], .opGetByValSlow
     andi JSObject::m_butterflyIndexingMask[t0], t1
     loadisFromInstruction(1 ,t0)
     loadd [t3, t1, 8], ft0
-    bdnequn ft0, ft0, .opGetByValOutOfBounds
+    bdnequn ft0, ft0, .opGetByValSlow
     fd2q ft0, t2
     subq tagTypeNumber, t2
     jmp .opGetByValDone
@@ -1535,22 +1535,17 @@ _llint_op_get_by_val:
 .opGetByValNotDouble:
     subi ArrayStorageShape, t2
     bia t2, SlowPutArrayStorageShape - ArrayStorageShape, .opGetByValNotIndexedStorage
-    biaeq t1, -sizeof IndexingHeader + IndexingHeader::u.lengths.vectorLength[t3], .opGetByValOutOfBounds
+    biaeq t1, -sizeof IndexingHeader + IndexingHeader::u.lengths.vectorLength[t3], .opGetByValSlow
     andi JSObject::m_butterflyIndexingMask[t0], t1
     loadisFromInstruction(1, t0)
     loadq ArrayStorage::m_vector[t3, t1, 8], t2
-    btqz t2, .opGetByValOutOfBounds
+    btqz t2, .opGetByValSlow
 
 .opGetByValDone:
     storeq t2, [cfr, t0, 8]
     valueProfile(t2, 5, t0)
     dispatch(constexpr op_get_by_val_length)
 
-.opGetByValOutOfBounds:
-    loadpFromInstruction(4, t0)
-    storeb 1, ArrayProfile::m_outOfBounds[t0]
-    jmp .opGetByValSlow
-
 .opGetByValNotIndexedStorage:
     # First lets check if we even have a typed array. This lets us do some boilerplate up front.
     loadb JSCell::m_type[t0], t2
index 8f7c444..2549f6c 100644 (file)
 
 #include "CodeBlock.h"
 #include "CodeSpecializationKind.h"
+#include "DirectArguments.h"
 #include "ExceptionHelpers.h"
 #include "FunctionCodeBlock.h"
+#include "ScopedArguments.h"
 #include "SlowPathReturnType.h"
 #include "StackAlignment.h"
 #include "VMInlines.h"
@@ -202,6 +204,27 @@ inline void tryCacheGetFromScopeGlobal(
     }
 }
 
+inline bool canAccessArgumentIndexQuickly(JSObject& object, uint32_t index)
+{
+    switch (object.structure()->typeInfo().type()) {
+    case DirectArgumentsType: {
+        DirectArguments* directArguments = jsCast<DirectArguments*>(&object);
+        if (directArguments->isMappedArgumentInDFG(index))
+            return true;
+        break;
+    }
+    case ScopedArgumentsType: {
+        ScopedArguments* scopedArguments = jsCast<ScopedArguments*>(&object);
+        if (scopedArguments->isMappedArgumentInDFG(index))
+            return true;
+        break;
+    }
+    default:
+        break;
+    }
+    return false;
+}
+
 } // namespace CommonSlowPaths
 
 class ExecState;