Bug 74633

Created attachment 119482 [details] Patch

What do we need this for?

(In reply to comment #2) > What do we need this for? It will be used in a later patch that implements block scoping.

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.

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.

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

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

<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.

Created attachment 122959 [details] Patch

Comment on attachment 122959 [details] Patch Please see my prior comment.

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