Skip to content

Commit d2cc956

Browse files
ConstellationConstellation
committed
[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): Canonical link: https://commits.webkit.org/212223@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@245667 268f45cc-cd09-0410-ab3c-d52691b4dbfc
1 parent 9cdeefe commit d2cc956

8 files changed

Lines changed: 77 additions & 7 deletions

‎JSTests/ChangeLog‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,13 @@
1+
2019-05-22 Yusuke Suzuki <ysuzuki@apple.com>
2+
3+
[JSC] ArrayAllocationProfile should not access to butterfly in concurrent compiler
4+
https://bugs.webkit.org/show_bug.cgi?id=197809
5+
6+
Reviewed by Michael Saboff.
7+
8+
* stress/array-allocation-profile-should-not-update-itself-in-concurrent-compiler.js: Added.
9+
(foo):
10+
111
2019-05-22 Ross Kirsling <ross.kirsling@sony.com>
212

313
[ESNext] Implement support for Numeric Separators

‎JSTests/stress/array-allocation-profile-should-not-update-itself-in-concurrent-compiler.js‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
//@ runDefault("--jitPolicyScale=0", "--useArrayAllocationProfiling=0")
2+
function foo() {
3+
for (let i = 0; i < 30; i++) {
4+
const ar = [];
5+
for (let j = 0; j <= 1500; j++) {
6+
ar[j] = null;
7+
}
8+
}
9+
}
10+
11+
foo();

‎Source/JavaScriptCore/ChangeLog‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,34 @@
1+
2019-05-22 Yusuke Suzuki <ysuzuki@apple.com>
2+
3+
[JSC] ArrayAllocationProfile should not access to butterfly in concurrent compiler
4+
https://bugs.webkit.org/show_bug.cgi?id=197809
5+
6+
Reviewed by Michael Saboff.
7+
8+
ArrayAllocationProfile assumes that Butterfly can be accessed concurrently. But this is not correct now
9+
since LargeAllocation Butterfly can be realloced. In this patch, we switch profiling array allocations
10+
only in the main thread. This allocation profiling is repeatedly called in the main thread's slow path,
11+
and it is also called when updating the profiles in the main thread.
12+
13+
We also rename updateAllPredictionsAndCountLiveness to updateAllValueProfilePredictionsAndCountLiveness
14+
since it only cares ValueProfiles.
15+
16+
* bytecode/ArrayAllocationProfile.cpp:
17+
(JSC::ArrayAllocationProfile::updateProfile):
18+
* bytecode/ArrayAllocationProfile.h:
19+
(JSC::ArrayAllocationProfile::selectIndexingTypeConcurrently):
20+
(JSC::ArrayAllocationProfile::selectIndexingType):
21+
(JSC::ArrayAllocationProfile::vectorLengthHintConcurrently):
22+
(JSC::ArrayAllocationProfile::vectorLengthHint):
23+
* bytecode/CodeBlock.cpp:
24+
(JSC::CodeBlock::updateAllValueProfilePredictionsAndCountLiveness):
25+
(JSC::CodeBlock::updateAllValueProfilePredictions):
26+
(JSC::CodeBlock::shouldOptimizeNow):
27+
(JSC::CodeBlock::updateAllPredictionsAndCountLiveness): Deleted.
28+
* bytecode/CodeBlock.h:
29+
* dfg/DFGByteCodeParser.cpp:
30+
(JSC::DFG::ByteCodeParser::parseBlock):
31+
132
2019-05-22 Yusuke Suzuki <ysuzuki@apple.com>
233

334
[JSC] Shrink Metadata

‎Source/JavaScriptCore/bytecode/ArrayAllocationProfile.cpp‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,13 @@ void ArrayAllocationProfile::updateProfile()
4747
// it's possible for that array to no longer be reachable, it cannot actually
4848
// be freed, since we require the GC to wait until all concurrent JITing
4949
// finishes.
50+
//
51+
// But one exception is vector length. We access vector length to get the vector
52+
// length hint. However vector length can be accessible only from the main
53+
// thread because large butterfly can be realloced in the main thread.
54+
// So for now, we update the allocation profile only from the main thread.
5055

56+
ASSERT(!isCompilationThread());
5157
JSArray* lastArray = m_lastArray;
5258
if (!lastArray)
5359
return;

‎Source/JavaScriptCore/bytecode/ArrayAllocationProfile.h‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,17 +39,29 @@ class ArrayAllocationProfile {
3939
initializeIndexingMode(recommendedIndexingMode);
4040
}
4141

42+
IndexingType selectIndexingTypeConcurrently()
43+
{
44+
return m_currentIndexingType;
45+
}
46+
4247
IndexingType selectIndexingType()
4348
{
49+
ASSERT(!isCompilationThread());
4450
JSArray* lastArray = m_lastArray;
4551
if (lastArray && UNLIKELY(lastArray->indexingType() != m_currentIndexingType))
4652
updateProfile();
4753
return m_currentIndexingType;
4854
}
4955

