1221144 - Disentangle static and dynamic scope chains

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

I propose the following names for these two different things:

*   An "environment" is a runtime (dynamic) scope.
    This is what you capture when you make a closure.
    The ES6 standard calls these Lexical Environments.
    The Debugger API calls them Environments.

    We have code and comments that call these things "scopes".
    Let's phase that out.

*   A "scope" is the corresponding static concept.
    Scopes appear on the static scope chain.
    A syntactic scope corresponds 1-1 with some textual region of a program.

    For now, since "scope" is still ambiguous in our codebase,
    we'll call these "static scopes".

These two concepts are clearly related, but they should not share an object hierarchy. In fact, arguably static scopes shouldn't be JSObjects at all.

(Arguably environments shouldn't either, but the JITs are able to save some code duplication because they know environments are laid out just like other NativeObjects.)

Comment 1

[Jason Orendorff [:jorendorff]]

The terms "environment" and "scope" are due to Shu, but I don't know if he'd want to be associated with the idea of language-policing the entire codebase the way I am...

Comment 2

[Shu-yu Guo [:shu]]

(In reply to Jason Orendorff [:jorendorff] from comment #1)
> The terms "environment" and "scope" are due to Shu, but I don't know if he'd
> want to be associated with the idea of language-policing the entire codebase
> the way I am...

I am a descriptivist linguist by training. Luckily JS isn't a natural language.

Comment 3

[Jason Orendorff [:jorendorff]]

Attachment: Part 1: Make static scope objects a separate class hierarchy from the runtime ScopeObjects

Comment 4

[Jason Orendorff [:jorendorff]]

Attachment: Part 2: Rename static scope classes away from "ScopeObject"

Comment 5

[Jason Orendorff [:jorendorff]]

Attachment: Part 3: Rename variables, arguments, and fields that point to static scopes away from names that indicate objects, like "scopeObj" and "blockObj"

Comment 6

[Jason Orendorff [:jorendorff]]

Attachment: Part 4: Rename a few functions about scopes away from names that indicate objects, like js::CloneNestedScopeObject

Comment 7

[Jason Orendorff [:jorendorff]]

Attachment: Part 5: Delete class js::BlockObject

Comment 8

[Jason Orendorff [:jorendorff]]

Attachment: Part 6: Introduce StaticModuleScope. Pretty silly so far. Bindings are still stored in the script

Comment 9

[Jason Orendorff [:jorendorff]]

Attachment: Part 7: Introduce StaticFunctionScope, same story

Until now, Function.prototype had a null enclosing static scope, as a special case. Now we give it a real static scope, like all other interpreted functions.

New XDR tests cover a path in XDRLazyScript() that was previously untested.

Comment 10

[Jason Orendorff [:jorendorff]]

Attachment: Part 8: Change C++ type of static scopes everywhere from JSObject* to StaticScope*

Comment 11

[Jason Orendorff [:jorendorff]]

OK, what is the benefit of this stack of patches?

-   Put all static scopes in a single C++ class hierarchy, with fairly uniform representation
    (stop having ModuleObjects and JSFunctions on static scope chains; stop mixing static and
    dynamic scopes in a single class hierarchy)

-   Stop using JSObject* as the type of static scopes, a necessary step if we want static scopes
    not to be objects.

-   Eliminate a few odd special cases, like Function.prototype not having an enclosing scope;
    a nice comment on JSScript::staticScope_; some new XDR tests; blah blah.

It's not a ton. You tell me if it's worth doing.

There are a few more patches in this stack. I'll upload them today.

Comment 12

[Jason Orendorff [:jorendorff]]

Attachment: Part 9: A few more JSObject* -> StaticScope* changes

Comment 13

[Jason Orendorff [:jorendorff]]

Attachment: Part 10: Delete an obsolete comment

Comment 14

[Jason Orendorff [:jorendorff]]

Attachment: Part 11: Delete a few lines of redundant code in Parser::forStatement()

Comment 15

[Jason Orendorff [:jorendorff]]

Attachment: Part 12: Remove StaticScopeIter<>::IsStaticScope

JSObject::is<StaticScope>() now means the same thing.

We retain a few assertions that were using IsStaticScope(), even though the work in this bug makes it unlikely these assertions will ever fail again.

Comment 16

[Jason Orendorff [:jorendorff]]

Attachment: Part 13: Change DumpStaticScopeChain to include a function scope when available, on the theory that more information is better

Comment 17

[Jason Orendorff [:jorendorff]]

Attachment: Part 14: Handlify scope arguments to methods around FunctionBox creation

Comment 18

[Shu-yu Guo [:shu]]

Comment on attachment 8687183 [details] [diff] [review]
Part 1: Make static scope objects a separate class hierarchy from the runtime ScopeObjects

Review of attachment 8687183 [details] [diff] [review]:
-----------------------------------------------------------------

FYI: I didn't pore over the renaming lines; they looked reasonable to me.

::: js/src/vm/ScopeObject.h
@@ +153,5 @@
> +
> +  public:
> +    static StaticBlockObject* create(ExclusiveContext* cx);
> +
> +    /* The global lexical scope is extensible. Other block scopes aren't. */

Nit: while you're here, please mention that non-syntactic lexical scopes are also extensible.

@@ +385,5 @@
> + *   Global lexical scope
> + *       |
> + *   DynamicWithObject wrapping FakeBackstagePass
> + *       |
> + *   Non-syntactic lexical scope

Existing nit: change this to "ClonedBlockObject" too.

Comment 19

[Shu-yu Guo [:shu]]

Comment on attachment 8687190 [details] [diff] [review]
Part 3: Rename variables, arguments, and fields that point to static scopes away from names that indicate objects, like "scopeObj" and "blockObj"

Review of attachment 8687190 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/vm/ScopeObject.cpp
@@ +1222,5 @@
>                             Handle<NestedStaticScope*> srcBlock)
>  {
>      if (srcBlock->is<StaticBlockScope>()) {
> +        Rooted<StaticBlockScope*> blockScope(cx, &srcBlock->as<StaticBlockScope>());
> +        return CloneStaticBlockObject(cx, enclosingScope, blockScope);

The clone functions here and below are still referring to BlockObject and WithObject.

Comment 20

[Shu-yu Guo [:shu]]

Comment on attachment 8687192 [details] [diff] [review]
Part 4: Rename a few functions about scopes away from names that indicate objects, like js::CloneNestedScopeObject

Review of attachment 8687192 [details] [diff] [review]:
-----------------------------------------------------------------

Oh, I see you renamed the functions in a separate patch! Ignore my comments for the previous patch.

Comment 21

[Shu-yu Guo [:shu]]

Comment on attachment 8687197 [details] [diff] [review]
Part 6: Introduce StaticModuleScope. Pretty silly so far. Bindings are still stored in the script

Review of attachment 8687197 [details] [diff] [review]:
-----------------------------------------------------------------

I agree that as it stands now, StaticModuleScope is pretty silly and superfluous. The plan is to remove Bindings and move it into the scope hierarchy though, I imagine.

::: js/src/vm/ScopeObject.h
@@ +1548,5 @@
> +js::CallObject::callee() const
> +{
> +    MOZ_ASSERT(!is<ModuleEnvironmentObject>());
> +    return getFixedSlot(CALLEE_SLOT).toObject().as<JSFunction>();
> +}

Why were these moved down here? Organizational reasons?

Comment 22

[Shu-yu Guo [:shu]]

Comment on attachment 8687219 [details] [diff] [review]
Part 7: Introduce StaticFunctionScope, same story

Review of attachment 8687219 [details] [diff] [review]:
-----------------------------------------------------------------

I like this paving the way for unifying Bindings and other scopes. My only concern is that we are now creating an extra JSObject for each script. This might cause regressions, so be out the lookout for that.

::: js/src/jsscriptinlines.h
@@ +102,5 @@
>  
> +inline JSObject*
> +JSScript::enclosingStaticScope() const
> +{
> +    return function_ ? staticScope_->enclosingScope() : staticScope_;

I think this logic deserves a comment about how staticScope_ is the function scope when it is a function script. In a way that is obvious from the expression, but at first glance it reads kind of odd.

Comment 23

[Shu-yu Guo [:shu]]

Comment on attachment 8687220 [details] [diff] [review]
Part 8: Change C++ type of static scopes everywhere from JSObject* to StaticScope*

Review of attachment 8687220 [details] [diff] [review]:
-----------------------------------------------------------------

This is nice.

::: js/src/jsfriendapi.cpp
@@ +399,5 @@
>  
>      if (!iter.isFunctionFrame())
>          return nullptr;
>  
> +    // ??? we can't re-lazify scripts that are on the stack, right? that would be crazy

I don't think we relazify compartments that've been entered at all, which should rule out on-stack scripts.

Remove this comment?

::: js/src/jsscript.cpp
@@ +1020,5 @@
>              if (mode == XDR_DECODE) {
>                  if (enclosingStaticScopeIndex != UINT32_MAX) {
>                      MOZ_ASSERT(enclosingStaticScopeIndex < i);
> +                    enclosingStaticScope =
> +                        &script->objects()->vector[enclosingStaticScopeIndex]->as<StaticScope>();

Nit: this long line is formatted inconsistently compared to other long lines further down, which all use the "line up arrows and dots" style.

@@ +3639,5 @@
>  {
>      MOZ_ASSERT(fun->isInterpreted());
>  
> +    Rooted<StaticFunctionScope*> funScope(cx,
> +        StaticFunctionScope::create(cx, fun, enclosingScope));

Nit: this seems like it should fit on a single line

::: js/src/vm/Interpreter.cpp
@@ +1810,5 @@
>      ReservedRooted<Value> val(&rootValue0, REGS.sp[-1]);
>      REGS.sp--;
>      ReservedRooted<JSObject*> staticWith(&rootObject0, script->getObject(REGS.pc));
>  
> +    if (!EnterWithOperation(cx, REGS.fp(), val, HandleObject(staticWith).as<StaticWithScope>()))

What's up with HandleObject(staticWith) here? To avoid re-rooting a cast? I'm pretty sure Rooted's as<T>() already coerces to a Handle<T>, no?

Comment 24

[Shu-yu Guo [:shu]]

Comment on attachment 8687233 [details] [diff] [review]
Part 11: Delete a few lines of redundant code in Parser::forStatement()

Review of attachment 8687233 [details] [diff] [review]:
-----------------------------------------------------------------

What the hell. Someone's (mine?) merge error?

Comment 25

[Shu-yu Guo [:shu]]

To the general question "is this worth it?" I think the answer is "yes".

If from this point we can get towards the Scope-Environment nomenclature split in the engine in comment 0. And, insofar as this is foundational work for moving Bindings out of JSScript and into the various static scopes so there can be a unified mechanism for getting dynamic environments from static scopes, these patches are worth it.

My main worry, per comment 22, are regressions.

Comment 26

[Jeff Walden [:Waldo]]

(In reply to Shu-yu Guo [:shu] from comment #23)
> ::: js/src/vm/Interpreter.cpp
> @@ +1810,5 @@
> >      ReservedRooted<Value> val(&rootValue0, REGS.sp[-1]);
> >      REGS.sp--;
> >      ReservedRooted<JSObject*> staticWith(&rootObject0, script->getObject(REGS.pc));
> >  
> > +    if (!EnterWithOperation(cx, REGS.fp(), val, HandleObject(staticWith).as<StaticWithScope>()))
> 
> What's up with HandleObject(staticWith) here? To avoid re-rooting a cast?
> I'm pretty sure Rooted's as<T>() already coerces to a Handle<T>, no?

I'm guessing ReservedRooted doesn't have |template <typename U> as();| on it.  We should probably add ReservedRootedBase<JSObject*> handling beneath ReservedRootedBase<Value>, along the lines done by RootedBase<JSObject*> in js/public/RootingAPI.h, so this can be eliminated.  (Just as ReservedRooted<Value> has ValueOperations in it.)

Comment 27

[Jason Orendorff [:jorendorff]]

(In reply to Shu-yu Guo [:shu] from comment #18)
> > +    /* The global lexical scope is extensible. Other block scopes aren't. */
> 
> Nit: while you're here, please mention that non-syntactic lexical scopes are
> also extensible.

Can definitely mention that. But those aren't block scopes, though, right?

(pinging you for follow-up on IRC)

> > + *   Global lexical scope
> > + *       |
> > + *   DynamicWithObject wrapping FakeBackstagePass
> > + *       |
> > + *   Non-syntactic lexical scope
> 
> Existing nit: change this to "ClonedBlockObject" too.

Just the last part? Neither way is clear to me, so pinging you for follow-up on this too...

Comment 28

[Jason Orendorff [:jorendorff]]

(In reply to Shu-yu Guo [:shu] from comment #21)
> > +    MOZ_ASSERT(!is<ModuleEnvironmentObject>());
> 
> Why were these moved down here? Organizational reasons?

In the new patch, there's a template specialization for
JSObject::is<js::ModuleEnvironmentObject>().

I had to move that assertion to someplace after the specialization.

Comment 29

[Jason Orendorff [:jorendorff]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3958c5a88347

Comment 30

[Jason Orendorff [:jorendorff]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2644f15c3365

Comment 31

[Jason Orendorff [:jorendorff]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=69326459b71c

Comment 32

[Jason Orendorff [:jorendorff]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b884ca013361

Comment 33

[Jason Orendorff [:jorendorff]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=958918b03d88

Comment 34

[Jason Orendorff [:jorendorff]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=599560e39dd1

Comment 35

[Jason Orendorff [:jorendorff]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9d9fc906bf75

Comment 36

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2b37862c36f1497fc15ad6c65067fabea1cf136c
Bug 1221144 - Part 1: Make static scope objects a separate class hierarchy from the runtime ScopeObjects. r=shu.

https://hg.mozilla.org/integration/mozilla-inbound/rev/5e46f927faa732231a8d0f2addf6b91df789a412
Bug 1221144 - Part 2: Rename static scope classes away from "ScopeObject". r=shu.

https://hg.mozilla.org/integration/mozilla-inbound/rev/50c0af0c258ac591ec17b7a1d5f0b7382ca58a82
Bug 1221144 - Part 3: Rename variables, arguments, and fields that point to static scopes away from names that indicate objects, like "scopeObj" and "blockObj". r=shu.

https://hg.mozilla.org/integration/mozilla-inbound/rev/191cb0dcb35d21d1a4960253b2489c330dc0ce66
Bug 1221144 - Part 4: Rename a few functions about scopes away from names that indicate objects, like js::CloneNestedScopeObject. r=shu.

https://hg.mozilla.org/integration/mozilla-inbound/rev/728ed80f10650c1774ca12b5825c568f86b18372
Bug 1221144 - Part 5: Delete class js::BlockObject. r=shu.

Comment 37

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/2b37862c36f1
https://hg.mozilla.org/mozilla-central/rev/5e46f927faa7
https://hg.mozilla.org/mozilla-central/rev/50c0af0c258a
https://hg.mozilla.org/mozilla-central/rev/191cb0dcb35d
https://hg.mozilla.org/mozilla-central/rev/728ed80f1065

Comment 38

[Jason Orendorff [:jorendorff]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a47bc1ea4350

Comment 39

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e0aa1056e6bba377e7723fa63859cca3f845f6f0
Bug 1221144 - Part 6: Introduce StaticModuleScope. Pretty silly so far. Bindings are still stored in the script. r=shu.

https://hg.mozilla.org/integration/mozilla-inbound/rev/fe0f3776e83ec4ba8863af4dd32d8528259b0f46
Bug 1221144 - Part 7: Introduce StaticFunctionScope, same story. r=shu.

https://hg.mozilla.org/integration/mozilla-inbound/rev/7a9f939bf24daa47024d471dd29c7a9572754f1a
Bug 1221144 - Part 8: Change C++ type of static scopes everywhere from JSObject* to StaticScope*. r=shu.

https://hg.mozilla.org/integration/mozilla-inbound/rev/731746468ebe75d67950049918878678be39d6e8
Bug 1221144 - Part 9: A few more JSObject* -> StaticScope* changes. r=shu.

https://hg.mozilla.org/integration/mozilla-inbound/rev/90ea0e5db89f1b5b283d9d29bcf0110dd8a6c413
Bug 1221144 - Part 10: Delete an obsolete comment. r=shu.

https://hg.mozilla.org/integration/mozilla-inbound/rev/c0f21d3e4eb5bea3b882e53c14a8ec9165b69673
Bug 1221144 - Part 12: Remove StaticScopeIter<>::IsStaticScope. r=shu.

https://hg.mozilla.org/integration/mozilla-inbound/rev/4033253f5d7fc44668ede75ba945e492b6ac316a
Bug 1221144 - Part 13: Change DumpStaticScopeChain to include a function scope when available, on the theory that more information is better. r=shu.

https://hg.mozilla.org/integration/mozilla-inbound/rev/0579589f4afc564e23c274af7750cd10c724c342
Bug 1221144 - Part 14: Handlify scope arguments to methods around FunctionBox creation. r=shu.

Comment 40

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dd847049b535544869f3d9f8fb0e29292d3a76c6
Fix busted test checked in with bug 1221144. r=red.

Comment 41

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e0aa1056e6bb
https://hg.mozilla.org/mozilla-central/rev/fe0f3776e83e
https://hg.mozilla.org/mozilla-central/rev/7a9f939bf24d
https://hg.mozilla.org/mozilla-central/rev/731746468ebe
https://hg.mozilla.org/mozilla-central/rev/90ea0e5db89f
https://hg.mozilla.org/mozilla-central/rev/c0f21d3e4eb5
https://hg.mozilla.org/mozilla-central/rev/4033253f5d7f
https://hg.mozilla.org/mozilla-central/rev/0579589f4afc
https://hg.mozilla.org/mozilla-central/rev/dd847049b535

Comment 42

[Joel Maher ( :jmaher ) (UTC -8)]

backed out for failure to address perf issue in bug 1246396:
https://hg.mozilla.org/integration/mozilla-inbound/rev/966f47ed2f25

Comment 43

[Eric Rahm [:erahm]]

This also seems to have caused a regression in JS memory usage on AWSY which cleared with the backout.

Comment 44

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:jorendorff, maybe it's time to close this bug?