[webkit-reviews] review denied: [Bug 208664] Allow deleteById to be cached in the DFG : [Attachment 393428] Patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has denied Justin Michaud
<justin_michaud at apple.com>'s request for review:
Bug 208664: Allow deleteById to be cached in the DFG
https://bugs.webkit.org/show_bug.cgi?id=208664

Attachment 393428: Patch

https://bugs.webkit.org/attachment.cgi?id=393428&action=review




--- Comment #7 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 393428
  --> https://bugs.webkit.org/attachment.cgi?id=393428
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=393428&action=review

Mostly LGTM, but I think I found a couple legitimate bugs

> Source/JavaScriptCore/ChangeLog:10
> +	   creating a new node, FilterDeleteByStatus, and then turning these
DeleteById nodes into a FilterDeleteByStatus,
> +	   CheckStructure, PutByOffset, then PutStructure. The logic for
pessimising this optimization is the same as for 

or just CheckStructure in the case of a miss delete, right?

> Source/JavaScriptCore/ChangeLog:14
> +	   This also adds a MultiDeleteByOffset node, for the case when there
are multiple structures seen by the delete.

If they're all misses, do you still just emit a CheckStructure?

> Source/JavaScriptCore/bytecode/DeleteByStatus.cpp:112
> +	       if (access.viaProxy())
> +		   return DeleteByStatus(JSC::slowVersion(summary), *stubInfo);

does this work currently in the IC? Like deleting via window proxy?

> Source/JavaScriptCore/bytecode/DeleteByStatus.cpp:115
> +	       if (access.usesPolyProto())
> +		   return DeleteByStatus(JSC::slowVersion(summary), *stubInfo);

is this even possible? Delete doesn't do anything on the prototype chain. Seems
like it should be an assert or just drop the line entirely

> Source/JavaScriptCore/bytecode/DeleteByStatus.cpp:131
> +		   ASSERT_UNUSED(offset, offset == access.offset());

how is this correct if we fail to find the transition?

> Source/JavaScriptCore/bytecode/DeleteByStatus.h:85
> +    bool finalize(VM&) { return true; }

this looks super wrong :-)

What if your variants structures aren't marked?

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4651
> +	   for (const DeleteByIdVariant& variant : deleteByStatus.variants()) {
> +	       m_graph.registerStructure(variant.oldStructure());
> +	       if (variant.newStructure())
> +		   m_graph.registerStructure(variant.newStructure());
> +	   }
> +
> +	   MultiDeleteByOffsetData* data =
m_graph.m_multiDeleteByOffsetData.add();
> +	   data->variants = deleteByStatus.variants();
> +	   data->identifierNumber = identifierNumber;
> +	   set(destination,
> +	       addToGraph(MultiDeleteByOffset, OpInfo(data), base));

if all of these are delete misses (and all either the non-configurable or the
configurable kind), can we just turn this into a CheckStructure followed by
"true" or "false" ? Seems like that's more efficient for other analyses

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:6030
> +	       handleDeleteById(bytecode.m_dst, base, identifierNumber,
deleteByStatus);

what about delete by val when the identifier is monotonic? If you don't do that
in this patch, please file a bug to do it. (You can see what we do for
get_by_val)

> Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:1345
> +	   addBaseCheck(indexInBlock, node, baseValue,
m_graph.registerStructure(variant.oldStructure()));

this seems to indicate we must be exitOK to begin with

> Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:1350
> +	       node->origin.exitOK = false;

nit: we usually write this as:
node->origin = node->origin.withInvalidExit()

But, that said, why has our origin changed at all? Seems like we should just
maintain if we're safe or not safe to exit, right?

> Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:1373
> +	   m_insertionSet.insertNode(
> +	       indexInBlock, SpecNone, PutByOffset, origin.withInvalidExit(),
OpInfo(&data), propertyStorage, node->child1(), Edge(clearValue));

This seems like it should maintain the same origin

> Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:1376
> +	   m_insertionSet.insertNode(
> +	       indexInBlock, SpecNone, PutStructure, origin.withInvalidExit(),
OpInfo(transition),
> +	       node->child1());

and this seems like it should be exit invalid (as you have), since the store
above invalidates exit state if we were safe to exit

> Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:1378
> +	   node->origin.exitOK = false;

nit: should be origin = origin.withInvalidExit()

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:8132
> +	   int missConfigurable = 0;
> +	   int missNonconfigurable = 0;

size_t or unsigned

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:8134
> +	   for (unsigned i = data.variants.size(); i--;) {

size_t or unsigned

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:8144
> +	   int uniqueCaseCount = data.variants.size();

size_t or unsigned

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:8150
> +	   int trueBlock = missConfigurable ? uniqueCaseCount - 1 : -1;
> +	   int falseBlock = missNonconfigurable ? uniqueCaseCount - 1 -
!!missConfigurable : -1;

nit: Optional<size_t> or Optional<unsigned>

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:8200
> +	       results.append(m_out.anchor(variant.result() ? m_out.intPtrOne :
m_out.intPtrZero));

This is wrong for booleans. We expect them to be i32. I wonder why nothing is
actually crashing because of this in validation.

Anyways, you want m_out.booleanTrue and m_out.booleanFalse

If you do some math on your boolean result in js, I bet you trigger some
validation errors

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:8207
> +	       results.append(m_out.anchor(m_out.intPtrZero));

ditto

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:8213
> +	       results.append(m_out.anchor(m_out.intPtrOne));

ditto

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:8223
> +	   setBoolean(m_out.phi(Int64, results));

and this should be Int32