Attachment 341694 Details for Bug 186162 – Patch

[patch] Patch

bug-186162-20180531145852.patch (text/plain), 18.41 KB, created by Keith Miller on 2018-05-31 14:58:53 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Keith Miller

Created: 2018-05-31 14:58:53 PDT

Size: 18.41 KB

patch

obsolete

>Subversion Revision: 232070
>diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
>index 20484aaec7c9c31623b92dc59521e108fde5bc55..4ce58fc35c9cd48cf0090b546b1970c2e3bcea45 100644
>--- a/Source/JavaScriptCore/ChangeLog
>+++ b/Source/JavaScriptCore/ChangeLog
>@@ -1,3 +1,44 @@
>+2018-05-31  Keith Miller  <keith_miller@apple.com>
>+
>+        DFGArrayModes needs to know more about CoW arrays
>+        https://bugs.webkit.org/show_bug.cgi?id=186162
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        This patch fixes two issues in DFGArrayMode.
>+
>+        1) fromObserved was missing switch cases for when the only observed ArrayModes are CopyOnWrite.
>+        2) DFGArrayModes needs to track if the ArrayClass is an OriginalCopyOnWriteArray in order
>+        to vend an accurate original structure.
>+
>+        Additionally, this patch fixes some places in Bytecode parsing where we told the array mode
>+        we were doing a read but actually doing a write. Also, DFGArrayMode will now print the
>+        action it is expecting when being dumped.
>+
>+        * bytecode/ArrayProfile.h:
>+        (JSC::hasSeenWritableArray):
>+        * dfg/DFGArrayMode.cpp:
>+        (JSC::DFG::ArrayMode::fromObserved):
>+        (JSC::DFG::ArrayMode::refine const):
>+        (JSC::DFG::ArrayMode::originalArrayStructure const):
>+        (JSC::DFG::arrayActionToString):
>+        (JSC::DFG::ArrayMode::dump const):
>+        (WTF::printInternal):
>+        * dfg/DFGArrayMode.h:
>+        (JSC::DFG::ArrayMode::withProfile const):
>+        (JSC::DFG::ArrayMode::isJSArray const):
>+        (JSC::DFG::ArrayMode::isJSArrayWithOriginalStructure const):
>+        (JSC::DFG::ArrayMode::arrayModesWithIndexingShape const):
>+        * dfg/DFGByteCodeParser.cpp:
>+        (JSC::DFG::ByteCodeParser::handleIntrinsicCall):
>+        (JSC::DFG::ByteCodeParser::parseBlock):
>+        * dfg/DFGFixupPhase.cpp:
>+        (JSC::DFG::FixupPhase::fixupNode):
>+        * dfg/DFGSpeculativeJIT.cpp:
>+        (JSC::DFG::SpeculativeJIT::jumpSlowForUnwantedArrayMode):
>+        * ftl/FTLLowerDFGToB3.cpp:
>+        (JSC::FTL::DFG::LowerDFGToB3::isArrayTypeForArrayify):
>+
> 2018-05-22  Keith Miller  <keith_miller@apple.com>
> 
>         We should have a CoW storage for NewArrayBuffer arrays.
>diff --git a/Source/JavaScriptCore/bytecode/ArrayProfile.h b/Source/JavaScriptCore/bytecode/ArrayProfile.h
>index 408960fde621953e8388837abc3dc3a784fe861e..220b67e7999e54af3c508a94624b2c5e8294e5e2 100644
>--- a/Source/JavaScriptCore/bytecode/ArrayProfile.h
>+++ b/Source/JavaScriptCore/bytecode/ArrayProfile.h
>@@ -98,14 +98,17 @@ inline constexpr ArrayModes asArrayModes(IndexingType indexingMode)
>     | CopyOnWriteArrayWithDoubleArrayMode               \
>     | CopyOnWriteArrayWithContiguousArrayMode)
> 
>-#define ALL_ARRAY_ARRAY_MODES                           \
>+#define ALL_WRITABLE_ARRAY_ARRAY_MODES                  \
>     (asArrayModes(ArrayClass)                           \
>     | asArrayModes(ArrayWithUndecided)                  \
>     | asArrayModes(ArrayWithInt32)                      \
>     | asArrayModes(ArrayWithDouble)                     \
>     | asArrayModes(ArrayWithContiguous)                 \
>     | asArrayModes(ArrayWithArrayStorage)               \
>-    | asArrayModes(ArrayWithSlowPutArrayStorage)        \
>+    | asArrayModes(ArrayWithSlowPutArrayStorage))
>+
>+#define ALL_ARRAY_ARRAY_MODES                           \
>+    (ALL_WRITABLE_ARRAY_ARRAY_MODES                     \
>     | ALL_COPY_ON_WRITE_ARRAY_MODES)
> 
> #define ALL_ARRAY_MODES (ALL_NON_ARRAY_ARRAY_MODES | ALL_ARRAY_ARRAY_MODES)
>@@ -205,6 +208,11 @@ inline bool hasSeenNonArray(ArrayModes arrayModes)
>     return arrayModes & ALL_NON_ARRAY_ARRAY_MODES;
> }
> 
>+inline bool hasSeenWritableArray(ArrayModes arrayModes)
>+{
>+    return arrayModes & ALL_WRITABLE_ARRAY_ARRAY_MODES;
>+}
>+
> inline bool hasSeenCopyOnWriteArray(ArrayModes arrayModes)
> {
>     return arrayModes & ALL_COPY_ON_WRITE_ARRAY_MODES;
>diff --git a/Source/JavaScriptCore/dfg/DFGArrayMode.cpp b/Source/JavaScriptCore/dfg/DFGArrayMode.cpp
>index 0e88d2a36fb47d8306222bcc90441b6b305e157b..034abe688054e747b29ea1dc94797ce11f64fc72 100644
>--- a/Source/JavaScriptCore/dfg/DFGArrayMode.cpp
>+++ b/Source/JavaScriptCore/dfg/DFGArrayMode.cpp
>@@ -86,6 +86,7 @@ ArrayMode ArrayMode::fromObserved(const ConcurrentJSLocker& locker, ArrayProfile
> 
>     case asArrayModes(NonArrayWithInt32):
>     case asArrayModes(ArrayWithInt32):
>+    case asArrayModes(CopyOnWriteArrayWithInt32):
>     case asArrayModes(NonArrayWithInt32) | asArrayModes(ArrayWithInt32):
>     case asArrayModes(NonArrayWithInt32) | asArrayModes(CopyOnWriteArrayWithInt32):
>     case asArrayModes(ArrayWithInt32) | asArrayModes(CopyOnWriteArrayWithInt32):
>@@ -94,6 +95,7 @@ ArrayMode ArrayMode::fromObserved(const ConcurrentJSLocker& locker, ArrayProfile
> 
>     case asArrayModes(NonArrayWithDouble):
>     case asArrayModes(ArrayWithDouble):
>+    case asArrayModes(CopyOnWriteArrayWithDouble):
>     case asArrayModes(NonArrayWithDouble) | asArrayModes(ArrayWithDouble):
>     case asArrayModes(NonArrayWithDouble) | asArrayModes(CopyOnWriteArrayWithDouble):
>     case asArrayModes(ArrayWithDouble) | asArrayModes(CopyOnWriteArrayWithDouble):
>@@ -102,6 +104,7 @@ ArrayMode ArrayMode::fromObserved(const ConcurrentJSLocker& locker, ArrayProfile
> 
>     case asArrayModes(NonArrayWithContiguous):
>     case asArrayModes(ArrayWithContiguous):
>+    case asArrayModes(CopyOnWriteArrayWithContiguous):
>     case asArrayModes(NonArrayWithContiguous) | asArrayModes(ArrayWithContiguous):
>     case asArrayModes(NonArrayWithContiguous) | asArrayModes(CopyOnWriteArrayWithContiguous):
>     case asArrayModes(ArrayWithContiguous) | asArrayModes(CopyOnWriteArrayWithContiguous):
>@@ -242,7 +245,7 @@ ArrayMode ArrayMode::refine(
>         Structure* arrayPrototypeStructure = globalObject->arrayPrototype()->structure();
>         Structure* objectPrototypeStructure = globalObject->objectPrototype()->structure();
>         if ((node->op() == GetByVal || canBecomeGetArrayLength(graph, node))
>-            && arrayClass() == Array::OriginalArray
>+            && isJSArrayWithOriginalStructure()
>             && !graph.hasExitSite(node->origin.semantic, OutOfBounds)
>             && arrayPrototypeStructure->transitionWatchpointSetIsStillValid()
>             && objectPrototypeStructure->transitionWatchpointSetIsStillValid()
>@@ -361,6 +364,23 @@ Structure* ArrayMode::originalArrayStructure(Graph& graph, const CodeOrigin& cod
>     JSGlobalObject* globalObject = graph.globalObjectFor(codeOrigin);
>     
>     switch (arrayClass()) {
>+    case Array::OriginalCopyOnWriteArray: {
>+        if (conversion() == Array::AsIs) {
>+            switch (type()) {
>+            case Array::Int32:
>+                return globalObject->originalArrayStructureForIndexingType(CopyOnWriteArrayWithInt32);
>+            case Array::Double:
>+                return globalObject->originalArrayStructureForIndexingType(CopyOnWriteArrayWithDouble);
>+            case Array::Contiguous:
>+                return globalObject->originalArrayStructureForIndexingType(CopyOnWriteArrayWithContiguous);
>+            default:
>+                CRASH();
>+                return nullptr;
>+            }
>+        }
>+        FALLTHROUGH;
>+    }
>+
>     case Array::OriginalArray: {
>         switch (type()) {
>         case Array::Int32:
>@@ -558,6 +578,18 @@ bool ArrayMode::alreadyChecked(Graph& graph, Node* node, const AbstractValue& va
>     return false;
> }
> 
>+const char* arrayActionToString(Array::Action action)
>+{
>+    switch (action) {
>+    case Array::Read:
>+        return "Read";
>+    case Array::Write:
>+        return "Write";
>+    default:
>+        return "Unknown!";
>+    }
>+}
>+
> const char* arrayTypeToString(Array::Type type)
> {
>     switch (type) {
>@@ -785,13 +817,18 @@ bool ArrayMode::permitsBoundsCheckLowering() const
> 
> void ArrayMode::dump(PrintStream& out) const
> {
>-    out.print(type(), "+", arrayClass(), "+", speculation(), "+", conversion());
>+    out.print(type(), "+", arrayClass(), "+", speculation(), "+", conversion(), "+", action());
> }
> 
> } } // namespace JSC::DFG
> 
> namespace WTF {
> 
>+void printInternal(PrintStream& out, JSC::DFG::Array::Action action)
>+{
>+    out.print(JSC::DFG::arrayActionToString(action));
>+}
>+
> void printInternal(PrintStream& out, JSC::DFG::Array::Type type)
> {
>     out.print(JSC::DFG::arrayTypeToString(type));
>diff --git a/Source/JavaScriptCore/dfg/DFGArrayMode.h b/Source/JavaScriptCore/dfg/DFGArrayMode.h
>index 1a15fec20c5e51ee0bde47f121f92e950fd51169..6878f1e462a1124cd1f0f82342f85574267a7f71 100644
>--- a/Source/JavaScriptCore/dfg/DFGArrayMode.h
>+++ b/Source/JavaScriptCore/dfg/DFGArrayMode.h
>@@ -84,6 +84,7 @@ enum Class : uint8_t {
>     OriginalNonArray, // Definitely some object that is not a JSArray, but that object has the original structure.
>     Array, // Definitely a JSArray, and may or may not have custom properties or have undergone some other bizarre transitions.
>     OriginalArray, // Definitely a JSArray, and still has one of the primordial JSArray structures for the global object that this code block (possibly inlined code block) belongs to.
>+    OriginalCopyOnWriteArray, // Definitely a copy on write JSArray, and still has one of the primordial JSArray copy on write structures for the global object that this code block (possibly inlined code block) belongs to.
>     PossiblyArray // Some object that may or may not be a JSArray.
> };
> 
>@@ -205,9 +206,15 @@ public:
>         Array::Class myArrayClass;
> 
>         if (isJSArray()) {
>-            if (profile->usesOriginalArrayStructures(locker) && benefitsFromOriginalArray())
>-                myArrayClass = Array::OriginalArray;
>-            else
>+            if (profile->usesOriginalArrayStructures(locker) && benefitsFromOriginalArray()) {
>+                ArrayModes arrayModes = profile->observedArrayModes(locker);
>+                if (hasSeenCopyOnWriteArray(arrayModes) && !hasSeenWritableArray(arrayModes))
>+                    myArrayClass = Array::OriginalCopyOnWriteArray;
>+                else if (!hasSeenCopyOnWriteArray(arrayModes) && hasSeenWritableArray(arrayModes))
>+                    myArrayClass = Array::OriginalArray;
>+                else
>+                    myArrayClass = Array::Array;
>+            } else
>                 myArrayClass = Array::Array;
>         } else
>             myArrayClass = arrayClass();
>@@ -256,6 +263,7 @@ public:
>         switch (arrayClass()) {
>         case Array::Array:
>         case Array::OriginalArray:
>+        case Array::OriginalCopyOnWriteArray:
>             return true;
>         default:
>             return false;
>@@ -264,7 +272,7 @@ public:
>     
>     bool isJSArrayWithOriginalStructure() const
>     {
>-        return arrayClass() == Array::OriginalArray;
>+        return arrayClass() == Array::OriginalArray || arrayClass() == Array::OriginalCopyOnWriteArray;
>     }
>     
>     bool isSaneChain() const
>@@ -487,10 +495,14 @@ private:
>         case Array::NonArray:
>         case Array::OriginalNonArray:
>             return asArrayModes(shape);
>+        case Array::OriginalCopyOnWriteArray:
>+            ASSERT(hasInt32(shape) || hasDouble(shape) || hasContiguous(shape));
>+            return asArrayModes(shape | IsArray) | asArrayModes(shape | IsArray | CopyOnWrite);
>         case Array::Array:
>-        case Array::OriginalArray:
>             if (hasInt32(shape) || hasDouble(shape) || hasContiguous(shape))
>                 return asArrayModes(shape | IsArray) | asArrayModes(shape | IsArray | CopyOnWrite);
>+            FALLTHROUGH;
>+        case Array::OriginalArray:
>             return asArrayModes(shape | IsArray);
>         case Array::PossiblyArray:
>             if (hasInt32(shape) || hasDouble(shape) || hasContiguous(shape))
>@@ -544,6 +556,7 @@ static inline bool neverNeedsStorage(const ArrayMode&)
> namespace WTF {
> 
> class PrintStream;
>+void printInternal(PrintStream&, JSC::DFG::Array::Action);
> void printInternal(PrintStream&, JSC::DFG::Array::Type);
> void printInternal(PrintStream&, JSC::DFG::Array::Class);
> void printInternal(PrintStream&, JSC::DFG::Array::Speculation);
>diff --git a/Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp b/Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp
>index 078444138751b6a99621076c986035ac7ba81da4..73e2bb8830a6183fcc1b6f5dabed098e540ecf1c 100644
>--- a/Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp
>+++ b/Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp
>@@ -895,12 +895,7 @@ private:
>         bool makeSafe = profile->outOfBounds(locker);
>         return ArrayMode::fromObserved(locker, profile, action, makeSafe);
>     }
>-    
>-    ArrayMode getArrayMode(ArrayProfile* profile)
>-    {
>-        return getArrayMode(profile, Array::Read);
>-    }
>-    
>+
>     Node* makeSafe(Node* node)
>     {
>         if (m_inlineStackTop->m_exitProfile.hasExitSite(m_currentIndex, Overflow))
>@@ -2211,7 +2206,7 @@ bool ByteCodeParser::handleIntrinsicCall(Node* callee, int resultOperand, Intrin
>         if (static_cast<unsigned>(argumentCountIncludingThis) >= MIN_SPARSE_ARRAY_INDEX)
>             return false;
>         
>-        ArrayMode arrayMode = getArrayMode(m_currentInstruction[OPCODE_LENGTH(op_call) - 2].u.arrayProfile);
>+        ArrayMode arrayMode = getArrayMode(m_currentInstruction[OPCODE_LENGTH(op_call) - 2].u.arrayProfile, Array::Write);
>         if (!arrayMode.isJSArray())
>             return false;
>         switch (arrayMode.type()) {
>@@ -2249,11 +2244,11 @@ bool ByteCodeParser::handleIntrinsicCall(Node* callee, int resultOperand, Intrin
>             || m_inlineStackTop->m_exitProfile.hasExitSite(m_currentIndex, BadCache))
>             return false;
> 
>-        ArrayMode arrayMode = getArrayMode(m_currentInstruction[OPCODE_LENGTH(op_call) - 2].u.arrayProfile);
>+        ArrayMode arrayMode = getArrayMode(m_currentInstruction[OPCODE_LENGTH(op_call) - 2].u.arrayProfile, Array::Read);
>         if (!arrayMode.isJSArray())
>             return false;
> 
>-        if (arrayMode.arrayClass() != Array::OriginalArray)
>+        if (!arrayMode.isJSArrayWithOriginalStructure())
>             return false;
> 
>         switch (arrayMode.type()) {
>@@ -2335,11 +2330,11 @@ bool ByteCodeParser::handleIntrinsicCall(Node* callee, int resultOperand, Intrin
>             || m_inlineStackTop->m_exitProfile.hasExitSite(m_currentIndex, BadType))
>             return false;
> 
>-        ArrayMode arrayMode = getArrayMode(m_currentInstruction[OPCODE_LENGTH(op_call) - 2].u.arrayProfile);
>+        ArrayMode arrayMode = getArrayMode(m_currentInstruction[OPCODE_LENGTH(op_call) - 2].u.arrayProfile, Array::Read);
>         if (!arrayMode.isJSArray())
>             return false;
> 
>-        if (arrayMode.arrayClass() != Array::OriginalArray)
>+        if (!arrayMode.isJSArrayWithOriginalStructure())
>             return false;
> 
>         // We do not want to convert arrays into one type just to perform indexOf.
>@@ -2395,7 +2390,7 @@ bool ByteCodeParser::handleIntrinsicCall(Node* callee, int resultOperand, Intrin
>         if (argumentCountIncludingThis != 1)
>             return false;
>         
>-        ArrayMode arrayMode = getArrayMode(m_currentInstruction[OPCODE_LENGTH(op_call) - 2].u.arrayProfile);
>+        ArrayMode arrayMode = getArrayMode(m_currentInstruction[OPCODE_LENGTH(op_call) - 2].u.arrayProfile, Array::Write);
>         if (!arrayMode.isJSArray())
>             return false;
>         switch (arrayMode.type()) {
>@@ -6397,7 +6392,7 @@ void ByteCodeParser::parseBlock(unsigned limit)
>         }
> 
>         case op_in_by_val: {
>-            ArrayMode arrayMode = getArrayMode(currentInstruction[OPCODE_LENGTH(op_in_by_val) - 1].u.arrayProfile);
>+            ArrayMode arrayMode = getArrayMode(currentInstruction[OPCODE_LENGTH(op_in_by_val) - 1].u.arrayProfile, Array::Read);
>             set(VirtualRegister(currentInstruction[1].u.operand),
>                 addToGraph(InByVal, OpInfo(arrayMode.asWord()), get(VirtualRegister(currentInstruction[2].u.operand)), get(VirtualRegister(currentInstruction[3].u.operand))));
>             NEXT_OPCODE(op_in_by_val);
>diff --git a/Source/JavaScriptCore/dfg/DFGFixupPhase.cpp b/Source/JavaScriptCore/dfg/DFGFixupPhase.cpp
>index ae76a9475354b7115cc5394a75bb1fd55d6b8ca1..313d7409e02aa2cb3c1b97f6e7ebfff43618a28d 100644
>--- a/Source/JavaScriptCore/dfg/DFGFixupPhase.cpp
>+++ b/Source/JavaScriptCore/dfg/DFGFixupPhase.cpp
>@@ -632,8 +632,7 @@ private:
>             switch (arrayMode.type()) {
>             case Array::Contiguous:
>             case Array::Double:
>-                if (arrayMode.arrayClass() == Array::OriginalArray
>-                    && arrayMode.speculation() == Array::InBounds) {
>+                if (arrayMode.isJSArrayWithOriginalStructure() && arrayMode.speculation() == Array::InBounds) {
>                     // Check if SaneChain will work on a per-type basis. Note that:
>                     //
>                     // 1) We don't want double arrays to sometimes return undefined, since
>diff --git a/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp b/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp
>index d18b102a40b62364a40d14655b407771d24254a0..0f3fb72cbc356600abc36d3ac4ab7fe14624228c 100644
>--- a/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp
>+++ b/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp
>@@ -745,6 +745,7 @@ JITCompiler::JumpList SpeculativeJIT::jumpSlowForUnwantedArrayMode(GPRReg tempGP
>         IndexingType shape = arrayMode.shapeMask();
>         switch (arrayMode.arrayClass()) {
>         case Array::OriginalArray:
>+        case Array::OriginalCopyOnWriteArray:
>             RELEASE_ASSERT_NOT_REACHED();
>             return result;
> 
>@@ -776,6 +777,7 @@ JITCompiler::JumpList SpeculativeJIT::jumpSlowForUnwantedArrayMode(GPRReg tempGP
> 
>         switch (arrayMode.arrayClass()) {
>         case Array::OriginalArray:
>+        case Array::OriginalCopyOnWriteArray:
>             RELEASE_ASSERT_NOT_REACHED();
>             return result;
> 
>diff --git a/Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp b/Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp
>index d98a8d80b6c591fc8b3c8a1922b35eb9d73b1030..adf9c99677793071db3eef91cae34d485dd56257 100644
>--- a/Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp
>+++ b/Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp
>@@ -15034,6 +15034,7 @@ private:
> 
>             switch (arrayMode.arrayClass()) {
>             case Array::OriginalArray:
>+            case Array::OriginalCopyOnWriteArray:
>                 DFG_CRASH(m_graph, m_node, "Unexpected original array");
>                 return nullptr;
> 
>@@ -15075,6 +15076,7 @@ private:
>             LBasicBlock lastNext = m_out.appendTo(checkCase, trueCase);
>             switch (arrayMode.arrayClass()) {
>             case Array::OriginalArray:
>+            case Array::OriginalCopyOnWriteArray:
>                 DFG_CRASH(m_graph, m_node, "Unexpected original array");
>                 return nullptr;
>

Flags:

fpizlo: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 186162: 341694