Bug 97221 – Refactored the interpreter and JIT so they don't dictate closure layout

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Geoffrey Garen]

Refactored the interpreter and JIT so they don't dictate closure layout

[Geoffrey Garen]

Created attachment 164946 [details]
Patch

[Oliver Hunt]

Comment on attachment 164946 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:4223
> +            size_t baseOffset =
> +                (node.codeOrigin.inlineCallFrame
> +                  ? node.codeOrigin.inlineCallFrame->stackOffset
> +                  : 0) * sizeof(Register);

i think i'd prefer a plain if() here.

baseOffset = 0;
if (inlineFrame) baseOffset = thingy * sizeof(Register)

[Geoffrey Garen]

Created attachment 164953 [details]
Patch

[Geoffrey Garen]

> i think i'd prefer a plain if() here.

"* sizeof(Register)" is not optional, so I couldn't do exactly what you suggested, but I think you'll like what I came up with.

[Oliver Hunt]

Comment on attachment 164953 [details]
Patch

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

This seems okay to me, the only concern is the baseOffset stuff.  I'll r+ as it is, but personally i think i'd prefer it if you factored out CodeOrigin::baseOffset()

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:4221
> +            size_t baseOffset = node.codeOrigin.inlineCallFrame ? node.codeOrigin.inlineCallFrame->stackOffset : 0;
> +            baseOffset *= sizeof(Register);

This is nicer, but i still think the purely if based one works nicer (actual code now rather than bus typing logic):
size_t baseOffset = 0;
if (InlineCallFrame* inlineCallFrame = node.codeOrigin.inlineCallFrame)
    baseOffset = inlineCallFrame->stackOffset * sizeof(Register)

how does that feel?  I'm okay with your code as it stands, but personally i'd prefer the if() approach

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:4239
> +        size_t baseOffset = node.codeOrigin.inlineCallFrame ? node.codeOrigin.inlineCallFrame->stackOffset : 0;
> +        baseOffset = (baseOffset + CallFrame::argumentOffsetIncludingThis(0)) * sizeof(Register);

Ditto here -- although the repeated logic here makes me wonder if we actually want a baseOffset() function on CodeOrigin, then all this code would be:

size_t baseOffset = (code.codeOrigin.baseOffset() + thingyIfNecessary) * sizeof(Register)

how does that sound?

It would also encapsulate that production of baseOffset, so that if we decided to change the logic in future we would need to go around hunting all these sites down.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:4301
> +            size_t baseOffset = node.codeOrigin.inlineCallFrame ? node.codeOrigin.inlineCallFrame->stackOffset : 0;
> +            baseOffset *= sizeof(Register);

Yeah, all this duplicated code makes me think a CodeOrigin::baseOffset() function is the best approach.  Thoughts?

[Oliver Hunt]

Comment on attachment 164953 [details]
Patch

(- != + )

[Geoffrey Garen]

Committed r129156: <http://trac.webkit.org/changeset/129156>