Diff
Modified: trunk/Source/_javascript_Core/ChangeLog (131867 => 131868)
--- trunk/Source/_javascript_Core/ChangeLog 2012-10-19 07:18:58 UTC (rev 131867)
+++ trunk/Source/_javascript_Core/ChangeLog 2012-10-19 07:24:32 UTC (rev 131868)
@@ -1,3 +1,51 @@
+2012-10-18 Filip Pizlo <[email protected]>
+
+ Baseline array profiling should be less accurate, and DFG OSR exit should update array profiles on CheckArray and CheckStructure failure
+ https://bugs.webkit.org/show_bug.cgi?id=99261
+
+ Reviewed by Oliver Hunt.
+
+ This makes array profiling stochastic, like value profiling. The point is to avoid
+ noticing one-off indexing types that we'll never see again, but instead to:
+
+ Notice the big ones: We want the DFG to compile based on the things that happen with
+ high probability. So, this change makes array profiling do like value profiling and
+ only notice a random subsampling of indexing types that flowed through an array
+ access. Prior to this patch array profiles noticed all indexing types and weighted
+ them identically.
+
+ Bias the recent: Often an array access will see awkward indexing types during the
+ first handful of executions because of artifacts of program startup. So, we want to
+ bias towards the indexing types that we saw most recently. With this change, array
+ profiling does like value profiling and usually tells use a random sampling that
+ is biased to what happened recently.
+
+ Have a backup plan: The above two things don't work by themselves because our
+ randomness is not that random (nor do we care enough to make it more random), and
+ because some procedures will have a <1/10 probability event that we must handle
+ without bailing because it dominates a hot loop. So, like value profiling, this
+ patch makes array profiling use OSR exits to tell us why we are bailing out, so
+ that we don't make the same mistake again in the future.
+
+ This change also makes the way that the 32-bit OSR exit compiler snatches scratch
+ registers more uniform. We don't need a scratch buffer when we can push and pop.
+
+ * bytecode/DFGExitProfile.h:
+ * dfg/DFGOSRExitCompiler32_64.cpp:
+ (JSC::DFG::OSRExitCompiler::compileExit):
+ * dfg/DFGOSRExitCompiler64.cpp:
+ (JSC::DFG::OSRExitCompiler::compileExit):
+ * dfg/DFGSpeculativeJIT.cpp:
+ (JSC::DFG::SpeculativeJIT::checkArray):
+ (JSC::DFG::SpeculativeJIT::arrayify):
+ * dfg/DFGSpeculativeJIT32_64.cpp:
+ (JSC::DFG::SpeculativeJIT::compile):
+ * dfg/DFGSpeculativeJIT64.cpp:
+ (JSC::DFG::SpeculativeJIT::compile):
+ * jit/JITInlineMethods.h:
+ (JSC::JIT::emitArrayProfilingSite):
+ * llint/LowLevelInterpreter.asm:
+
2012-10-18 Yuqiang Xian <[email protected]>
[Qt] REGRESSION(r131858): It broke the ARM build
Modified: trunk/Source/_javascript_Core/bytecode/DFGExitProfile.h (131867 => 131868)
--- trunk/Source/_javascript_Core/bytecode/DFGExitProfile.h 2012-10-19 07:18:58 UTC (rev 131867)
+++ trunk/Source/_javascript_Core/bytecode/DFGExitProfile.h 2012-10-19 07:24:32 UTC (rev 131868)
@@ -36,6 +36,7 @@
ExitKindUnset,
BadType, // We exited because a type prediction was wrong.
BadCache, // We exited because an inline cache was wrong.
+ BadIndexingType, // We exited because an indexing type was wrong.
Overflow, // We exited because of overflow.
NegativeZero, // We exited because we encountered negative zero.
OutOfBounds, // We had an out-of-bounds access to an array.
Modified: trunk/Source/_javascript_Core/dfg/DFGOSRExitCompiler32_64.cpp (131867 => 131868)
--- trunk/Source/_javascript_Core/dfg/DFGOSRExitCompiler32_64.cpp 2012-10-19 07:18:58 UTC (rev 131867)
+++ trunk/Source/_javascript_Core/dfg/DFGOSRExitCompiler32_64.cpp 2012-10-19 07:24:32 UTC (rev 131868)
@@ -83,28 +83,85 @@
// 3) Refine some value profile, if appropriate.
- if (!!exit.m_jsValueSource && !!exit.m_valueProfile) {
- EncodedJSValue* bucket = exit.m_valueProfile.getSpecFailBucket(0);
+ if (!!exit.m_jsValueSource) {
+ if (exit.m_kind == BadCache || exit.m_kind == BadIndexingType) {
+ // If the instruction that this originated from has an array profile, then
+ // refine it. If it doesn't, then do nothing. The latter could happen for
+ // hoisted checks, or checks emitted for operations that didn't have array
+ // profiling - either ops that aren't array accesses at all, or weren't
+ // known to be array acceses in the bytecode. The latter case is a FIXME
+ // while the former case is an outcome of a CheckStructure not knowing why
+ // it was emitted (could be either due to an inline cache of a property
+ // property access, or due to an array profile).
+
+ // Note: We are free to assume that the jsValueSource is already known to
+ // be a cell since both BadCache and BadIndexingType exits occur after
+ // the cell check would have already happened.
+
+ CodeOrigin codeOrigin = exit.m_codeOriginForExitProfile;
+ if (ArrayProfile* arrayProfile = m_jit.baselineCodeBlockFor(codeOrigin)->getArrayProfile(codeOrigin.bytecodeIndex)) {
+ GPRReg usedRegister1;
+ GPRReg usedRegister2;
+ if (exit.m_jsValueSource.isAddress()) {
+ usedRegister1 = exit.m_jsValueSource.base();
+ usedRegister2 = InvalidGPRReg;
+ } else {
+ usedRegister1 = exit.m_jsValueSource.payloadGPR();
+ if (exit.m_jsValueSource.hasKnownTag())
+ usedRegister2 = InvalidGPRReg;
+ else
+ usedRegister2 = exit.m_jsValueSource.tagGPR();
+ }
+
+ GPRReg scratch1;
+ GPRReg scratch2;
+ scratch1 = AssemblyHelpers::selectScratchGPR(usedRegister1, usedRegister2);
+ scratch2 = AssemblyHelpers::selectScratchGPR(usedRegister1, usedRegister2, scratch1);
+
+ m_jit.push(scratch1);
+ m_jit.push(scratch2);
+
+ GPRReg value;
+ if (exit.m_jsValueSource.isAddress()) {
+ value = scratch1;
+ m_jit.loadPtr(AssemblyHelpers::Address(exit.m_jsValueSource.asAddress()), value);
+ } else
+ value = exit.m_jsValueSource.payloadGPR();
+
+ m_jit.loadPtr(AssemblyHelpers::Address(value, JSCell::structureOffset()), scratch1);
+ m_jit.storePtr(scratch1, arrayProfile->addressOfLastSeenStructure());
+ m_jit.load8(AssemblyHelpers::Address(scratch1, Structure::indexingTypeOffset()), scratch1);
+ m_jit.move(AssemblyHelpers::TrustedImm32(1), scratch2);
+ m_jit.lshift32(scratch1, scratch2);
+ m_jit.or32(scratch2, AssemblyHelpers::AbsoluteAddress(arrayProfile->addressOfArrayModes()));
+
+ m_jit.pop(scratch2);
+ m_jit.pop(scratch1);
+ }
+ }
- if (exit.m_jsValueSource.isAddress()) {
- // Save a register so we can use it.
- GPRReg scratch = GPRInfo::regT0;
- if (scratch == exit.m_jsValueSource.base())
- scratch = GPRInfo::regT1;
- ScratchBuffer* scratchBuffer = m_jit.globalData()->scratchBufferForSize(sizeof(uint32_t));
- EncodedJSValue* scratchDataBuffer = static_cast<EncodedJSValue*>(scratchBuffer->dataBuffer());
- m_jit.store32(scratch, scratchDataBuffer);
- m_jit.load32(exit.m_jsValueSource.asAddress(OBJECT_OFFSETOF(EncodedValueDescriptor, asBits.tag)), scratch);
- m_jit.store32(scratch, &bitwise_cast<EncodedValueDescriptor*>(bucket)->asBits.tag);
- m_jit.load32(exit.m_jsValueSource.asAddress(OBJECT_OFFSETOF(EncodedValueDescriptor, asBits.payload)), scratch);
- m_jit.store32(scratch, &bitwise_cast<EncodedValueDescriptor*>(bucket)->asBits.payload);
- m_jit.load32(scratchDataBuffer, scratch);
- } else if (exit.m_jsValueSource.hasKnownTag()) {
- m_jit.store32(AssemblyHelpers::TrustedImm32(exit.m_jsValueSource.tag()), &bitwise_cast<EncodedValueDescriptor*>(bucket)->asBits.tag);
- m_jit.store32(exit.m_jsValueSource.payloadGPR(), &bitwise_cast<EncodedValueDescriptor*>(bucket)->asBits.payload);
- } else {
- m_jit.store32(exit.m_jsValueSource.tagGPR(), &bitwise_cast<EncodedValueDescriptor*>(bucket)->asBits.tag);
- m_jit.store32(exit.m_jsValueSource.payloadGPR(), &bitwise_cast<EncodedValueDescriptor*>(bucket)->asBits.payload);
+ if (!!exit.m_valueProfile) {
+ EncodedJSValue* bucket = exit.m_valueProfile.getSpecFailBucket(0);
+
+ if (exit.m_jsValueSource.isAddress()) {
+ // Save a register so we can use it.
+ GPRReg scratch = AssemblyHelpers::selectScratchGPR(exit.m_jsValueSource.base());
+
+ m_jit.push(scratch);
+
+ m_jit.load32(exit.m_jsValueSource.asAddress(OBJECT_OFFSETOF(EncodedValueDescriptor, asBits.tag)), scratch);
+ m_jit.store32(scratch, &bitwise_cast<EncodedValueDescriptor*>(bucket)->asBits.tag);
+ m_jit.load32(exit.m_jsValueSource.asAddress(OBJECT_OFFSETOF(EncodedValueDescriptor, asBits.payload)), scratch);
+ m_jit.store32(scratch, &bitwise_cast<EncodedValueDescriptor*>(bucket)->asBits.payload);
+
+ m_jit.pop(scratch);
+ } else if (exit.m_jsValueSource.hasKnownTag()) {
+ m_jit.store32(AssemblyHelpers::TrustedImm32(exit.m_jsValueSource.tag()), &bitwise_cast<EncodedValueDescriptor*>(bucket)->asBits.tag);
+ m_jit.store32(exit.m_jsValueSource.payloadGPR(), &bitwise_cast<EncodedValueDescriptor*>(bucket)->asBits.payload);
+ } else {
+ m_jit.store32(exit.m_jsValueSource.tagGPR(), &bitwise_cast<EncodedValueDescriptor*>(bucket)->asBits.tag);
+ m_jit.store32(exit.m_jsValueSource.payloadGPR(), &bitwise_cast<EncodedValueDescriptor*>(bucket)->asBits.payload);
+ }
}
}
Modified: trunk/Source/_javascript_Core/dfg/DFGOSRExitCompiler64.cpp (131867 => 131868)
--- trunk/Source/_javascript_Core/dfg/DFGOSRExitCompiler64.cpp 2012-10-19 07:18:58 UTC (rev 131867)
+++ trunk/Source/_javascript_Core/dfg/DFGOSRExitCompiler64.cpp 2012-10-19 07:24:32 UTC (rev 131868)
@@ -86,23 +86,70 @@
}
}
- // 3) Refine some value profile, if appropriate.
+ // 3) Refine some array and/or value profile, if appropriate.
- if (!!exit.m_jsValueSource && !!exit.m_valueProfile) {
- EncodedJSValue* bucket = exit.m_valueProfile.getSpecFailBucket(0);
+ if (!!exit.m_jsValueSource) {
+ if (exit.m_kind == BadCache || exit.m_kind == BadIndexingType) {
+ // If the instruction that this originated from has an array profile, then
+ // refine it. If it doesn't, then do nothing. The latter could happen for
+ // hoisted checks, or checks emitted for operations that didn't have array
+ // profiling - either ops that aren't array accesses at all, or weren't
+ // known to be array acceses in the bytecode. The latter case is a FIXME
+ // while the former case is an outcome of a CheckStructure not knowing why
+ // it was emitted (could be either due to an inline cache of a property
+ // property access, or due to an array profile).
+
+ CodeOrigin codeOrigin = exit.m_codeOriginForExitProfile;
+ if (ArrayProfile* arrayProfile = m_jit.baselineCodeBlockFor(codeOrigin)->getArrayProfile(codeOrigin.bytecodeIndex)) {
+ GPRReg usedRegister;
+ if (exit.m_jsValueSource.isAddress())
+ usedRegister = exit.m_jsValueSource.base();
+ else
+ usedRegister = exit.m_jsValueSource.gpr();
+
+ GPRReg scratch1;
+ GPRReg scratch2;
+ scratch1 = AssemblyHelpers::selectScratchGPR(usedRegister);
+ scratch2 = AssemblyHelpers::selectScratchGPR(usedRegister, scratch1);
+
+ m_jit.push(scratch1);
+ m_jit.push(scratch2);
+
+ GPRReg value;
+ if (exit.m_jsValueSource.isAddress()) {
+ value = scratch1;
+ m_jit.loadPtr(AssemblyHelpers::Address(exit.m_jsValueSource.asAddress()), value);
+ } else
+ value = exit.m_jsValueSource.gpr();
+
+ m_jit.loadPtr(AssemblyHelpers::Address(value, JSCell::structureOffset()), scratch1);
+ m_jit.storePtr(scratch1, arrayProfile->addressOfLastSeenStructure());
+ m_jit.load8(AssemblyHelpers::Address(scratch1, Structure::indexingTypeOffset()), scratch1);
+ m_jit.move(AssemblyHelpers::TrustedImm32(1), scratch2);
+ m_jit.lshift32(scratch1, scratch2);
+ m_jit.or32(scratch2, AssemblyHelpers::AbsoluteAddress(arrayProfile->addressOfArrayModes()));
+
+ m_jit.pop(scratch2);
+ m_jit.pop(scratch1);
+ }
+ }
+ if (!!exit.m_valueProfile) {
+ EncodedJSValue* bucket = exit.m_valueProfile.getSpecFailBucket(0);
+
#if DFG_ENABLE(VERBOSE_SPECULATION_FAILURE)
- dataLog(" (have exit profile, bucket %p) ", bucket);
+ dataLog(" (have exit profile, bucket %p) ", bucket);
#endif
- if (exit.m_jsValueSource.isAddress()) {
- // We can't be sure that we have a spare register. So use the tagTypeNumberRegister,
- // since we know how to restore it.
- m_jit.load64(AssemblyHelpers::Address(exit.m_jsValueSource.asAddress()), GPRInfo::tagTypeNumberRegister);
- m_jit.store64(GPRInfo::tagTypeNumberRegister, bucket);
- m_jit.move(AssemblyHelpers::TrustedImm64(bitwise_cast<int64_t>(TagTypeNumber)), GPRInfo::tagTypeNumberRegister);
- } else
- m_jit.store64(exit.m_jsValueSource.gpr(), bucket);
+ if (exit.m_jsValueSource.isAddress()) {
+ // We can't be sure that we have a spare register. So use the tagTypeNumberRegister,
+ // since we know how to restore it.
+ m_jit.load64(AssemblyHelpers::Address(exit.m_jsValueSource.asAddress()), GPRInfo::tagTypeNumberRegister);
+ m_jit.store64(GPRInfo::tagTypeNumberRegister, bucket);
+ m_jit.move(AssemblyHelpers::TrustedImmPtr(bitwise_cast<void*>(TagTypeNumber)), GPRInfo::tagTypeNumberRegister);
+ } else
+ m_jit.store64(exit.m_jsValueSource.gpr(), bucket);
+ }
}
// 4) Figure out how many scratch slots we'll need. We need one for every GPR/FPR
Modified: trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT.cpp (131867 => 131868)
--- trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT.cpp 2012-10-19 07:18:58 UTC (rev 131867)
+++ trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT.cpp 2012-10-19 07:24:32 UTC (rev 131868)
@@ -437,7 +437,7 @@
MacroAssembler::Address(baseReg, JSCell::structureOffset()), tempGPR);
m_jit.load8(MacroAssembler::Address(tempGPR, Structure::indexingTypeOffset()), tempGPR);
speculationCheck(
- Uncountable, JSValueRegs(), NoNode,
+ BadIndexingType, JSValueSource::unboxedCell(baseReg), NoNode,
jumpSlowForUnwantedArrayMode(tempGPR, node.arrayMode()));
noResult(m_compileIndex);
@@ -535,7 +535,7 @@
// Next check that the object does not intercept indexed accesses. If it does,
// then this mode won't work.
speculationCheck(
- Uncountable, JSValueRegs(), NoNode,
+ BadIndexingType, JSValueSource::unboxedCell(baseReg), NoNode,
m_jit.branchTest8(
MacroAssembler::NonZero,
MacroAssembler::Address(structureGPR, Structure::typeInfoFlagsOffset()),
@@ -569,7 +569,7 @@
m_jit.load8(
MacroAssembler::Address(structureGPR, Structure::indexingTypeOffset()), structureGPR);
speculationCheck(
- Uncountable, JSValueRegs(), NoNode,
+ BadIndexingType, JSValueSource::unboxedCell(baseReg), NoNode,
jumpSlowForUnwantedArrayMode(structureGPR, desiredArrayMode));
done.link(&m_jit);
Modified: trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT32_64.cpp (131867 => 131868)
--- trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT32_64.cpp 2012-10-19 07:18:58 UTC (rev 131867)
+++ trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT32_64.cpp 2012-10-19 07:24:32 UTC (rev 131868)
@@ -3879,7 +3879,7 @@
if (node.structureSet().size() == 1) {
speculationCheckWithConditionalDirection(
- BadCache, JSValueRegs(), NoNode,
+ BadCache, JSValueSource::unboxedCell(base.gpr()), NoNode,
m_jit.branchWeakPtr(
JITCompiler::NotEqual,
JITCompiler::Address(base.gpr(), JSCell::structureOffset()),
@@ -3896,7 +3896,7 @@
done.append(m_jit.branchWeakPtr(JITCompiler::Equal, structure.gpr(), node.structureSet()[i]));
speculationCheckWithConditionalDirection(
- BadCache, JSValueRegs(), NoNode,
+ BadCache, JSValueSource::unboxedCell(base.gpr()), NoNode,
m_jit.branchWeakPtr(
JITCompiler::NotEqual, structure.gpr(), node.structureSet().last()),
node.op() == ForwardCheckStructure);
@@ -3910,6 +3910,13 @@
case StructureTransitionWatchpoint:
case ForwardStructureTransitionWatchpoint: {
+ // There is a fascinating question here of what to do about array profiling.
+ // We *could* try to tell the OSR exit about where the base of the access is.
+ // The DFG will have kept it alive, though it may not be in a register, and
+ // we shouldn't really load it since that could be a waste. For now though,
+ // we'll just rely on the fact that when a watchpoint fires then that's
+ // quite a hint already.
+
m_jit.addWeakReference(node.structure());
node.structure()->addTransitionWatchpoint(
speculationWatchpointWithConditionalDirection(
Modified: trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT64.cpp (131867 => 131868)
--- trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT64.cpp 2012-10-19 07:18:58 UTC (rev 131867)
+++ trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT64.cpp 2012-10-19 07:24:32 UTC (rev 131868)
@@ -3875,7 +3875,7 @@
if (node.structureSet().size() == 1) {
speculationCheckWithConditionalDirection(
- BadCache, JSValueRegs(), NoNode,
+ BadCache, JSValueRegs(base.gpr()), NoNode,
m_jit.branchWeakPtr(
JITCompiler::NotEqual,
JITCompiler::Address(base.gpr(), JSCell::structureOffset()),
@@ -3892,7 +3892,7 @@
done.append(m_jit.branchWeakPtr(JITCompiler::Equal, structure.gpr(), node.structureSet()[i]));
speculationCheckWithConditionalDirection(
- BadCache, JSValueRegs(), NoNode,
+ BadCache, JSValueRegs(base.gpr()), NoNode,
m_jit.branchWeakPtr(
JITCompiler::NotEqual, structure.gpr(), node.structureSet().last()),
node.op() == ForwardCheckStructure);
@@ -3906,6 +3906,13 @@
case StructureTransitionWatchpoint:
case ForwardStructureTransitionWatchpoint: {
+ // There is a fascinating question here of what to do about array profiling.
+ // We *could* try to tell the OSR exit about where the base of the access is.
+ // The DFG will have kept it alive, though it may not be in a register, and
+ // we shouldn't really load it since that could be a waste. For now though,
+ // we'll just rely on the fact that when a watchpoint fires then that's
+ // quite a hint already.
+
m_jit.addWeakReference(node.structure());
node.structure()->addTransitionWatchpoint(
speculationWatchpointWithConditionalDirection(
Modified: trunk/Source/_javascript_Core/jit/JITInlineMethods.h (131867 => 131868)
--- trunk/Source/_javascript_Core/jit/JITInlineMethods.h 2012-10-19 07:18:58 UTC (rev 131867)
+++ trunk/Source/_javascript_Core/jit/JITInlineMethods.h 2012-10-19 07:24:32 UTC (rev 131868)
@@ -548,17 +548,15 @@
inline void JIT::emitArrayProfilingSite(RegisterID structureAndIndexingType, RegisterID scratch, ArrayProfile* arrayProfile)
{
+ UNUSED_PARAM(scratch); // We had found this scratch register useful here before, so I will keep it for now.
+
RegisterID structure = structureAndIndexingType;
RegisterID indexingType = structureAndIndexingType;
- if (canBeOptimized()) {
+ if (canBeOptimized())
storePtr(structure, arrayProfile->addressOfLastSeenStructure());
- load8(Address(structure, Structure::indexingTypeOffset()), indexingType);
- move(TrustedImm32(1), scratch);
- lshift32(indexingType, scratch);
- or32(scratch, AbsoluteAddress(arrayProfile->addressOfArrayModes()));
- } else
- load8(Address(structure, Structure::indexingTypeOffset()), indexingType);
+
+ load8(Address(structure, Structure::indexingTypeOffset()), indexingType);
}
inline void JIT::emitArrayProfilingSiteForBytecodeIndex(RegisterID structureAndIndexingType, RegisterID scratch, unsigned bytecodeIndex)
Modified: trunk/Source/_javascript_Core/llint/LowLevelInterpreter.asm (131867 => 131868)
--- trunk/Source/_javascript_Core/llint/LowLevelInterpreter.asm 2012-10-19 07:18:58 UTC (rev 131867)
+++ trunk/Source/_javascript_Core/llint/LowLevelInterpreter.asm 2012-10-19 07:24:32 UTC (rev 131868)
@@ -239,13 +239,8 @@
const indexingType = structureAndIndexingType
if VALUE_PROFILER
storep structure, ArrayProfile::m_lastSeenStructure[profile]
- loadb Structure::m_indexingType[structure], indexingType
- move 1, scratch
- lshifti indexingType, scratch
- ori scratch, ArrayProfile::m_observedArrayModes[profile]
- else
- loadb Structure::m_indexingType[structure], indexingType
end
+ loadb Structure::m_indexingType[structure], indexingType
end
macro checkSwitchToJIT(increment, action)
