Attachment 345751 Details for Bug 187949 – Patch

[patch] Patch

bug-187949-20180725200839.patch (text/plain), 10.85 KB, created by Yusuke Suzuki on 2018-07-25 04:08:40 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Yusuke Suzuki

Created: 2018-07-25 04:08:40 PDT

Size: 10.85 KB

patch

obsolete

>Subversion Revision: 234192
>diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
>index 33ee8b51dbc1873e4db953990f2b4b44b1ea19fe..681dff8073e2b08ffc40bbd4646f3715dfd51fb8 100644
>--- a/Source/JavaScriptCore/ChangeLog
>+++ b/Source/JavaScriptCore/ChangeLog
>@@ -1,3 +1,25 @@
>+2018-07-25  Yusuke Suzuki  <utatane.tea@gmail.com>
>+
>+        [JSC] Record CoW status in ArrayProfile correctly
>+        https://bugs.webkit.org/show_bug.cgi?id=187949
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        In this patch, we simplify asArrayModes: just shifting the value with IndexingMode.
>+        This is important since our OSR exit compiler records m_observedArrayModes by calculating
>+        ArrayModes with shifting. Since ArrayModes for CoW arrays are incorrectly calculated,
>+        our OSR exit compiler records incorrect results in ArrayProfile. And it leads to
>+        Array::Generic DFG nodes.
>+
>+        * bytecode/ArrayProfile.h:
>+        (JSC::asArrayModes):
>+        (JSC::ArrayProfile::ArrayProfile):
>+        * dfg/DFGOSRExit.cpp:
>+        (JSC::DFG::OSRExit::compileExit):
>+        * ftl/FTLOSRExitCompiler.cpp:
>+        (JSC::FTL::compileStub):
>+        * runtime/IndexingType.h:
>+
> 2018-07-24  Fujii Hironori  <Hironori.Fujii@sony.com>
> 
>         runJITThreadLimitTests is failing
>diff --git a/Source/JavaScriptCore/bytecode/ArrayProfile.h b/Source/JavaScriptCore/bytecode/ArrayProfile.h
>index 220b67e7999e54af3c508a94624b2c5e8294e5e2..863cd7e57217ad91fd15849dad6e8da46aa9353a 100644
>--- a/Source/JavaScriptCore/bytecode/ArrayProfile.h
>+++ b/Source/JavaScriptCore/bytecode/ArrayProfile.h
>@@ -39,36 +39,27 @@ class LLIntOffsetsExtractor;
> // There are 9 typed array types taking the bits 16 to 25.
> typedef unsigned ArrayModes;
> 
>-const ArrayModes CopyOnWriteArrayWithInt32ArrayMode = 1 << 16;
>-const ArrayModes CopyOnWriteArrayWithDoubleArrayMode = 1 << 17;
>-const ArrayModes CopyOnWriteArrayWithContiguousArrayMode = 1 << 18;
>-
>-const ArrayModes Int8ArrayMode = 1 << 19;
>-const ArrayModes Int16ArrayMode = 1 << 20;
>-const ArrayModes Int32ArrayMode = 1 << 21;
>-const ArrayModes Uint8ArrayMode = 1 << 22;
>-const ArrayModes Uint8ClampedArrayMode = 1 << 23;
>-const ArrayModes Uint16ArrayMode = 1 << 24;
>-const ArrayModes Uint32ArrayMode = 1 << 25;
>-const ArrayModes Float32ArrayMode = 1 << 26;
>-const ArrayModes Float64ArrayMode = 1 << 27;
>+// The possible IndexingTypes are limited within (0 - 16, 21, 23, 25).
>+// This is because CoW types only appear for JSArrays.
>+static_assert(CopyOnWriteArrayWithInt32 == 21, "");
>+static_assert(CopyOnWriteArrayWithDouble == 23, "");
>+static_assert(CopyOnWriteArrayWithContiguous == 25, "");
>+const ArrayModes CopyOnWriteArrayWithInt32ArrayMode = 1 << CopyOnWriteArrayWithInt32;
>+const ArrayModes CopyOnWriteArrayWithDoubleArrayMode = 1 << CopyOnWriteArrayWithDouble;
>+const ArrayModes CopyOnWriteArrayWithContiguousArrayMode = 1 << CopyOnWriteArrayWithContiguous;
>+
>+const ArrayModes Int8ArrayMode = 1 << 16;
>+const ArrayModes Int16ArrayMode = 1 << 17;
>+const ArrayModes Int32ArrayMode = 1 << 18;
>+const ArrayModes Uint8ArrayMode = 1 << 19;
>+const ArrayModes Uint8ClampedArrayMode = 1 << 20; // 21 - 25 are used for CoW arrays.
>+const ArrayModes Uint16ArrayMode = 1 << 26;
>+const ArrayModes Uint32ArrayMode = 1 << 27;
>+const ArrayModes Float32ArrayMode = 1 << 28;
>+const ArrayModes Float64ArrayMode = 1 << 29;
> 
> inline constexpr ArrayModes asArrayModes(IndexingType indexingMode)
> {
>-    if (isCopyOnWrite(indexingMode)) {
>-        switch (indexingMode) {
>-        case CopyOnWriteArrayWithInt32:
>-            return CopyOnWriteArrayWithInt32ArrayMode;
>-        case CopyOnWriteArrayWithDouble:
>-            return CopyOnWriteArrayWithDoubleArrayMode;
>-        case CopyOnWriteArrayWithContiguous:
>-            return CopyOnWriteArrayWithContiguousArrayMode;
>-        default:
>-            UNREACHABLE_FOR_PLATFORM();
>-            return 0;
>-        }
>-    }
>-
>     return static_cast<unsigned>(1) << static_cast<unsigned>(indexingMode);
> }
> 
>@@ -221,26 +212,15 @@ inline bool hasSeenCopyOnWriteArray(ArrayModes arrayModes)
> class ArrayProfile {
> public:
>     ArrayProfile()
>-        : m_bytecodeOffset(std::numeric_limits<unsigned>::max())
>-        , m_lastSeenStructureID(0)
>-        , m_mayStoreToHole(false)
>-        , m_outOfBounds(false)
>-        , m_mayInterceptIndexedAccesses(false)
>-        , m_usesOriginalArrayStructures(true)
>-        , m_didPerformFirstRunPruning(false)
>-        , m_observedArrayModes(0)
>+        : ArrayProfile(std::numeric_limits<unsigned>::max())
>     {
>     }
>     
>-    ArrayProfile(unsigned bytecodeOffset)
>+    explicit ArrayProfile(unsigned bytecodeOffset)
>         : m_bytecodeOffset(bytecodeOffset)
>-        , m_lastSeenStructureID(0)
>-        , m_mayStoreToHole(false)
>-        , m_outOfBounds(false)
>         , m_mayInterceptIndexedAccesses(false)
>         , m_usesOriginalArrayStructures(true)
>         , m_didPerformFirstRunPruning(false)
>-        , m_observedArrayModes(0)
>     {
>     }
>     
>@@ -281,13 +261,13 @@ class ArrayProfile {
>     static Structure* polymorphicStructure() { return static_cast<Structure*>(reinterpret_cast<void*>(1)); }
>     
>     unsigned m_bytecodeOffset;
>-    StructureID m_lastSeenStructureID;
>-    bool m_mayStoreToHole; // This flag may become overloaded to indicate other special cases that were encountered during array access, as it depends on indexing type. Since we currently have basically just one indexing type (two variants of ArrayStorage), this flag for now just means exactly what its name implies.
>-    bool m_outOfBounds;
>+    StructureID m_lastSeenStructureID { 0 };
>+    bool m_mayStoreToHole { false }; // This flag may become overloaded to indicate other special cases that were encountered during array access, as it depends on indexing type. Since we currently have basically just one indexing type (two variants of ArrayStorage), this flag for now just means exactly what its name implies.
>+    bool m_outOfBounds { false };
>     bool m_mayInterceptIndexedAccesses : 1;
>     bool m_usesOriginalArrayStructures : 1;
>     bool m_didPerformFirstRunPruning : 1;
>-    ArrayModes m_observedArrayModes;
>+    ArrayModes m_observedArrayModes { 0 };
> };
> 
> typedef SegmentedVector<ArrayProfile, 4> ArrayProfileVector;
>diff --git a/Source/JavaScriptCore/dfg/DFGOSRExit.cpp b/Source/JavaScriptCore/dfg/DFGOSRExit.cpp
>index 00f5977b473177666e9ba48517add77bb55bb1ed..3883bc7f7e761897e9c5bb299df37fb9942f78c5 100644
>--- a/Source/JavaScriptCore/dfg/DFGOSRExit.cpp
>+++ b/Source/JavaScriptCore/dfg/DFGOSRExit.cpp
>@@ -1199,6 +1199,7 @@ void OSRExit::compileExit(CCallHelpers& jit, VM& vm, const OSRExit& exit, const
> #else
>                 jit.load8(AssemblyHelpers::Address(scratch1, Structure::indexingModeIncludingHistoryOffset()), scratch1);
> #endif
>+                jit.and32(AssemblyHelpers::TrustedImm32(IndexingModeMask), scratch1);
>                 jit.move(AssemblyHelpers::TrustedImm32(1), scratch2);
>                 jit.lshift32(scratch1, scratch2);
>                 jit.or32(scratch2, AssemblyHelpers::AbsoluteAddress(arrayProfile->addressOfArrayModes()));
>diff --git a/Source/JavaScriptCore/ftl/FTLOSRExitCompiler.cpp b/Source/JavaScriptCore/ftl/FTLOSRExitCompiler.cpp
>index 9922a6e48b84c8a73ba23f3b70311d7a4c85e2b0..f066d0ba46623be4a190df3c2d6f2030858f846d 100644
>--- a/Source/JavaScriptCore/ftl/FTLOSRExitCompiler.cpp
>+++ b/Source/JavaScriptCore/ftl/FTLOSRExitCompiler.cpp
>@@ -278,6 +278,7 @@ static void compileStub(
>                 jit.load32(MacroAssembler::Address(GPRInfo::regT0, JSCell::structureIDOffset()), GPRInfo::regT1);
>                 jit.store32(GPRInfo::regT1, arrayProfile->addressOfLastSeenStructureID());
>                 jit.load8(MacroAssembler::Address(GPRInfo::regT0, JSCell::indexingTypeAndMiscOffset()), GPRInfo::regT1);
>+                jit.and32(MacroAssembler::TrustedImm32(IndexingModeMask), GPRInfo::regT1);
>                 jit.move(MacroAssembler::TrustedImm32(1), GPRInfo::regT2);
>                 jit.lshift32(GPRInfo::regT1, GPRInfo::regT2);
>                 jit.or32(GPRInfo::regT2, MacroAssembler::AbsoluteAddress(arrayProfile->addressOfArrayModes()));
>diff --git a/Source/JavaScriptCore/runtime/IndexingType.h b/Source/JavaScriptCore/runtime/IndexingType.h
>index 2ad4ee6e46092c83aaa783ff9d8ec30eb43fab62..b9f899e2fa59984b510afb1c15fe287132087f03 100644
>--- a/Source/JavaScriptCore/runtime/IndexingType.h
>+++ b/Source/JavaScriptCore/runtime/IndexingType.h
>@@ -79,6 +79,7 @@ static const IndexingType IndexingTypeMask                = IndexingShapeMask |
> // Whether or not the butterfly is copy on write. If it is copy on write then the butterfly is actually a JSImmutableButterfly. This should only ever be set if there are no named properties.
> static const IndexingType CopyOnWrite                      = 0x10;
> static const IndexingType IndexingShapeAndWritabilityMask  = CopyOnWrite | IndexingShapeMask;
>+static const IndexingType IndexingModeMask                 = CopyOnWrite | IndexingTypeMask;
> static const IndexingType NumberOfCopyOnWriteIndexingModes = 3; // We only have copy on write for int32, double, and contiguous shapes.
> static const IndexingType NumberOfArrayIndexingModes       = NumberOfIndexingShapes + NumberOfCopyOnWriteIndexingModes;
> 
>diff --git a/JSTests/ChangeLog b/JSTests/ChangeLog
>index a9b8e5edc09c1c425ecec9ecc9c3bd981b4d7d7f..3e7c6e464e3c7af09e588c2f9bb042f2f943d02b 100644
>--- a/JSTests/ChangeLog
>+++ b/JSTests/ChangeLog
>@@ -1,3 +1,16 @@
>+2018-07-25  Yusuke Suzuki  <utatane.tea@gmail.com>
>+
>+        [JSC] Record CoW status in ArrayProfile correctly
>+        https://bugs.webkit.org/show_bug.cgi?id=187949
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * stress/array-profile-should-record-copy-on-write.js: Added.
>+        (shouldBe):
>+        (test1):
>+        (test2):
>+        (test3):
>+
> 2018-07-24  Commit Queue  <commit-queue@webkit.org>
> 
>         Unreviewed, rolling out r234183.
>diff --git a/JSTests/stress/array-profile-should-record-copy-on-write.js b/JSTests/stress/array-profile-should-record-copy-on-write.js
>new file mode 100644
>index 0000000000000000000000000000000000000000..0de63e92304ff4dc2e57a010a9f88e7f0b38ac40
>--- /dev/null
>+++ b/JSTests/stress/array-profile-should-record-copy-on-write.js
>@@ -0,0 +1,39 @@
>+function shouldBe(actual, expected)
>+{
>+    if (actual !== expected)
>+        throw new Error('bad value: ' + actual);
>+}
>+noInline(shouldBe);
>+
>+function test1(array)
>+{
>+    for (var i = 0; i < 5; ++i) {
>+        array[0] = array[0] + 1;
>+    }
>+    return array;
>+}
>+noInline(test1);
>+
>+function test2(array)
>+{
>+    for (var i = 0; i < 5; ++i) {
>+        array[0] = array[0] + 1;
>+    }
>+    return array;
>+}
>+noInline(test2);
>+
>+function test3(array)
>+{
>+    for (var i = 0; i < 5; ++i) {
>+        array[0] = array[0] + 1;
>+    }
>+    return array;
>+}
>+noInline(test3);
>+
>+for (var i = 0; i < 1e5; ++i) {
>+    shouldBe(String(test1([0, 1, 2, 3, 4])), `5,1,2,3,4`);
>+    shouldBe(String(test2([0.1, 1.1, 2.1, 3.1, 4.1])), `5.1,1.1,2.1,3.1,4.1`);
>+    shouldBe(String(test3(['C', 'o', 'c', 'o', 'a'])), `C11111,o,c,o,a`);
>+}

Flags:

saam: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 187949: 345647 | 345648 | 345658 | 345662 | 345664 | 345672 | 345678 | 345751