Skip to content

Commit

Permalink
Merge r228720 - Don't mark an array profile out of bounds for the cas…
Browse files Browse the repository at this point in the history 
…es where the DFG will convert the access to SaneChain

https://bugs.webkit.org/show_bug.cgi?id=182912
<rdar://problem/37685083>

Reviewed by Keith Miller.

In the baseline JIT and LLInt, when we loading a hole from an original array,
with the array prototype chain being normal, we end up marking the ArrayProfile
for that GetByVal as out of bounds. However, the DFG knows exactly how to
optimize this case by returning undefined when loading from a hole. Currently,
it only does this for Contiguous arrays (and sometimes Double arrays).
This patch just makes sure to not mark the ArrayProfile as out of bounds
in this scenario for Contiguous arrays, since the DFG will always optimize
this case.

However, we should extend this by profiling when a GetByVal loads a hole. By
doing so, we can optimize this for Int32, ArrayStorage, and maybe even Double
arrays. That work will happen in:
https://bugs.webkit.org/show_bug.cgi?id=182940

This patch is a 30-50%  speedup on JetStream's hash-map test. This patch
speeds up JetStream by 1% when testing on my iMac.

* dfg/DFGArrayMode.cpp:
(JSC::DFG::ArrayMode::refine const):
* dfg/DFGFixupPhase.cpp:
(JSC::DFG::FixupPhase::fixupNode):
* jit/JITOperations.cpp:
(JSC::getByVal):
(JSC::canAccessArgumentIndexQuickly): Deleted.
* llint/LLIntSlowPaths.cpp:
(JSC::LLInt::getByVal):
(JSC::LLInt::LLINT_SLOW_PATH_DECL):
* llint/LowLevelInterpreter32_64.asm:
* llint/LowLevelInterpreter64.asm:
* runtime/CommonSlowPaths.h:
(JSC::CommonSlowPaths::canAccessArgumentIndexQuickly):
  • Loading branch information
@carlosgcampos
Saam Barati authored and carlosgcampos committed Feb 26, 2018
1 parent 9c04090 commit 9d00370
Show file tree
Hide file tree
Showing 8 changed files with 168 additions and 96 deletions.
40 changes: 40 additions & 0 deletions Source/JavaScriptCore/ChangeLog
View file
@@ -1,3 +1,43 @@
2018-02-19 Saam Barati <sbarati@apple.com>

Don't mark an array profile out of bounds for the cases where the DFG will convert the access to SaneChain
https://bugs.webkit.org/show_bug.cgi?id=182912
<rdar://problem/37685083>

Reviewed by Keith Miller.

In the baseline JIT and LLInt, when we loading a hole from an original array,
with the array prototype chain being normal, we end up marking the ArrayProfile
for that GetByVal as out of bounds. However, the DFG knows exactly how to
optimize this case by returning undefined when loading from a hole. Currently,
it only does this for Contiguous arrays (and sometimes Double arrays).
This patch just makes sure to not mark the ArrayProfile as out of bounds
in this scenario for Contiguous arrays, since the DFG will always optimize
this case.

However, we should extend this by profiling when a GetByVal loads a hole. By
doing so, we can optimize this for Int32, ArrayStorage, and maybe even Double
arrays. That work will happen in:
https://bugs.webkit.org/show_bug.cgi?id=182940

This patch is a 30-50% speedup on JetStream's hash-map test. This patch
speeds up JetStream by 1% when testing on my iMac.

* dfg/DFGArrayMode.cpp:
(JSC::DFG::ArrayMode::refine const):
* dfg/DFGFixupPhase.cpp:
(JSC::DFG::FixupPhase::fixupNode):
* jit/JITOperations.cpp:
(JSC::getByVal):
(JSC::canAccessArgumentIndexQuickly): Deleted.
* llint/LLIntSlowPaths.cpp:
(JSC::LLInt::getByVal):
(JSC::LLInt::LLINT_SLOW_PATH_DECL):
* llint/LowLevelInterpreter32_64.asm:
* llint/LowLevelInterpreter64.asm:
* runtime/CommonSlowPaths.h:
(JSC::CommonSlowPaths::canAccessArgumentIndexQuickly):

2018-02-17 Filip Pizlo <fpizlo@apple.com>

