Attachment 370457 Details for Bug 197809 – Patch

[patch] Patch

bug-197809-20190522155116.patch (text/plain), 9.80 KB, created by Yusuke Suzuki on 2019-05-22 15:51:17 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Yusuke Suzuki

Created: 2019-05-22 15:51:17 PDT

Size: 9.80 KB

patch

obsolete

>Subversion Revision: 245649
>diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
>index 9d141dfe3700364b88fbdae27fbbe8df539ebe73..dd00e975ed0ffa79a4760469a443fbbc1647474c 100644
>--- a/Source/JavaScriptCore/ChangeLog
>+++ b/Source/JavaScriptCore/ChangeLog
>@@ -1,3 +1,34 @@
>+2019-05-22  Yusuke Suzuki  <ysuzuki@apple.com>
>+
>+        [JSC] ArrayAllocationProfile should not access to butterfly in concurrent compiler
>+        https://bugs.webkit.org/show_bug.cgi?id=197809
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        ArrayAllocationProfile assumes that Butterfly can be accessed concurrently. But this is not correct now
>+        since LargeAllocation Butterfly can be realloced. In this patch, we switch profiling array allocations
>+        only in the main thread. This allocation profiling is repeatedly called in the main thread's slow path,
>+        and it is also called when updating the profiles in the main thread.
>+
>+        We also rename updateAllPredictionsAndCountLiveness to updateAllValueProfilePredictionsAndCountLiveness
>+        since it only cares ValueProfiles.
>+
>+        * bytecode/ArrayAllocationProfile.cpp:
>+        (JSC::ArrayAllocationProfile::updateProfile):
>+        * bytecode/ArrayAllocationProfile.h:
>+        (JSC::ArrayAllocationProfile::selectIndexingTypeConcurrently):
>+        (JSC::ArrayAllocationProfile::selectIndexingType):
>+        (JSC::ArrayAllocationProfile::vectorLengthHintConcurrently):
>+        (JSC::ArrayAllocationProfile::vectorLengthHint):
>+        * bytecode/CodeBlock.cpp:
>+        (JSC::CodeBlock::updateAllValueProfilePredictionsAndCountLiveness):
>+        (JSC::CodeBlock::updateAllValueProfilePredictions):
>+        (JSC::CodeBlock::shouldOptimizeNow):
>+        (JSC::CodeBlock::updateAllPredictionsAndCountLiveness): Deleted.
>+        * bytecode/CodeBlock.h:
>+        * dfg/DFGByteCodeParser.cpp:
>+        (JSC::DFG::ByteCodeParser::parseBlock):
>+
> 2019-05-22  Commit Queue  <commit-queue@webkit.org>
> 
>         Unreviewed, rolling out r245634.
>diff --git a/Source/JavaScriptCore/bytecode/ArrayAllocationProfile.cpp b/Source/JavaScriptCore/bytecode/ArrayAllocationProfile.cpp
>index bd73651ab22cdc1015b67fa8b8c366aeea133b2b..50e267875e315f10030968653fb619e218f4e248 100644
>--- a/Source/JavaScriptCore/bytecode/ArrayAllocationProfile.cpp
>+++ b/Source/JavaScriptCore/bytecode/ArrayAllocationProfile.cpp
>@@ -47,7 +47,13 @@ void ArrayAllocationProfile::updateProfile()
>     //   it's possible for that array to no longer be reachable, it cannot actually
>     //   be freed, since we require the GC to wait until all concurrent JITing
>     //   finishes.
>+    //
>+    // But one exception is vector length. We access vector length to get the vector
>+    // length hint. However vector length can be accessible only from the main
>+    // thread because large butterfly can be realloced in the main thread.
>+    // So for now, we update the allocation profile only from the main thread.
>     
>+    ASSERT(!isCompilationThread());
>     JSArray* lastArray = m_lastArray;
>     if (!lastArray)
>         return;
>diff --git a/Source/JavaScriptCore/bytecode/ArrayAllocationProfile.h b/Source/JavaScriptCore/bytecode/ArrayAllocationProfile.h
>index fef936e257888ed22ea20f8b18f9d9c7b13265f3..6611b91a8846838fb5df2f71ed8f6fb72e61989c 100644
>--- a/Source/JavaScriptCore/bytecode/ArrayAllocationProfile.h
>+++ b/Source/JavaScriptCore/bytecode/ArrayAllocationProfile.h
>@@ -39,8 +39,14 @@ class ArrayAllocationProfile {
>         initializeIndexingMode(recommendedIndexingMode);
>     }
> 
>+    IndexingType selectIndexingTypeConcurrently()
>+    {
>+        return m_currentIndexingType;
>+    }
>+
>     IndexingType selectIndexingType()
>     {
>+        ASSERT(!isCompilationThread());
>         JSArray* lastArray = m_lastArray;
>         if (lastArray && UNLIKELY(lastArray->indexingType() != m_currentIndexingType))
>             updateProfile();
>@@ -48,8 +54,14 @@ class ArrayAllocationProfile {
>     }
> 
>     // vector length hint becomes [0, BASE_CONTIGUOUS_VECTOR_LEN_MAX].
>+    unsigned vectorLengthHintConcurrently()
>+    {
>+        return m_largestSeenVectorLength;
>+    }
>+
>     unsigned vectorLengthHint()
>     {
>+        ASSERT(!isCompilationThread());
>         JSArray* lastArray = m_lastArray;
>         if (lastArray && (m_largestSeenVectorLength != BASE_CONTIGUOUS_VECTOR_LEN_MAX) && UNLIKELY(lastArray->getVectorLength() > m_largestSeenVectorLength))
>             updateProfile();
>diff --git a/Source/JavaScriptCore/bytecode/CodeBlock.cpp b/Source/JavaScriptCore/bytecode/CodeBlock.cpp
>index bc9ed129def31a37d4f78451c3a167a71b8dee09..f8bc37b41ec9454d78032b2eafab5ba37acb8e6b 100644
>--- a/Source/JavaScriptCore/bytecode/CodeBlock.cpp
>+++ b/Source/JavaScriptCore/bytecode/CodeBlock.cpp
>@@ -2626,7 +2626,7 @@ const Identifier& CodeBlock::identifier(int index) const
> }
> #endif // ENABLE(DFG_JIT)
> 
>-void CodeBlock::updateAllPredictionsAndCountLiveness(unsigned& numberOfLiveNonArgumentValueProfiles, unsigned& numberOfSamplesInProfiles)
>+void CodeBlock::updateAllValueProfilePredictionsAndCountLiveness(unsigned& numberOfLiveNonArgumentValueProfiles, unsigned& numberOfSamplesInProfiles)
> {
>     ConcurrentJSLocker locker(m_lock);
> 
>@@ -2663,7 +2663,7 @@ void CodeBlock::updateAllPredictionsAndCountLiveness(unsigned& numberOfLiveNonAr
> void CodeBlock::updateAllValueProfilePredictions()
> {
>     unsigned ignoredValue1, ignoredValue2;
>-    updateAllPredictionsAndCountLiveness(ignoredValue1, ignoredValue2);
>+    updateAllValueProfilePredictionsAndCountLiveness(ignoredValue1, ignoredValue2);
> }
> 
> void CodeBlock::updateAllArrayPredictions()
>@@ -2697,7 +2697,7 @@ bool CodeBlock::shouldOptimizeNow()
>     
>     unsigned numberOfLiveNonArgumentValueProfiles;
>     unsigned numberOfSamplesInProfiles;
>-    updateAllPredictionsAndCountLiveness(numberOfLiveNonArgumentValueProfiles, numberOfSamplesInProfiles);
>+    updateAllValueProfilePredictionsAndCountLiveness(numberOfLiveNonArgumentValueProfiles, numberOfSamplesInProfiles);
> 
>     if (Options::verboseOSR()) {
>         dataLogF(
>diff --git a/Source/JavaScriptCore/bytecode/CodeBlock.h b/Source/JavaScriptCore/bytecode/CodeBlock.h
>index a93c36fb918f3b30cab0c798e61e70ca591b18cf..623092b335d8e4b81b627f603bf9997b3a98956c 100644
>--- a/Source/JavaScriptCore/bytecode/CodeBlock.h
>+++ b/Source/JavaScriptCore/bytecode/CodeBlock.h
>@@ -907,7 +907,7 @@ class CodeBlock : public JSCell {
>     
>     double optimizationThresholdScalingFactor();
> 
>-    void updateAllPredictionsAndCountLiveness(unsigned& numberOfLiveNonArgumentValueProfiles, unsigned& numberOfSamplesInProfiles);
>+    void updateAllValueProfilePredictionsAndCountLiveness(unsigned& numberOfLiveNonArgumentValueProfiles, unsigned& numberOfSamplesInProfiles);
> 
>     void setConstantIdentifierSetRegisters(VM&, const Vector<ConstantIdentifierSetEntry>& constants);
> 
>diff --git a/Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp b/Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp
>index 6505ad57934702119c58fb6787d9a6cd8834d3c5..7e4710242bf81b8548f859dec50adca92934000e 100644
>--- a/Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp
>+++ b/Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp
>@@ -4882,8 +4882,8 @@ void ByteCodeParser::parseBlock(unsigned limit)
>             ArrayAllocationProfile& profile = bytecode.metadata(codeBlock).m_arrayAllocationProfile;
>             for (int operandIdx = startOperand; operandIdx > startOperand - numOperands; --operandIdx)
>                 addVarArgChild(get(VirtualRegister(operandIdx)));
>-            unsigned vectorLengthHint = std::max<unsigned>(profile.vectorLengthHint(), numOperands);
>-            set(bytecode.m_dst, addToGraph(Node::VarArg, NewArray, OpInfo(profile.selectIndexingType()), OpInfo(vectorLengthHint)));
>+            unsigned vectorLengthHint = std::max<unsigned>(profile.vectorLengthHintConcurrently(), numOperands);
>+            set(bytecode.m_dst, addToGraph(Node::VarArg, NewArray, OpInfo(profile.selectIndexingTypeConcurrently()), OpInfo(vectorLengthHint)));
>             NEXT_OPCODE(op_new_array);
>         }
> 
>@@ -4913,7 +4913,7 @@ void ByteCodeParser::parseBlock(unsigned limit)
>         case op_new_array_with_size: {
>             auto bytecode = currentInstruction->as<OpNewArrayWithSize>();
>             ArrayAllocationProfile& profile = bytecode.metadata(codeBlock).m_arrayAllocationProfile;
>-            set(bytecode.m_dst, addToGraph(NewArrayWithSize, OpInfo(profile.selectIndexingType()), get(bytecode.m_length)));
>+            set(bytecode.m_dst, addToGraph(NewArrayWithSize, OpInfo(profile.selectIndexingTypeConcurrently()), get(bytecode.m_length)));
>             NEXT_OPCODE(op_new_array_with_size);
>         }
>             
>diff --git a/JSTests/ChangeLog b/JSTests/ChangeLog
>index 016fe014926479921280074dd526759f0392d761..7fe084c2907065095d633b4769bfa5cb2b36186b 100644
>--- a/JSTests/ChangeLog
>+++ b/JSTests/ChangeLog
>@@ -1,3 +1,13 @@
>+2019-05-22  Yusuke Suzuki  <ysuzuki@apple.com>
>+
>+        [JSC] ArrayAllocationProfile should not access to butterfly in concurrent compiler
>+        https://bugs.webkit.org/show_bug.cgi?id=197809
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * stress/array-allocation-profile-should-not-update-itself-in-concurrent-compiler.js: Added.
>+        (foo):
>+
> 2019-05-22  Commit Queue  <commit-queue@webkit.org>
> 
>         Unreviewed, rolling out r245634.
>diff --git a/JSTests/stress/array-allocation-profile-should-not-update-itself-in-concurrent-compiler.js b/JSTests/stress/array-allocation-profile-should-not-update-itself-in-concurrent-compiler.js
>new file mode 100644
>index 0000000000000000000000000000000000000000..f7f7360e6d14d7d78f446fb3bc0c8e83d908a1cd
>--- /dev/null
>+++ b/JSTests/stress/array-allocation-profile-should-not-update-itself-in-concurrent-compiler.js
>@@ -0,0 +1,11 @@
>+//@ runDefault(" --jitPolicyScale=0", "--useArrayAllocationProfiling=0")
>+function foo() {
>+    for (let i = 0; i < 30; i++) {
>+        const ar = [];
>+        for (let j = 0; j <= 1500; j++) {
>+            ar[j] = null;
>+        }
>+    }
>+}
>+
>+foo();

Flags:

msaboff: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 197809: 369632 | 370456 | 370457