WebKit Bugzilla
Attachment 370456 Details for
Bug 197809
: [JSC] ArrayAllocationProfile should not access to butterfly in concurrent compiler
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Requests
|
Help
|
New Account
|
Log In
Remember
[x]
|
Forgot Password
Login:
[x]
[patch]
Patch
bug-197809-20190522154855.patch (text/plain), 10.27 KB, created by
Yusuke Suzuki
on 2019-05-22 15:48:56 PDT
(
hide
)
Description:
Patch
Filename:
MIME Type:
Creator:
Yusuke Suzuki
Created:
2019-05-22 15:48:56 PDT
Size:
10.27 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..3f38afb99b34d7297cf63b8a060c785c4fec599f 100644 >--- a/Source/JavaScriptCore/bytecode/ArrayAllocationProfile.cpp >+++ b/Source/JavaScriptCore/bytecode/ArrayAllocationProfile.cpp >@@ -47,6 +47,11 @@ 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. > > JSArray* lastArray = m_lastArray; > if (!lastArray) >@@ -60,7 +65,8 @@ void ArrayAllocationProfile::updateProfile() > indexingType |= CopyOnWrite; > } > m_currentIndexingType = indexingType; >- m_largestSeenVectorLength = std::min(std::max(m_largestSeenVectorLength, lastArray->getVectorLength()), BASE_CONTIGUOUS_VECTOR_LEN_MAX); >+ if (!isCompilationThread()) >+ m_largestSeenVectorLength = std::min(std::max(m_largestSeenVectorLength, lastArray->getVectorLength()), BASE_CONTIGUOUS_VECTOR_LEN_MAX); > } > m_lastArray = nullptr; > } >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();
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Formatted Diff
|
Diff
Attachments on
bug 197809
:
369632
|
370456
|
370457