GetArrayMask should support constant folding
Expand Down
12 changes: 8 additions & 4 deletions Source/JavaScriptCore/dfg/DFGArrayMode.cpp
View file
Expand Up @@ -210,12 +210,16 @@ ArrayMode ArrayMode::refine(
// If we have an OriginalArray and the JSArray prototype chain is sane,
// any indexed access always return undefined. We have a fast path for that.
JSGlobalObject* globalObject = graph.globalObjectFor(node->origin.semantic);
Structure* arrayPrototypeStructure = globalObject->arrayPrototype()->structure();
Structure* objectPrototypeStructure = globalObject->objectPrototype()->structure();
if ((node->op() == GetByVal || canBecomeGetArrayLength(graph, node))
&& arrayClass() == Array::OriginalArray
&& globalObject->arrayPrototypeChainIsSane()
&& !graph.hasExitSite(node->origin.semantic, OutOfBounds)) {
graph.registerAndWatchStructureTransition(globalObject->arrayPrototype()->structure());
graph.registerAndWatchStructureTransition(globalObject->objectPrototype()->structure());
&& !graph.hasExitSite(node->origin.semantic, OutOfBounds)
&& arrayPrototypeStructure->transitionWatchpointSetIsStillValid()
&& objectPrototypeStructure->transitionWatchpointSetIsStillValid()
&& globalObject->arrayPrototypeChainIsSane()) {
graph.registerAndWatchStructureTransition(arrayPrototypeStructure);
graph.registerAndWatchStructureTransition(objectPrototypeStructure);
if (globalObject->arrayPrototypeChainIsSane())
return withSpeculation(Array::SaneChain);
}
Expand Down
93 changes: 48 additions & 45 deletions Source/JavaScriptCore/dfg/DFGFixupPhase.cpp
View file
Expand Up @@ -722,55 +722,58 @@ class FixupPhase : public Phase {
case Array::Double:
if (arrayMode.arrayClass() == Array::OriginalArray
&& arrayMode.speculation() == Array::InBounds) {
JSGlobalObject* globalObject = m_graph.globalObjectFor(node->origin.semantic);
if (globalObject->arrayPrototypeChainIsSane()) {
// Check if SaneChain will work on a per-type basis. Note that:
//
// 1) We don't want double arrays to sometimes return undefined, since
// that would require a change to the return type and it would pessimise
// things a lot. So, we'd only want to do that if we actually had
// evidence that we could read from a hole. That's pretty annoying.
// Likely the best way to handle that case is with an equivalent of
// SaneChain for OutOfBounds. For now we just detect when Undefined and
// NaN are indistinguishable according to backwards propagation, and just
// use SaneChain in that case. This happens to catch a lot of cases.
//
// 2) We don't want int32 array loads to have to do a hole check just to
// coerce to Undefined, since that would mean twice the checks.
//
// This has two implications. First, we have to do more checks than we'd
// like. It's unfortunate that we have to do the hole check. Second,
// some accesses that hit a hole will now need to take the full-blown
// out-of-bounds slow path. We can fix that with:
// https://bugs.webkit.org/show_bug.cgi?id=144668
// Check if SaneChain will work on a per-type basis. Note that:
//
// 1) We don't want double arrays to sometimes return undefined, since
// that would require a change to the return type and it would pessimise
// things a lot. So, we'd only want to do that if we actually had
// evidence that we could read from a hole. That's pretty annoying.
// Likely the best way to handle that case is with an equivalent of
// SaneChain for OutOfBounds. For now we just detect when Undefined and
// NaN are indistinguishable according to backwards propagation, and just
// use SaneChain in that case. This happens to catch a lot of cases.
//
// 2) We don't want int32 array loads to have to do a hole check just to
// coerce to Undefined, since that would mean twice the checks.
//
// This has two implications. First, we have to do more checks than we'd
// like. It's unfortunate that we have to do the hole check. Second,
// some accesses that hit a hole will now need to take the full-blown
// out-of-bounds slow path. We can fix that with:
// https://bugs.webkit.org/show_bug.cgi?id=144668

bool canDoSaneChain = false;
switch (arrayMode.type()) {
case Array::Contiguous:
// This is happens to be entirely natural. We already would have
// returned any JSValue, and now we'll return Undefined. We still do
// the check but it doesn't require taking any kind of slow path.
canDoSaneChain = true;
break;

bool canDoSaneChain = false;
switch (arrayMode.type()) {
case Array::Contiguous:
// This is happens to be entirely natural. We already would have
// returned any JSValue, and now we'll return Undefined. We still do
// the check but it doesn't require taking any kind of slow path.
case Array::Double:
if (!(node->flags() & NodeBytecodeUsesAsOther)) {
// Holes look like NaN already, so if the user doesn't care
// about the difference between Undefined and NaN then we can
// do this.
canDoSaneChain = true;
break;

case Array::Double:
if (!(node->flags() & NodeBytecodeUsesAsOther)) {
// Holes look like NaN already, so if the user doesn't care
// about the difference between Undefined and NaN then we can
// do this.
canDoSaneChain = true;
}
break;

default:
break;
}
break;

if (canDoSaneChain) {
m_graph.registerAndWatchStructureTransition(globalObject->arrayPrototype()->structure());
m_graph.registerAndWatchStructureTransition(globalObject->objectPrototype()->structure());
if (globalObject->arrayPrototypeChainIsSane())
node->setArrayMode(arrayMode.withSpeculation(Array::SaneChain));
default:
break;
}

if (canDoSaneChain) {
JSGlobalObject* globalObject = m_graph.globalObjectFor(node->origin.semantic);
Structure* arrayPrototypeStructure = globalObject->arrayPrototype()->structure();
Structure* objectPrototypeStructure = globalObject->objectPrototype()->structure();
if (arrayPrototypeStructure->transitionWatchpointSetIsStillValid()
&& objectPrototypeStructure->transitionWatchpointSetIsStillValid()
&& globalObject->arrayPrototypeChainIsSane()) {
m_graph.registerAndWatchStructureTransition(arrayPrototypeStructure);
m_graph.registerAndWatchStructureTransition(objectPrototypeStructure);
node->setArrayMode(arrayMode.withSpeculation(Array::SaneChain));
}
}
}
Expand Down
36 changes: 12 additions & 24 deletions Source/JavaScriptCore/jit/JITOperations.cpp
View file
Expand Up @@ -1724,27 +1724,6 @@ int32_t JIT_OPERATION operationInstanceOfCustom(ExecState* exec, EncodedJSValue

}

static bool canAccessArgumentIndexQuickly(JSObject& object, uint32_t index)
{
switch (object.structure()->typeInfo().type()) {
case DirectArgumentsType: {
DirectArguments* directArguments = jsCast<DirectArguments*>(&object);
if (directArguments->isMappedArgumentInDFG(index))
return true;
break;
}
case ScopedArgumentsType: {
ScopedArguments* scopedArguments = jsCast<ScopedArguments*>(&object);
if (scopedArguments->isMappedArgumentInDFG(index))
return true;
break;
}
default:
break;
}
return false;
}

static JSValue getByVal(ExecState* exec, JSValue baseValue, JSValue subscript, ByValInfo* byValInfo, ReturnAddressPtr returnAddress)
{
VM& vm = exec->vm();
Expand Down Expand Up @@ -1781,7 +1760,16 @@ static JSValue getByVal(ExecState* exec, JSValue baseValue, JSValue subscript, B
if (object->canGetIndexQuickly(i))
return object->getIndexQuickly(i);

if (!canAccessArgumentIndexQuickly(*object, i)) {
bool skipMarkingOutOfBounds = false;

if (object->indexingType() == ArrayWithContiguous && i < object->butterfly()->publicLength()) {
// FIXME: expand this to ArrayStorage, Int32, and maybe Double:
// https://bugs.webkit.org/show_bug.cgi?id=182940
auto* globalObject = object->globalObject();
skipMarkingOutOfBounds = globalObject->isOriginalArrayStructure(object->structure()) && globalObject->arrayPrototypeChainIsSane();
}

if (!skipMarkingOutOfBounds && !CommonSlowPaths::canAccessArgumentIndexQuickly(*object, i)) {
// FIXME: This will make us think that in-bounds typed array accesses are actually
// out-of-bounds.
// https://bugs.webkit.org/show_bug.cgi?id=149886
Expand Down Expand Up @@ -1950,7 +1938,7 @@ EncodedJSValue JIT_OPERATION operationHasIndexedPropertyDefault(ExecState* exec,
if (object->canGetIndexQuickly(index))
return JSValue::encode(JSValue(JSValue::JSTrue));

if (!canAccessArgumentIndexQuickly(*object, index)) {
if (!CommonSlowPaths::canAccessArgumentIndexQuickly(*object, index)) {
// FIXME: This will make us think that in-bounds typed array accesses are actually
// out-of-bounds.
// https://bugs.webkit.org/show_bug.cgi?id=149886
Expand All @@ -1974,7 +1962,7 @@ EncodedJSValue JIT_OPERATION operationHasIndexedPropertyGeneric(ExecState* exec,
if (object->canGetIndexQuickly(index))
return JSValue::encode(JSValue(JSValue::JSTrue));

if (!canAccessArgumentIndexQuickly(*object, index)) {
if (!CommonSlowPaths::canAccessArgumentIndexQuickly(*object, index)) {
// FIXME: This will make us think that in-bounds typed array accesses are actually
// out-of-bounds.
// https://bugs.webkit.org/show_bug.cgi?id=149886
Expand Down
32 changes: 27 additions & 5 deletions Source/JavaScriptCore/llint/LLIntSlowPaths.cpp
View file
Expand Up @@ -836,7 +836,7 @@ LLINT_SLOW_PATH_DECL(slow_path_del_by_id)
LLINT_RETURN(jsBoolean(couldDelete));
}

static ALWAYS_INLINE JSValue getByVal(VM& vm, ExecState* exec, JSValue baseValue, JSValue subscript)
static ALWAYS_INLINE JSValue getByVal(VM& vm, ExecState* exec, Instruction* pc, JSValue baseValue, JSValue subscript)
{
auto scope = DECLARE_THROW_SCOPE(vm);

Expand All @@ -852,10 +852,32 @@ static ALWAYS_INLINE JSValue getByVal(VM& vm, ExecState* exec, JSValue baseValue

if (subscript.isUInt32()) {
uint32_t i = subscript.asUInt32();
if (isJSString(baseValue) && asString(baseValue)->canGetIndex(i)) {
scope.release();
return asString(baseValue)->getIndex(exec, i);
ArrayProfile* arrayProfile = pc[4].u.arrayProfile;

if (isJSString(baseValue)) {
if (asString(baseValue)->canGetIndex(i)) {
scope.release();
return asString(baseValue)->getIndex(exec, i);
}
arrayProfile->setOutOfBounds();
} else if (baseValue.isObject()) {
JSObject* object = asObject(baseValue);
if (object->canGetIndexQuickly(i))
return object->getIndexQuickly(i);

bool skipMarkingOutOfBounds = false;

if (object->indexingType() == ArrayWithContiguous && i < object->butterfly()->publicLength()) {
// FIXME: expand this to ArrayStorage, Int32, and maybe Double:
// https://bugs.webkit.org/show_bug.cgi?id=182940
auto* globalObject = object->globalObject();
skipMarkingOutOfBounds = globalObject->isOriginalArrayStructure(object->structure()) && globalObject->arrayPrototypeChainIsSane();
}

if (!skipMarkingOutOfBounds && !CommonSlowPaths::canAccessArgumentIndexQuickly(*object, i))
arrayProfile->setOutOfBounds();
}

scope.release();
return baseValue.get(exec, i);
}
Expand All @@ -871,7 +893,7 @@ static ALWAYS_INLINE JSValue getByVal(VM& vm, ExecState* exec, JSValue baseValue
LLINT_SLOW_PATH_DECL(slow_path_get_by_val)
{
LLINT_BEGIN();
LLINT_RETURN_PROFILED(op_get_by_val, getByVal(vm, exec, LLINT_OP_C(2).jsValue(), LLINT_OP_C(3).jsValue()));
LLINT_RETURN_PROFILED(op_get_by_val, getByVal(vm, exec, pc, LLINT_OP_C(2).jsValue(), LLINT_OP_C(3).jsValue()));
}

LLINT_SLOW_PATH_DECL(slow_path_put_by_val)
Expand Down
11 changes: 4 additions & 7 deletions Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm
View file
Expand Up @@ -1583,15 +1583,15 @@ _llint_op_get_by_val:
bineq t2, ContiguousShape, .opGetByValNotContiguous

.opGetByValIsContiguous:
biaeq t1, -sizeof IndexingHeader + IndexingHeader::u.lengths.publicLength[t3], .opGetByValOutOfBounds
biaeq t1, -sizeof IndexingHeader + IndexingHeader::u.lengths.publicLength[t3], .opGetByValSlow
andi JSObject::m_butterflyIndexingMask[t0], t1
loadi TagOffset[t3, t1, 8], t2
loadi PayloadOffset[t3, t1, 8], t1
jmp .opGetByValDone

.opGetByValNotContiguous:
bineq t2, DoubleShape, .opGetByValNotDouble
biaeq t1, -sizeof IndexingHeader + IndexingHeader::u.lengths.publicLength[t3], .opGetByValOutOfBounds
biaeq t1, -sizeof IndexingHeader + IndexingHeader::u.lengths.publicLength[t3], .opGetByValSlow
andi JSObject::m_butterflyIndexingMask[t0], t1
loadd [t3, t1, 8], ft0
bdnequn ft0, ft0, .opGetByValSlow
Expand All @@ -1603,23 +1603,20 @@ _llint_op_get_by_val:
.opGetByValNotDouble:
subi ArrayStorageShape, t2
bia t2, SlowPutArrayStorageShape - ArrayStorageShape, .opGetByValSlow
biaeq t1, -sizeof IndexingHeader + IndexingHeader::u.lengths.vectorLength[t3], .opGetByValOutOfBounds
biaeq t1, -sizeof IndexingHeader + IndexingHeader::u.lengths.vectorLength[t3], .opGetByValSlow
andi JSObject::m_butterflyIndexingMask[t0], t1
loadi ArrayStorage::m_vector + TagOffset[t3, t1, 8], t2
loadi ArrayStorage::m_vector + PayloadOffset[t3, t1, 8], t1

.opGetByValDone:
loadi 4[PC], t0
bieq t2, EmptyValueTag, .opGetByValOutOfBounds
bieq t2, EmptyValueTag, .opGetByValSlow
.opGetByValNotEmpty:
storei t2, TagOffset[cfr, t0, 8]
storei t1, PayloadOffset[cfr, t0, 8]
valueProfile(t2, t1, 20, t0)
dispatch(constexpr op_get_by_val_length)

.opGetByValOutOfBounds:
loadpFromInstruction(4, t0)
storeb 1, ArrayProfile::m_outOfBounds[t0]
.opGetByValSlow:
callOpcodeSlowPath(_llint_slow_path_get_by_val)
dispatch(constexpr op_get_by_val_length)
Expand Down
17 changes: 6 additions & 11 deletions Source/JavaScriptCore/llint/LowLevelInterpreter64.asm
View file
Expand Up @@ -1514,43 +1514,38 @@ _llint_op_get_by_val:
bineq t2, ContiguousShape, .opGetByValNotContiguous

.opGetByValIsContiguous:
biaeq t1, -sizeof IndexingHeader + IndexingHeader::u.lengths.publicLength[t3], .opGetByValOutOfBounds
biaeq t1, -sizeof IndexingHeader + IndexingHeader::u.lengths.publicLength[t3], .opGetByValSlow
andi JSObject::m_butterflyIndexingMask[t0], t1
loadisFromInstruction(1, t0)
loadq [t3, t1, 8], t2
btqz t2, .opGetByValOutOfBounds
btqz t2, .opGetByValSlow
jmp .opGetByValDone

.opGetByValNotContiguous:
bineq t2, DoubleShape, .opGetByValNotDouble
biaeq t1, -sizeof IndexingHeader + IndexingHeader::u.lengths.publicLength[t3], .opGetByValOutOfBounds
biaeq t1, -sizeof IndexingHeader + IndexingHeader::u.lengths.publicLength[t3], .opGetByValSlow
andi JSObject::m_butterflyIndexingMask[t0], t1
loadisFromInstruction(1 ,t0)
loadd [t3, t1, 8], ft0
bdnequn ft0, ft0, .opGetByValOutOfBounds
bdnequn ft0, ft0, .opGetByValSlow
fd2q ft0, t2
subq tagTypeNumber, t2
jmp .opGetByValDone
.opGetByValNotDouble:
subi ArrayStorageShape, t2
bia t2, SlowPutArrayStorageShape - ArrayStorageShape, .opGetByValNotIndexedStorage
biaeq t1, -sizeof IndexingHeader + IndexingHeader::u.lengths.vectorLength[t3], .opGetByValOutOfBounds
biaeq t1, -sizeof IndexingHeader + IndexingHeader::u.lengths.vectorLength[t3], .opGetByValSlow
andi JSObject::m_butterflyIndexingMask[t0], t1
loadisFromInstruction(1, t0)
loadq ArrayStorage::m_vector[t3, t1, 8], t2
btqz t2, .opGetByValOutOfBounds
btqz t2, .opGetByValSlow

.opGetByValDone:
storeq t2, [cfr, t0, 8]
valueProfile(t2, 5, t0)
dispatch(constexpr op_get_by_val_length)

.opGetByValOutOfBounds:
loadpFromInstruction(4, t0)
storeb 1, ArrayProfile::m_outOfBounds[t0]
jmp .opGetByValSlow

.opGetByValNotIndexedStorage:
# First lets check if we even have a typed array. This lets us do some boilerplate up front.
loadb JSCell::m_type[t0], t2
Expand Down

0 comments on commit 9d00370

Please sign in to comment.