[JSC] Remove RageConvert array conversion
https://bugs.webkit.org/show_bug.cgi?id=144433
Patch by Benjamin Poulain <bpoulain@apple.com> on 2015-04-29
Reviewed by Filip Pizlo.
RageConvert was causing a subtle bug that was hitting the Kraken crypto tests
pretty hard:
-The indexing types shows that the array access varies between Int32 and DoubleArray.
-ArrayMode::fromObserved() decided to use the most generic type: DoubleArray.
An Arrayify node would convert the Int32 to that type.
-Somewhere, a GetByVal or PutByVal would have the flag NodeBytecodeUsesAsInt. That
node would use RageConvert instead of Convert.
-The Arrayify for that GetByVal with RageConvert would not convert the array to
Contiguous.
-All the following array access that do not have the flag NodeBytecodeUsesAsInt would
now expect a DoubleArray and always get a Contiguous Array. The CheckStructure
fail systematically and we never get to run the later code.
Getting rid of RageConvert fixes the problem and does not seems to have any
negative side effect on other benchmarks.
The improvments on Kraken are:
-stanford-crypto-aes: definitely 1.0915x faster.
-stanford-crypto-pbkdf2: definitely 1.2446x faster.
-stanford-crypto-sha256-iterative: definitely 1.0544x faster.
* dfg/DFGAbstractInterpreterInlines.h:
(JSC::DFG::AbstractInterpreter<AbstractStateType>::executeEffects):
* dfg/DFGArrayMode.cpp:
(JSC::DFG::ArrayMode::refine):
(JSC::DFG::arrayConversionToString):
* dfg/DFGArrayMode.h:
* dfg/DFGArrayifySlowPathGenerator.h:
* dfg/DFGFixupPhase.cpp:
(JSC::DFG::FixupPhase::fixupNode):
* dfg/DFGOperations.cpp:
* dfg/DFGOperations.h:
* dfg/DFGPredictionPropagationPhase.cpp:
(JSC::DFG::PredictionPropagationPhase::propagate):
* dfg/DFGTypeCheckHoistingPhase.cpp:
(JSC::DFG::TypeCheckHoistingPhase::identifyRedundantStructureChecks):
* ftl/FTLLowerDFGToLLVM.cpp:
(JSC::FTL::LowerDFGToLLVM::compileArrayifyToStructure):
* runtime/JSObject.cpp:
(JSC::JSObject::convertDoubleToContiguous):
(JSC::JSObject::ensureContiguousSlow):
(JSC::JSObject::genericConvertDoubleToContiguous): Deleted.
(JSC::JSObject::rageConvertDoubleToContiguous): Deleted.
(JSC::JSObject::rageEnsureContiguousSlow): Deleted.
* runtime/JSObject.h:
(JSC::JSObject::rageEnsureContiguous): Deleted.
git-svn-id: http://svn.webkit.org/repository/webkit/trunk@183615 268f45cc-cd09-0410-ab3c-d52691b4dbfc
diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
index 09d1143..bbba860 100644
--- a/Source/JavaScriptCore/ChangeLog
+++ b/Source/JavaScriptCore/ChangeLog
@@ -1,3 +1,57 @@
+2015-04-29 Benjamin Poulain <bpoulain@apple.com>
+
+ [JSC] Remove RageConvert array conversion
+ https://bugs.webkit.org/show_bug.cgi?id=144433
+
+ Reviewed by Filip Pizlo.
+
+ RageConvert was causing a subtle bug that was hitting the Kraken crypto tests
+ pretty hard:
+ -The indexing types shows that the array access varies between Int32 and DoubleArray.
+ -ArrayMode::fromObserved() decided to use the most generic type: DoubleArray.
+ An Arrayify node would convert the Int32 to that type.
+ -Somewhere, a GetByVal or PutByVal would have the flag NodeBytecodeUsesAsInt. That
+ node would use RageConvert instead of Convert.
+ -The Arrayify for that GetByVal with RageConvert would not convert the array to
+ Contiguous.
+ -All the following array access that do not have the flag NodeBytecodeUsesAsInt would
+ now expect a DoubleArray and always get a Contiguous Array. The CheckStructure
+ fail systematically and we never get to run the later code.
+
+ Getting rid of RageConvert fixes the problem and does not seems to have any
+ negative side effect on other benchmarks.
+
+ The improvments on Kraken are:
+ -stanford-crypto-aes: definitely 1.0915x faster.
+ -stanford-crypto-pbkdf2: definitely 1.2446x faster.
+ -stanford-crypto-sha256-iterative: definitely 1.0544x faster.
+
+ * dfg/DFGAbstractInterpreterInlines.h:
+ (JSC::DFG::AbstractInterpreter<AbstractStateType>::executeEffects):
+ * dfg/DFGArrayMode.cpp:
+ (JSC::DFG::ArrayMode::refine):
+ (JSC::DFG::arrayConversionToString):
+ * dfg/DFGArrayMode.h:
+ * dfg/DFGArrayifySlowPathGenerator.h:
+ * dfg/DFGFixupPhase.cpp:
+ (JSC::DFG::FixupPhase::fixupNode):
+ * dfg/DFGOperations.cpp:
+ * dfg/DFGOperations.h:
+ * dfg/DFGPredictionPropagationPhase.cpp:
+ (JSC::DFG::PredictionPropagationPhase::propagate):
+ * dfg/DFGTypeCheckHoistingPhase.cpp:
+ (JSC::DFG::TypeCheckHoistingPhase::identifyRedundantStructureChecks):
+ * ftl/FTLLowerDFGToLLVM.cpp:
+ (JSC::FTL::LowerDFGToLLVM::compileArrayifyToStructure):
+ * runtime/JSObject.cpp:
+ (JSC::JSObject::convertDoubleToContiguous):
+ (JSC::JSObject::ensureContiguousSlow):
+ (JSC::JSObject::genericConvertDoubleToContiguous): Deleted.
+ (JSC::JSObject::rageConvertDoubleToContiguous): Deleted.
+ (JSC::JSObject::rageEnsureContiguousSlow): Deleted.
+ * runtime/JSObject.h:
+ (JSC::JSObject::rageEnsureContiguous): Deleted.
+
2015-04-29 Joseph Pecoraro <pecoraro@apple.com>
Gracefully handle missing auto pause key on remote inspector setup
diff --git a/Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h b/Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h
index 12d283c1..22f9ccc 100644
--- a/Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h
+++ b/Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h
@@ -1686,8 +1686,7 @@
m_state.setFoundConstants(true);
break;
}
- ASSERT(node->arrayMode().conversion() == Array::Convert
- || node->arrayMode().conversion() == Array::RageConvert);
+ ASSERT(node->arrayMode().conversion() == Array::Convert);
clobberStructures(clobberLimit);
filterArrayModes(node->child1(), node->arrayMode().arrayModesThatPassFiltering());
break;
diff --git a/Source/JavaScriptCore/dfg/DFGArrayMode.cpp b/Source/JavaScriptCore/dfg/DFGArrayMode.cpp
index 7f31140..ea67bfb 100644
--- a/Source/JavaScriptCore/dfg/DFGArrayMode.cpp
+++ b/Source/JavaScriptCore/dfg/DFGArrayMode.cpp
@@ -151,8 +151,7 @@
ArrayMode ArrayMode::refine(
Graph& graph, Node* node,
- SpeculatedType base, SpeculatedType index, SpeculatedType value,
- NodeFlags flags) const
+ SpeculatedType base, SpeculatedType index, SpeculatedType value) const
{
if (!base || !index) {
// It can be that we had a legitimate arrayMode but no incoming predictions. That'll
@@ -197,15 +196,11 @@
return withTypeAndConversion(Array::Contiguous, Array::Convert);
case Array::Double:
- if (flags & NodeBytecodeUsesAsInt)
- return withTypeAndConversion(Array::Contiguous, Array::RageConvert);
if (!value || isFullNumberSpeculation(value))
return *this;
return withTypeAndConversion(Array::Contiguous, Array::Convert);
case Array::Contiguous:
- if (doesConversion() && (flags & NodeBytecodeUsesAsInt))
- return withConversion(Array::RageConvert);
return *this;
case Array::Int8Array:
@@ -579,8 +574,6 @@
return "AsIs";
case Array::Convert:
return "Convert";
- case Array::RageConvert:
- return "RageConvert";
default:
return "Unknown!";
}
diff --git a/Source/JavaScriptCore/dfg/DFGArrayMode.h b/Source/JavaScriptCore/dfg/DFGArrayMode.h
index b381ce3..e9ed23a 100644
--- a/Source/JavaScriptCore/dfg/DFGArrayMode.h
+++ b/Source/JavaScriptCore/dfg/DFGArrayMode.h
@@ -95,8 +95,7 @@
};
enum Conversion {
AsIs,
- Convert,
- RageConvert
+ Convert
};
} // namespace Array
@@ -222,7 +221,7 @@
return ArrayMode(type, arrayClass(), speculation(), conversion);
}
- ArrayMode refine(Graph&, Node*, SpeculatedType base, SpeculatedType index, SpeculatedType value = SpecNone, NodeFlags = 0) const;
+ ArrayMode refine(Graph&, Node*, SpeculatedType base, SpeculatedType index, SpeculatedType value = SpecNone) const;
bool alreadyChecked(Graph&, Node*, AbstractValue&) const;
diff --git a/Source/JavaScriptCore/dfg/DFGArrayifySlowPathGenerator.h b/Source/JavaScriptCore/dfg/DFGArrayifySlowPathGenerator.h
index 6d54b01..9019eb1 100644
--- a/Source/JavaScriptCore/dfg/DFGArrayifySlowPathGenerator.h
+++ b/Source/JavaScriptCore/dfg/DFGArrayifySlowPathGenerator.h
@@ -101,10 +101,7 @@
jit->callOperation(operationEnsureDouble, m_tempGPR, m_baseGPR);
break;
case Array::Contiguous:
- if (m_arrayMode.conversion() == Array::RageConvert)
- jit->callOperation(operationRageEnsureContiguous, m_tempGPR, m_baseGPR);
- else
- jit->callOperation(operationEnsureContiguous, m_tempGPR, m_baseGPR);
+ jit->callOperation(operationEnsureContiguous, m_tempGPR, m_baseGPR);
break;
case Array::ArrayStorage:
case Array::SlowPutArrayStorage:
diff --git a/Source/JavaScriptCore/dfg/DFGFixupPhase.cpp b/Source/JavaScriptCore/dfg/DFGFixupPhase.cpp
index 77662c6..652cb6a 100644
--- a/Source/JavaScriptCore/dfg/DFGFixupPhase.cpp
+++ b/Source/JavaScriptCore/dfg/DFGFixupPhase.cpp
@@ -517,7 +517,7 @@
m_graph, node,
node->child1()->prediction(),
node->child2()->prediction(),
- SpecNone, node->flags()));
+ SpecNone));
blessArrayOperation(node->child1(), node->child2(), node->child3());
@@ -1105,7 +1105,7 @@
m_graph, node,
node->child1()->prediction(),
node->child2()->prediction(),
- SpecNone, node->flags()));
+ SpecNone));
blessArrayOperation(node->child1(), node->child2(), node->child3());
fixEdge<CellUse>(node->child1());
diff --git a/Source/JavaScriptCore/dfg/DFGOperations.cpp b/Source/JavaScriptCore/dfg/DFGOperations.cpp
index ec8bc2f..64a4534 100644
--- a/Source/JavaScriptCore/dfg/DFGOperations.cpp
+++ b/Source/JavaScriptCore/dfg/DFGOperations.cpp
@@ -951,17 +951,6 @@
return reinterpret_cast<char*>(asObject(cell)->ensureContiguous(vm).data());
}
-char* JIT_OPERATION operationRageEnsureContiguous(ExecState* exec, JSCell* cell)
-{
- VM& vm = exec->vm();
- NativeCallFrameTracer tracer(&vm, exec);
-
- if (!cell->isObject())
- return 0;
-
- return reinterpret_cast<char*>(asObject(cell)->rageEnsureContiguous(vm).data());
-}
-
char* JIT_OPERATION operationEnsureArrayStorage(ExecState* exec, JSCell* cell)
{
VM& vm = exec->vm();
diff --git a/Source/JavaScriptCore/dfg/DFGOperations.h b/Source/JavaScriptCore/dfg/DFGOperations.h
index 03937bb..6b7441d 100644
--- a/Source/JavaScriptCore/dfg/DFGOperations.h
+++ b/Source/JavaScriptCore/dfg/DFGOperations.h
@@ -113,7 +113,6 @@
char* JIT_OPERATION operationEnsureInt32(ExecState*, JSCell*);
char* JIT_OPERATION operationEnsureDouble(ExecState*, JSCell*);
char* JIT_OPERATION operationEnsureContiguous(ExecState*, JSCell*);
-char* JIT_OPERATION operationRageEnsureContiguous(ExecState*, JSCell*);
char* JIT_OPERATION operationEnsureArrayStorage(ExecState*, JSCell*);
StringImpl* JIT_OPERATION operationResolveRope(ExecState*, JSString*);
JSString* JIT_OPERATION operationSingleCharacterString(ExecState*, int32_t);
diff --git a/Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp b/Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp
index c24a5a9..fdb8fda 100644
--- a/Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp
+++ b/Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp
@@ -391,7 +391,7 @@
m_graph, node,
node->child1()->prediction(),
node->child2()->prediction(),
- SpecNone, node->flags());
+ SpecNone);
switch (arrayMode.type()) {
case Array::Double:
diff --git a/Source/JavaScriptCore/dfg/DFGTypeCheckHoistingPhase.cpp b/Source/JavaScriptCore/dfg/DFGTypeCheckHoistingPhase.cpp
index 5cdeb61..3416edf 100644
--- a/Source/JavaScriptCore/dfg/DFGTypeCheckHoistingPhase.cpp
+++ b/Source/JavaScriptCore/dfg/DFGTypeCheckHoistingPhase.cpp
@@ -228,7 +228,9 @@
noticeStructureCheck(variable, node->structureSet());
break;
}
-
+
+ case ArrayifyToStructure:
+ case Arrayify:
case GetByOffset:
case PutByOffset:
case PutStructure:
@@ -250,22 +252,6 @@
// Don't count these uses.
break;
- case ArrayifyToStructure:
- case Arrayify:
- if (node->arrayMode().conversion() == Array::RageConvert) {
- // Rage conversion changes structures. We should avoid tying to do
- // any kind of hoisting when rage conversion is in play.
- Node* child = node->child1().node();
- if (child->op() != GetLocal)
- break;
- VariableAccessData* variable = child->variableAccessData();
- variable->vote(VoteOther);
- if (!shouldConsiderForHoisting<StructureTypeCheck>(variable))
- break;
- noticeStructureCheck(variable, 0);
- }
- break;
-
case SetLocal: {
// Find all uses of the source of the SetLocal. If any of them are a
// kind of CheckStructure, then we should notice them to ensure that
diff --git a/Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp b/Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp
index 18b8b72..d902ae7 100644
--- a/Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp
+++ b/Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp
@@ -1943,10 +1943,7 @@
vmCall(m_out.operation(operationEnsureDouble), m_callFrame, cell);
break;
case Array::Contiguous:
- if (m_node->arrayMode().conversion() == Array::RageConvert)
- vmCall(m_out.operation(operationRageEnsureContiguous), m_callFrame, cell);
- else
- vmCall(m_out.operation(operationEnsureContiguous), m_callFrame, cell);
+ vmCall(m_out.operation(operationEnsureContiguous), m_callFrame, cell);
break;
case Array::ArrayStorage:
case Array::SlowPutArrayStorage:
diff --git a/Source/JavaScriptCore/runtime/JSObject.cpp b/Source/JavaScriptCore/runtime/JSObject.cpp
index 1ed5b0f..b895cbe 100644
--- a/Source/JavaScriptCore/runtime/JSObject.cpp
+++ b/Source/JavaScriptCore/runtime/JSObject.cpp
@@ -836,8 +836,7 @@
return convertInt32ToArrayStorage(vm, structure(vm)->suggestedArrayStorageTransition());
}
-template<JSObject::DoubleToContiguousMode mode>
-ContiguousJSValues JSObject::genericConvertDoubleToContiguous(VM& vm)
+ContiguousJSValues JSObject::convertDoubleToContiguous(VM& vm)
{
ASSERT(hasDouble(indexingType()));
@@ -849,20 +848,7 @@
currentAsValue->clear();
continue;
}
- JSValue v;
- switch (mode) {
- case EncodeValueAsDouble:
- v = JSValue(JSValue::EncodeAsDouble, value);
- break;
- case RageConvertDoubleToValue:
- v = jsNumber(value);
- break;
- default:
- v = JSValue();
- RELEASE_ASSERT_NOT_REACHED();
- break;
- }
- ASSERT(v.isNumber());
+ JSValue v = JSValue(JSValue::EncodeAsDouble, value);
currentAsValue->setWithoutWriteBarrier(v);
}
@@ -870,16 +856,6 @@
return m_butterfly->contiguous();
}
-ContiguousJSValues JSObject::convertDoubleToContiguous(VM& vm)
-{
- return genericConvertDoubleToContiguous<EncodeValueAsDouble>(vm);
-}
-
-ContiguousJSValues JSObject::rageConvertDoubleToContiguous(VM& vm)
-{
- return genericConvertDoubleToContiguous<RageConvertDoubleToValue>(vm);
-}
-
ArrayStorage* JSObject::convertDoubleToArrayStorage(VM& vm, NonPropertyTransition transition)
{
DeferGC deferGC(vm.heap);
@@ -1049,7 +1025,7 @@
}
}
-ContiguousJSValues JSObject::ensureContiguousSlow(VM& vm, DoubleToContiguousMode mode)
+ContiguousJSValues JSObject::ensureContiguousSlow(VM& vm)
{
ASSERT(inherits(info()));
@@ -1066,8 +1042,6 @@
return convertInt32ToContiguous(vm);
case ALL_DOUBLE_INDEXING_TYPES:
- if (mode == RageConvertDoubleToValue)
- return rageConvertDoubleToContiguous(vm);
return convertDoubleToContiguous(vm);
case ALL_ARRAY_STORAGE_INDEXING_TYPES:
@@ -1079,16 +1053,6 @@
}
}
-ContiguousJSValues JSObject::ensureContiguousSlow(VM& vm)
-{
- return ensureContiguousSlow(vm, EncodeValueAsDouble);
-}
-
-ContiguousJSValues JSObject::rageEnsureContiguousSlow(VM& vm)
-{
- return ensureContiguousSlow(vm, RageConvertDoubleToValue);
-}
-
ArrayStorage* JSObject::ensureArrayStorageSlow(VM& vm)
{
ASSERT(inherits(info()));
diff --git a/Source/JavaScriptCore/runtime/JSObject.h b/Source/JavaScriptCore/runtime/JSObject.h
index d6282b07..d12cb32 100644
--- a/Source/JavaScriptCore/runtime/JSObject.h
+++ b/Source/JavaScriptCore/runtime/JSObject.h
@@ -689,18 +689,7 @@
return ensureContiguousSlow(vm);
}
-
- // Same as ensureContiguous(), except that if the indexed storage is in
- // double mode, then it does a rage conversion to contiguous: it
- // attempts to convert each double to an int32.
- ContiguousJSValues rageEnsureContiguous(VM& vm)
- {
- if (LIKELY(hasContiguous(indexingType())))
- return m_butterfly->contiguous();
-
- return rageEnsureContiguousSlow(vm);
- }
-
+
// Ensure that the object is in a mode where it has array storage. Use
// this if you're about to perform actions that would have required the
// object to be converted to have array storage, if it didn't have it
@@ -797,7 +786,6 @@
ArrayStorage* convertInt32ToArrayStorage(VM&);
ContiguousJSValues convertDoubleToContiguous(VM&);
- ContiguousJSValues rageConvertDoubleToContiguous(VM&);
ArrayStorage* convertDoubleToArrayStorage(VM&, NonPropertyTransition);
ArrayStorage* convertDoubleToArrayStorage(VM&);
@@ -983,14 +971,8 @@
ContiguousJSValues ensureInt32Slow(VM&);
ContiguousDoubles ensureDoubleSlow(VM&);
ContiguousJSValues ensureContiguousSlow(VM&);
- ContiguousJSValues rageEnsureContiguousSlow(VM&);
JS_EXPORT_PRIVATE ArrayStorage* ensureArrayStorageSlow(VM&);
-
- enum DoubleToContiguousMode { EncodeValueAsDouble, RageConvertDoubleToValue };
- template<DoubleToContiguousMode mode>
- ContiguousJSValues genericConvertDoubleToContiguous(VM&);
- ContiguousJSValues ensureContiguousSlow(VM&, DoubleToContiguousMode);
-
+
protected:
CopyWriteBarrier<Butterfly> m_butterfly;
#if USE(JSVALUE32_64)
