Bug 111497 – Bring back eager resolution of function scoped variables

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

[Oliver Hunt]

Bring back eager resolution of function scoped variables

[Oliver Hunt]

Created attachment 191603 [details]
Patch

[Oliver Hunt]

Created attachment 191604 [details]
Patch

[Build Bot]

Comment on attachment 191604 [details]
Patch

Attachment 191604 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17079093

New failing tests:
fast/js/dfg-int32-to-double-on-known-number.html
fast/workers/shared-worker-frame-lifecycle.html
fast/js/dfg-cse-cfa-discrepancy.html
fast/frames/take-focus-from-iframe.html
fast/workers/dedicated-worker-lifecycle.html
fast/js/dfg-obvious-constant-cfa.html
fast/js/regress/inline-get-scoped-var.html
jquery/attributes.html

[Oliver Hunt]

(In reply to comment #3)
> (From update of attachment 191604 [details])
> Attachment 191604 [details] did not pass mac-wk2-ews (mac-wk2):
> Output: http://webkit-commit-queue.appspot.com/results/17079093
> 
> New failing tests:
> fast/js/dfg-int32-to-double-on-known-number.html
> fast/workers/shared-worker-frame-lifecycle.html
> fast/js/dfg-cse-cfa-discrepancy.html
> fast/frames/take-focus-from-iframe.html
> fast/workers/dedicated-worker-lifecycle.html
> fast/js/dfg-obvious-constant-cfa.html
> fast/js/regress/inline-get-scoped-var.html
> jquery/attributes.html

wk2 is a 64bit bot right?

[Build Bot]

Comment on attachment 191604 [details]
Patch

Attachment 191604 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17066104

New failing tests:
fast/js/dfg-int32-to-double-on-known-number.html
fast/workers/shared-worker-frame-lifecycle.html
fast/js/dfg-cse-cfa-discrepancy.html
jquery/attributes.html
fast/js/dfg-obvious-constant-cfa.html
fast/js/regress/inline-get-scoped-var.html
http/tests/workers/terminate-during-sync-operation.html
fast/workers/dedicated-worker-lifecycle.html

[Oliver Hunt]

Comment on attachment 191604 [details]
Patch

r-'ing until i work out wtf is happening to the mac bots

[Oliver Hunt]

Created attachment 191809 [details]
Patch

[Geoffrey Garen]

Comment on attachment 191809 [details]
Patch

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

What's the performance result here?

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:1944
> +#if ENABLE(DFG_JIT)

I believe this should be ENABLE(VALUE_PROFILING).

> Source/JavaScriptCore/bytecode/CodeBlock.h:1158
> +        bool m_needsFullScopeChain;

Let's call this m_needsActivation.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:241
> +    , m_globalObjectRegister(0)

Can we use -1 as the default value in the bytecode generator, to match the code block?

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1280
> +    size_t depthOfFirstScopeWithDynamicChecks = 0;

I believe this value is wrong if the current scope needsActivation(), since there isn't any code that adds 1 in that case.

Actually, let's just remove this, since it's unused.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1294
> +            depth += m_codeBlock->needsFullScopeChain();

I think it would be clearer to initialize 'depth' to 0 or 1 based on needsActivation(), rather than adding 1 in at the end. That way, depth is always consistent with the depth you'll see at runtime.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1296
> +                // Global scope

This comment would be better if it explained why global scope forces a dynamic resolve. I'm not sure, from reading this, why it should.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1307
> +            break;

I think this would be clearer as an early  return, like you used in other places in the function.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1310
> +        if (!(flags & ResolveResult::DynamicFlag)) {
> +            if (scopeRequiresDynamicChecks)
> +                flags |= ResolveResult::DynamicFlag;

Can this |= just be unconditional? I think that would be clearer.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1375
> +        // Global object is the base

I don't think this comment adds anything over the code below it.

[Oliver Hunt]

Committed r145000: <http://trac.webkit.org/changeset/145000>