Bug 74718 – Remove the `value' argument to op_push_new_scope

• Component: JavaScriptCore
• Status: RESOLVED INVALID

[Andy Wingo]

Quoth the changelog:

        For exception scopes, instead of pushing a scope with a value
        already set in it, push a fresh scope and use op_put_scoped_var to
        bind the var.  This will allow op_push_new_scope to be used for
        multi-variable scopes, like ES6 block scopes.

[Andy Wingo]

Created attachment 119611 [details]
Patch

[Andy Wingo]

Depends on earlier work.  Next patch finally adds block scope, though without temporal dead zones.

[Andy Wingo]

Created attachment 119620 [details]
Patch

[Andy Wingo]

New patch introduces a new helper, BytecodeGenerator::emitInitializeBlockScopedLocal.

[Geoffrey Garen]

It's not sound to use the same mechanism for block scope as we currently use for catch. Catch is rare, and already slow, so it's OK that catch requires an object allocation and scope chain push/pop. But if "let is the new var", it's not acceptable to allocate a new scope object and push/pop the head of the scope chain every time you enter/exit a let scope. We want a new solution that distinguishes between variables that are captured and variables that aren't, allocates uncultured variables on the stack, and tears of captured variables as needed.

[Gavin Barraclough]

(In reply to comment #5)
> It's not sound to use the same mechanism for block scope as we currently use for catch. Catch is rare, and already slow, so it's OK that catch requires an object allocation and scope chain push/pop. But if "let is the new var", it's not acceptable to allocate a new scope object and push/pop the head of the scope chain every time you enter/exit a let scope. We want a new solution that distinguishes between variables that are captured and variables that aren't, allocates uncultured variables on the stack, and tears of captured variables as needed.

Hey Geoff,

Actually I think Andy's approach may be the right one here.  For scopes that contain captured let/const variables we're going to potentially need multiple copies of the value, each with share-by-reference semantics between a set of closures (basically the same as catch, which is by intention).  Clearly for the common case where values are not captured we will want something that is more optimal, but I think it makes sense to develop the correct and compete form first, and then optimize.

By landing a scope based mechanism first, we can get this checked in (disabled by default, but being switched on in some cases by DRT to test it) with layout tests checking for correct behavior – and then add the complexity of a mechanism to optimize let/const values that are not captured to regular locals (also perhaps merging nested block scopes where safe - particularly it may make sense to merge outer scopes into the activation).

Having a functional (but not yet optimal) implementation for const will also allow us to answer another question, which is that we'd like to be able to test enabling this in place of the existing const implementation would be compatibility with the web (if so we'll only need to support one set of semantics going forwards).

What are your thoughts? - we could implement this the other way around - we could start out by treating let/const as a block-scoped version of locals with var-like capture semantics, and then bolt on a scope-based mechanism to handle correct capture semantics, but that seems less desirable to me.

G.

[Geoffrey Garen]

Andy, I'm really worried that you'll have to redo most of this code once you start considering performance issues -- eager vs lazy tear-off, optimizing out uncaptured lets, merging captured lets, etc. 

That said, Gavin, I guess I don't want to let my worry stand in the way of a first-cut implementation, if you think the approach is sound. 

> 1944            generator.emitPushBlockScope(generator.newTemporary(), scopeTemplate);
> 1945            generator.emitInitializeBlockScopedLocal(m_exceptionIdent, exceptionRegister.get());

Building on the refactoring I suggested in bug 74708, I think this would be clearer as:

emitPushBlockScope(...)
ResolveResult result = resolve(...)
emitPutScopedVar(result, m_exceptionIdent, exceptionRegister.get())

Then you can get rid of emitInitializeBlockScopedLocal(). As long as you're not optimizing for let yet, you should avoid, as much as possible, building let-specific functionality into the bytecompiler.

[Gavin Barraclough]

Actually I think you have a really good point about eager vs lazy tear-off – I hadn't considered that.  I think the other issues can be treated as optimizations to be added, but we should probably make tear-off lazy from the start.

[Andy Wingo]

(In reply to comment #7)
> Andy, I'm really worried that you'll have to redo most of this code once you start considering performance issues -- eager vs lazy tear-off, optimizing out uncaptured lets, merging captured lets, etc. 

I agree that there are many places where optimizations can be made.  The read barrier will also complicate things.  At some point it might make sense to implement display closures, too.  Still, though, the mechanism is similar enough to `catch' (modulo read barriers) that it does seem like a path to a correct implementation, that can be incrementally turned into a fast implementation in many common cases.

Using JSStaticScope also has the advantage that the code can be tested, even in the absence of ENABLE(HARMONY) or ENABLE(ES6_BLOCK_SCOPE) or whatever.

If you or Gavin gets a chance, I would appreciate a few words on lazy tear-off; it is not something that I ran into yet.

> emitPushBlockScope(...)
> ResolveResult result = resolve(...)
> emitPutScopedVar(result, m_exceptionIdent, exceptionRegister.get())

Will do.

> Then you can get rid of emitInitializeBlockScopedLocal(). As long as you're not optimizing for let yet, you should avoid, as much as possible, building let-specific functionality into the bytecompiler.

Sure.  Note that in ES6, const also has block scope.  And, the catch-bound var and the name of a named function expression also have block scope.  But I like your suggested refactor anyway.

Regards,

Andy

[Andy Wingo]

Created attachment 122961 [details]
Patch

[Andy Wingo]

Marking as invalid, given that bug 74708 seems to be subsuming these micro-changes.