Bug 74633 – Allow JSStaticScope to bind more than one variable

• Status: RESOLVED
• Resolution: INVALID
• Component: JavaScriptCore

[Andy Wingo]

The patch to be attached generalizes JSStaticScope to be able to bind more than one variable.  As the symbol table could grow large, instead of copying it all the time, we refcount it.  A "template" JSStaticScope can beget other scope objects that share the symbol table, but have their own storage.  It also changes the push_new_scope op to take a template scope as an argument instead of an identifier.  Eventually we will remove the "value" arg from push_new_scope, and instead bind the variable with an op_put_scoped_var.

[Andy Wingo]

Created attachment 119482 [details]
Patch

[Oliver Hunt]

What do we need this for?

[Andy Wingo]

(In reply to comment #2)
> What do we need this for?

It will be used in a later patch that implements block scoping.

[Gavin Barraclough]

Comment on attachment 119482 [details]
Patch

So, we're really keen to avoid unnecessary complexity introduced by premature optimizations, and I think this patch is probably not truly necessary at this stage.  I think it would be perfectly possible to implement block scoped const by pushing multiple consts each with their own scope.  Clearly letting them share a scope object is more efficient, but it seems this is just an optimization.  As such it would have been much better to implement the feature first, then the optimization for it, rather than implementing an optimization for a feature that doesn't just exist, and as such we can't assess the value of!

I think it's likely enough that this will be a benefit in the long run that we can let this slide this time, so I won't make you restructure your set of changes to land this after block scoped const, but for future reference we really do want changes like this to be justified empirically.

[Gavin Barraclough]

Comment on attachment 119482 [details]
Patch

Hmmm, retracting my r+ for now based on Geoff's comments in https://bugs.webkit.org/show_bug.cgi?id=74718 .  If we don't use the scope chain for const, this change won't make sense.

[Andy Wingo]

Gavin, do you still think that this patch should not go in, after the discussion in bug 74718?  I'm uncertain, as Geoff is arguing for more efficiency, but you seem to think this is a premature optimization.

It seems to me that single-var JSStaticScope is not the right thing, because `const' and `let' form mutually recursive bindings:

  {
     const f = function() { return g; }
     const g = Foo();
     return f;
   }

Thanks,

Andy

[Gavin Barraclough]

Hi Andy,

Ah, good point – you're right we will need a solution based around a scope supporting multiple variables.

However I think Geoff has a good point about eager vs lazy tear-off, an that it really might be worth a quick investigation before moving forwards.  It may be much better to switch the const/let mechanism to be based on create_activation rather than push_new_scope, and since doing so may make a number of these changes redundant it could be a good idea to at least look into how hard this would be before going ahead with the push_new_scope based solution.

Lazy tear-off of activations offers two key performance advantages over static scopes.  In some cases the variables that might be captured may be accessed frequently within the function they are declared in and benefit from fast access to local virtual registers.  Further, we usually try to be lazy about allocating activation objects, only doing so if a function expression is actually reached (it is not atypical for a program to contain functions that can and sometimes do create closures, but upon many invocations do not).

There may be further advantages to using activations.  Activations already support multiple variables, and if generalized to be able to be used at block scope as well as function scope may allow us to remove push_new_scope, reducing complexity.  Considering let/const variables at function scope, it would be inefficient to create separate objects on the scope chain for vars and const/let variables, placing const/let values on the activation would allow us to use the same object.  And a key optimization for both vars and let/const values will be escape/capture analysis.  This mechanism will want to operate over all types of variables, and having 

static

[Gavin Barraclough]

<Gah, accidental commit!>

... , and having differing mechanisms underlying const/let/var may complicate implementation (e.g. activation based lazy tear-off results in eager virtual register allocation, use of static scopes does not – in the case of activation, if capture analysis shows a variable cannot be captured the only change in code generation might be to omit the create-activation/tear-off checks, at the points variables are accessed there would be no change in the byte code emitted).

In the case of optimizations to const/let that are additive to the basic mechanism, there is no need for these to be considered as part of the initial implementation (indeed I'd argue it's best not to – to land the code in its simplest functional, correct form, and to optimize incrementally from there showing the benefit of each change).  But we don't want to land large changed that would only be torn out weeks later, so I think the question of whether the const/let implementation should be based on static scope objects or activations is a fundamental question that really ought to be addressed before proceeding.

What do you think?

cheers,
G.

[Andy Wingo]

Created attachment 122959 [details]
Patch

[Gavin Barraclough]

Comment on attachment 122959 [details]
Patch

Please see my prior comment.

[Andy Wingo]

Resolving as invalid; the parts that were good (?) live on in bug 74708.