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

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has granted 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 393552: Patch

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




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

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

r=me with a few comments

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

I think this should be moved after the branch on "newStructure"

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:6076
> +		       if (identifier.isCell()) {

this should be an assert. Also, it should be an assert in the get_by_val code.
get_by_val is doing it wrong if it's not a cell, since we can't keep it alive.

> Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:1348
> +

nit: you could also do:
origin = origin.withInvalidExit()
here and then use that everywhere below instead of repeatedly calling it

> Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:1370
> +	   Node* clearValue = m_insertionSet.insertNode(indexInBlock, SpecNone,
JSConstant, origin, OpInfo(m_graph.freezeStrong(JSValue())));

shouldn't this also be withInvalidExit?

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

can we assert node->child1() useKind is KnownCellUse? Or can wee just emit an
edge with KnownCellUse directly to have the semantics be clearer?

Actually, looking at your code in mixup, it seems like we won't make this
KnownCellUse. So I think that's required for proper exit semantics/bookkeeping