[Webkit-unassigned] [Bug 175738] Make GetDynamicVar propagate heap predictions instead of saying HeapTop

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=175738

--- Comment #4 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 318557
  --> https://bugs.webkit.org/attachment.cgi?id=318557
Patch, not yet benchmarked, nits fixed

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

The patch LGTM but I’ll hold off r+ing until it builds.

The one thing word adding is a micro benchmark just so you can verify it’s indeed faster.

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4067
> +// Both GetDynamicVar and PutDynamicVar need to cram two 32-bit numbers into their

Style nit: This comment is unnecessary IMO. It’s not adding anything that’s not obvious from reading the code.

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4071
> +    static_assert(sizeof(identifierNumber) == 4,

FWIW, this is probably a hard assumption when compiling WK. I’d bet many thing would break if this didn’t hold.

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4073
> +    return identifierNumber | (static_cast<uint64_t>(getPutInfo) << 32);

That’s tricky! I guess Node::identifierNumber() just works?

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:5373
> +                uint64_t opInfo1 = makeDynamicVarOpInfo(identifierNumber, currentInstruction[4].u.operand);

Style nit: not sure either of these really need to be local variables

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20170821/6b0fbc89/attachment.html>