5056
// vector length hint becomes [0, BASE_CONTIGUOUS_VECTOR_LEN_MAX].
57+
unsigned vectorLengthHintConcurrently()
58+
{
59+
return m_largestSeenVectorLength;
60+
}
61+
5162
unsigned vectorLengthHint()
5263
{
64+
ASSERT(!isCompilationThread());
5365
JSArray* lastArray = m_lastArray;
5466
if (lastArray && (m_largestSeenVectorLength != BASE_CONTIGUOUS_VECTOR_LEN_MAX) && UNLIKELY(lastArray->getVectorLength() > m_largestSeenVectorLength))
5567
updateProfile();

‎Source/JavaScriptCore/bytecode/CodeBlock.cpp‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2626,7 +2626,7 @@ const Identifier& CodeBlock::identifier(int index) const
26262626
}
26272627
#endif // ENABLE(DFG_JIT)
26282628

2629-
void CodeBlock::updateAllPredictionsAndCountLiveness(unsigned& numberOfLiveNonArgumentValueProfiles, unsigned& numberOfSamplesInProfiles)
2629+
void CodeBlock::updateAllValueProfilePredictionsAndCountLiveness(unsigned& numberOfLiveNonArgumentValueProfiles, unsigned& numberOfSamplesInProfiles)
26302630
{
26312631
ConcurrentJSLocker locker(m_lock);
26322632

@@ -2664,7 +2664,7 @@ void CodeBlock::updateAllPredictionsAndCountLiveness(unsigned& numberOfLiveNonAr
26642664
void CodeBlock::updateAllValueProfilePredictions()
26652665
{
26662666
unsigned ignoredValue1, ignoredValue2;
2667-
updateAllPredictionsAndCountLiveness(ignoredValue1, ignoredValue2);
2667+
updateAllValueProfilePredictionsAndCountLiveness(ignoredValue1, ignoredValue2);
26682668
}
26692669

26702670
void CodeBlock::updateAllArrayPredictions()
@@ -2698,7 +2698,7 @@ bool CodeBlock::shouldOptimizeNow()
26982698

26992699
unsigned numberOfLiveNonArgumentValueProfiles;
27002700
unsigned numberOfSamplesInProfiles;
2701-
updateAllPredictionsAndCountLiveness(numberOfLiveNonArgumentValueProfiles, numberOfSamplesInProfiles);
2701+
updateAllValueProfilePredictionsAndCountLiveness(numberOfLiveNonArgumentValueProfiles, numberOfSamplesInProfiles);
27022702

27032703
if (Options::verboseOSR()) {
27042704
dataLogF(

‎Source/JavaScriptCore/bytecode/CodeBlock.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -906,7 +906,7 @@ class CodeBlock : public JSCell {
906906

907907
double optimizationThresholdScalingFactor();
908908

909-
void updateAllPredictionsAndCountLiveness(unsigned& numberOfLiveNonArgumentValueProfiles, unsigned& numberOfSamplesInProfiles);
909+
void updateAllValueProfilePredictionsAndCountLiveness(unsigned& numberOfLiveNonArgumentValueProfiles, unsigned& numberOfSamplesInProfiles);
910910

911911
void setConstantIdentifierSetRegisters(VM&, const Vector<ConstantIdentifierSetEntry>& constants);
912912

‎Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4885,8 +4885,8 @@ void ByteCodeParser::parseBlock(unsigned limit)
48854885
ArrayAllocationProfile& profile = bytecode.metadata(codeBlock).m_arrayAllocationProfile;
48864886
for (int operandIdx = startOperand; operandIdx > startOperand - numOperands; --operandIdx)
48874887
addVarArgChild(get(VirtualRegister(operandIdx)));
4888-
unsigned vectorLengthHint = std::max<unsigned>(profile.vectorLengthHint(), numOperands);
4889-
set(bytecode.m_dst, addToGraph(Node::VarArg, NewArray, OpInfo(profile.selectIndexingType()), OpInfo(vectorLengthHint)));
4888+
unsigned vectorLengthHint = std::max<unsigned>(profile.vectorLengthHintConcurrently(), numOperands);
4889+
set(bytecode.m_dst, addToGraph(Node::VarArg, NewArray, OpInfo(profile.selectIndexingTypeConcurrently()), OpInfo(vectorLengthHint)));
48904890
NEXT_OPCODE(op_new_array);
48914891
}
48924892

@@ -4916,7 +4916,7 @@ void ByteCodeParser::parseBlock(unsigned limit)
49164916
case op_new_array_with_size: {
49174917
auto bytecode = currentInstruction->as<OpNewArrayWithSize>();
49184918
ArrayAllocationProfile& profile = bytecode.metadata(codeBlock).m_arrayAllocationProfile;
4919-
set(bytecode.m_dst, addToGraph(NewArrayWithSize, OpInfo(profile.selectIndexingType()), get(bytecode.m_length)));
4919+
set(bytecode.m_dst, addToGraph(NewArrayWithSize, OpInfo(profile.selectIndexingTypeConcurrently()), get(bytecode.m_length)));
49204920
NEXT_OPCODE(op_new_array_with_size);
49214921
}
49224922

0 commit comments

Comments
 (0)