[webkit-reviews] review granted: [Bug 111497] Bring back eager resolution of function scoped variables : [Attachment 191809] Patch

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has granted Oliver Hunt <oliver at apple.com>'s
request for review:
Bug 111497: Bring back eager resolution of function scoped variables
https://bugs.webkit.org/show_bug.cgi?id=111497

Attachment 191809: Patch
https://bugs.webkit.org/attachment.cgi?id=191809&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
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.