Changeset 245667 in webkit

Timestamp:

May 22, 2019, 6:22:33 PM (7 years ago)

Author:

ysuzuki@apple.com

Message:

[JSC] ArrayAllocationProfile should not access to butterfly in concurrent compiler
​https://bugs.webkit.org/show_bug.cgi?id=197809

Reviewed by Michael Saboff.

JSTests:

• stress/array-allocation-profile-should-not-update-itself-in-concurrent-compiler.js: Added.

(foo):

Source/JavaScriptCore:

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):

Location:

trunk

Files:

• JSTests/ChangeLog (modified) (1 diff)
• JSTests/stress/array-allocation-profile-should-not-update-itself-in-concurrent-compiler.js (added)
• Source/JavaScriptCore/ChangeLog (modified) (1 diff)
• Source/JavaScriptCore/bytecode/ArrayAllocationProfile.cpp (modified) (1 diff)
• Source/JavaScriptCore/bytecode/ArrayAllocationProfile.h (modified) (2 diffs)
• Source/JavaScriptCore/bytecode/CodeBlock.cpp (modified) (3 diffs)
• Source/JavaScriptCore/bytecode/CodeBlock.h (modified) (1 diff)
• Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp (modified) (2 diffs)

trunk/JSTests/ChangeLog

@@ +1 @@
+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 Michael Saboff.
+
+        * stress/array-allocation-profile-should-not-update-itself-in-concurrent-compiler.js: Added.
+        (foo):
+
 2019-05-22  Ross Kirsling  <ross.kirsling@sony.com>
 

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+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 Michael Saboff.
+
+        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  Yusuke Suzuki  <ysuzuki@apple.com>
 

trunk/Source/JavaScriptCore/bytecode/ArrayAllocationProfile.cpp

@@ -48 +48 @@
     //   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)

trunk/Source/JavaScriptCore/bytecode/ArrayAllocationProfile.h

@@ -40 +40 @@
     }
 
+    IndexingType selectIndexingTypeConcurrently()
+    {
+        return m_currentIndexingType;
+    }
+
     IndexingType selectIndexingType()
     {
+        ASSERT(!isCompilationThread());
         JSArray* lastArray = m_lastArray;
         if (lastArray && UNLIKELY(lastArray->indexingType() != m_currentIndexingType))
@@ -49 +55 @@
 
     // 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))

trunk/Source/JavaScriptCore/bytecode/CodeBlock.cpp

@@ -2627 +2627 @@
 #endif // ENABLE(DFG_JIT)
 
-void CodeBlock::updateAllPredictionsAndCountLiveness(unsigned& numberOfLiveNonArgumentValueProfiles, unsigned& numberOfSamplesInProfiles)
+void CodeBlock::updateAllValueProfilePredictionsAndCountLiveness(unsigned& numberOfLiveNonArgumentValueProfiles, unsigned& numberOfSamplesInProfiles)
 {
     ConcurrentJSLocker locker(m_lock);
@@ -2665 +2665 @@
 {
     unsigned ignoredValue1, ignoredValue2;
-    updateAllPredictionsAndCountLiveness(ignoredValue1, ignoredValue2);
+    updateAllValueProfilePredictionsAndCountLiveness(ignoredValue1, ignoredValue2);
 }
 
@@ -2699 +2699 @@
     unsigned numberOfLiveNonArgumentValueProfiles;
     unsigned numberOfSamplesInProfiles;
-    updateAllPredictionsAndCountLiveness(numberOfLiveNonArgumentValueProfiles, numberOfSamplesInProfiles);
+    updateAllValueProfilePredictionsAndCountLiveness(numberOfLiveNonArgumentValueProfiles, numberOfSamplesInProfiles);
 
     if (Options::verboseOSR()) {

trunk/Source/JavaScriptCore/bytecode/CodeBlock.h

@@ -907 +907 @@
     double optimizationThresholdScalingFactor();
 
-    void updateAllPredictionsAndCountLiveness(unsigned& numberOfLiveNonArgumentValueProfiles, unsigned& numberOfSamplesInProfiles);
+    void updateAllValueProfilePredictionsAndCountLiveness(unsigned& numberOfLiveNonArgumentValueProfiles, unsigned& numberOfSamplesInProfiles);
 
     void setConstantIdentifierSetRegisters(VM&, const Vector<ConstantIdentifierSetEntry>& constants);

trunk/Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp

@@ -4886 +4886 @@
             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);
         }
@@ -4917 +4917 @@
             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);
         }