Bug 97221

Summary: Refactored the interpreter and JIT so they don't dictate closure layout
Product: WebKit Reporter: Geoffrey Garen <ggaren>
Component: New BugsAssignee: Geoffrey Garen <ggaren>
Status: RESOLVED FIXED    
Severity: Normal CC: barraclough, fpizlo, oliver
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch oliver: review+

Description Geoffrey Garen 2012-09-20 08:20:32 PDT
Refactored the interpreter and JIT so they don't dictate closure layout
Comment 1 Geoffrey Garen 2012-09-20 10:59:46 PDT
Created attachment 164946 [details]
Patch
Comment 2 Oliver Hunt 2012-09-20 11:09:01 PDT
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)
Comment 3 Geoffrey Garen 2012-09-20 11:20:02 PDT
Created attachment 164953 [details]
Patch
Comment 4 Geoffrey Garen 2012-09-20 11:20:46 PDT
> 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.
Comment 5 Oliver Hunt 2012-09-20 11:32:25 PDT
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?
Comment 6 Oliver Hunt 2012-09-20 11:32:40 PDT
Comment on attachment 164953 [details]
Patch

(- != + )
Comment 7 Geoffrey Garen 2012-09-20 12:51:25 PDT
Committed r129156: <http://trac.webkit.org/changeset/129156>