Closed Bug 1221144 Opened 5 years ago Closed 1 year ago

Disentangle static and dynamic scope chains

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED WONTFIX
Tracking Status
firefox45 --- affected

People

(Reporter: jorendorff, Assigned: jorendorff)

References

Details

Attachments

(14 files)

101.09 KB, patch
shu
: review+
Details | Diff | Splinter Review
163.48 KB, patch
shu
: review+
Details | Diff | Splinter Review
52.13 KB, patch
shu
: review+
Details | Diff | Splinter Review
5.79 KB, patch
shu
: review+
Details | Diff | Splinter Review
16.54 KB, patch
shu
: review+
Details | Diff | Splinter Review
27.49 KB, patch
shu
: review+
Details | Diff | Splinter Review
77.88 KB, patch
shu
: review+
Details | Diff | Splinter Review
109.46 KB, patch
shu
: review+
Details | Diff | Splinter Review
6.49 KB, patch
shu
: review+
Details | Diff | Splinter Review
1.37 KB, patch
shu
: review+
Details | Diff | Splinter Review
1.35 KB, patch
shu
: review+
Details | Diff | Splinter Review
3.33 KB, patch
shu
: review+
Details | Diff | Splinter Review
1.69 KB, patch
shu
: review+
Details | Diff | Splinter Review
4.25 KB, patch
shu
: review+
Details | Diff | Splinter Review
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.)
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...
(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.
Assignee: nobody → jorendorff
Status: NEW → ASSIGNED
Attachment #8687183 - Attachment is obsolete: true
Attachment #8687183 - Flags: review?(shu)
Attachment #8687183 - Attachment is obsolete: false
Attachment #8687183 - Flags: review?(shu)
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.
Attachment #8687219 - Flags: review?(shu)
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.
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.
Attachment #8687234 - Flags: review?(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.
Attachment #8687183 - Flags: review?(shu) → review+
Attachment #8687187 - Flags: review?(shu) → review+
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.
Attachment #8687190 - Flags: review?(shu) → review+
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.
Attachment #8687192 - Flags: review?(shu) → review+
Attachment #8687194 - Flags: review?(shu) → review+
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?
Attachment #8687197 - Flags: review?(shu) → review+
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.
Attachment #8687219 - Flags: review?(shu) → review+
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?
Attachment #8687220 - Flags: review?(shu) → review+
Attachment #8687229 - Flags: review?(shu) → review+
Attachment #8687231 - Flags: review?(shu) → review+
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?
Attachment #8687233 - Flags: review?(shu) → review+
Attachment #8687234 - Flags: review?(shu) → review+
Attachment #8687235 - Flags: review?(shu) → review+
Attachment #8687236 - Flags: review?(shu) → review+
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.
(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.)
(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...
(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.
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.
Keywords: leave-open
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.
Depends on: 1246396
Depends on: 1246589
Blocks: 1243808
This also seems to have caused a regression in JS memory usage on AWSY which cleared with the backout.
The leave-open keyword is there and there is no activity for 6 months.
:jorendorff, maybe it's time to close this bug?
Flags: needinfo?(jorendorff)
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Flags: needinfo?(jorendorff)
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.