1179063 - Hook up topStaticScope to StmtInfo and clean it up

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Shu-yu Guo [:shu]]

I feel like StmtInfo in the frontend should reflect the static scope story more closely. In preparation for ES6 global lexicals, for instance, it would make sense to push the global lexical scope onto the frontend stmt stack.

Right now, eval static scopes live separately from the stmt stack static scopes. It would be cleaner unified.

Comment 1

[Shu-yu Guo [:shu]]

Attachment: Cleanup: make StmtType an enum class.

Comment 2

[Shu-yu Guo [:shu]]

Attachment: Cleanup: remove superfluous StmtInfoBase::isNestedScope.

Comment 3

[Shu-yu Guo [:shu]]

Attachment: Cleanup: use an RAII struct to manage the parser statement stack.

Comment 4

[Shu-yu Guo [:shu]]

Attachment: Cleanup: use StmtInfoStack inside BCE and remove templated StmtInfo helper functions.

The BCE did not get the RAII treatment the Parser got, because all the
enter/exit scope operations in the BCE are fallible. Separating out the
fallible paths from the statement stack management made things less
readable.

Comment 5

[Shu-yu Guo [:shu]]

Attachment: Cleanup: remove topStaticScope in favor of using topScopeStmt.

This is to distinguish between getting the top scope associated with a
statement local to the script, or the innermost scope, which may not be
local to the script.

Comment 6

[Shu-yu Guo [:shu]]

Attachment: Cleanup: Rename top -> innermost, down -> enclosing in StmtInfoStack.

Comment 7

[Shu-yu Guo [:shu]]

Attachment: Cleanup: Remove dead argument to Parser::parse

Comment 8

[Shu-yu Guo [:shu]]

Attachment: Hook up the static scope chain in the Parser and replace SharedContext walking with scope walking.

Comment 9

[Eric Faust [:efaust]]

Comment on attachment 8637583 [details] [diff] [review]
Cleanup: make StmtType an enum class.

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

APPROVED.

Comment 10

[Eric Faust [:efaust]]

Comment on attachment 8637584 [details] [diff] [review]
Cleanup: remove superfluous StmtInfoBase::isNestedScope.

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

LGTM

::: js/src/frontend/SharedContext.h
@@ +554,5 @@
>      stmt->label = nullptr;
>      stmt->staticScope = nullptr;
>      stmt->down = ct->topStmt;
>      ct->topStmt = stmt;
> +    stmt->downScope = nullptr;

Nice.

Comment 11

[Shu-yu Guo [:shu]]

Attachment: Simplify enclosingStaticScope and rename to innermostStaticScope in BCE.

Comment 12

[Shu-yu Guo [:shu]]

Comment on attachment 8640198 [details] [diff] [review]
Simplify enclosingStaticScope and rename to innermostStaticScope in BCE.

Oops, got some random stuff in the patch by accident.

Comment 13

[Shu-yu Guo [:shu]]

Attachment: Simplify enclosingStaticScope and rename to innermostStaticScope in BCE.

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/47fe6e30240d
https://hg.mozilla.org/integration/mozilla-inbound/rev/f251b3836314

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Backed out for total build bustage. Please verify that the patches build locally before pushing again.
https://treeherder.mozilla.org/logviewer.html#?job_id=12306715&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/189161dc1616

Comment 16

[Eric Faust [:efaust]]

Comment on attachment 8637585 [details] [diff] [review]
Cleanup: use an RAII struct to manage the parser statement stack.

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

Yay. The less manual fidgeting with chains, the better.

::: js/src/frontend/Parser.h
@@ +350,5 @@
> +
> +      public:
> +        AutoPushStmtInfoPC(Parser<ParseHandler>& parser, StmtType type);
> +        AutoPushStmtInfoPC(Parser<ParseHandler>& parser, StmtType type,
> +                           NestedScopeObject& staticScope);

Does this need the guard param thing so that people can't 

AutoPushStmtInfoPC(parser, type;

? Probably not, since it'll be obviously wrong to any tests.

Comment 17

[Eric Faust [:efaust]]

Comment on attachment 8637586 [details] [diff] [review]
Cleanup: use StmtInfoStack inside BCE and remove templated StmtInfo helper functions.

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

This is much cleaner. Looks like the name predates the patch, though, so what is one to do.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +945,5 @@
>          return false;
>  
>      pushStatement(stmt, stmtType, offset());
>      scopeObj->initEnclosingNestedScope(enclosingStaticScope());
> +    stmtStack.linkAsTopScopal(stmt, *scopeObj);

I have no idea what a "scopal" is. Maybe I will as I keep reading. Is this meant to be like "Scopal unit", or something?

Comment 18

[Eric Faust [:efaust]]

Comment on attachment 8638863 [details] [diff] [review]
Cleanup: remove topStaticScope in favor of using topScopeStmt.

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

Hmmm. If topStaticScope didn't already mean something, I would assume it was the static scope of the top thing. Weird that we have to inline that everywhere instead of just rewriting the helper. Unfortunately, name overloading sucks.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +572,5 @@
>        : bce(bce_),
>          savedScopeIndex(bce->blockScopeList.length()),
>          savedDepth(bce->stackDepth),
> +        openScopeIndex(UINT32_MAX)
> +    {

Normally I wouldn't comment on moving a brace like this, but this one took me two attempts to visually parse in the old configuration. Thank you.

Comment 19

[Eric Faust [:efaust]]

Comment on attachment 8638864 [details] [diff] [review]
Cleanup: Rename top -> innermost, down -> enclosing in StmtInfoStack.

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

After this, I feel like someone might have a fighting chance of understanding what's going on around here.

Comment 20

[Eric Faust [:efaust]]

Comment on attachment 8640069 [details] [diff] [review]
Cleanup: Remove dead argument to Parser::parse

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

APPROVED.

Comment 21

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/52758787f324
https://hg.mozilla.org/integration/mozilla-inbound/rev/dbfb03132803
https://hg.mozilla.org/integration/mozilla-inbound/rev/d5d0d7a18180
https://hg.mozilla.org/integration/mozilla-inbound/rev/4fd477096108
https://hg.mozilla.org/integration/mozilla-inbound/rev/0722492759ff
https://hg.mozilla.org/integration/mozilla-inbound/rev/b25c64b68491
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab699f666b06
https://hg.mozilla.org/integration/mozilla-inbound/rev/856f588ad29e

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/52758787f324
https://hg.mozilla.org/mozilla-central/rev/dbfb03132803
https://hg.mozilla.org/mozilla-central/rev/d5d0d7a18180
https://hg.mozilla.org/mozilla-central/rev/4fd477096108
https://hg.mozilla.org/mozilla-central/rev/0722492759ff
https://hg.mozilla.org/mozilla-central/rev/b25c64b68491
https://hg.mozilla.org/mozilla-central/rev/ab699f666b06
https://hg.mozilla.org/mozilla-central/rev/856f588ad29e

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a9e9b999b337

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a9e9b999b337

Comment 25

[Eric Faust [:efaust]]

Comment on attachment 8640070 [details] [diff] [review]
Hook up the static scope chain in the Parser and replace SharedContext walking with scope walking.

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

This looks good. It could have been two patches, though, I think.

::: js/src/frontend/BytecodeCompiler.cpp
@@ +117,5 @@
>      TokenStream::Position startPosition;
>  
>      RootedScript script;
>      Maybe<BytecodeEmitter> emitter;
> +};

wtf? How did that get there?

::: js/src/frontend/Parser.cpp
@@ +161,5 @@
> +            superScopeAlreadyNeedsHomeObject_ = true;
> +            return;
> +        }
> +    }
> +    MOZ_CRASH("Must have found a super function box scope that allows super.property");

what does the first "super" mean here? "Enclosing," right?

::: js/src/frontend/SharedContext.h
@@ +213,5 @@
> +    // GlobalSharedContext and FunctionBox have different lifetimes.
> +    // GlobalSharedContexts are stack allocated and thus may use RootedObject
> +    // for the static scope. FunctionBoxes are LifoAlloc'd and need to
> +    // manually trace their static scope.
> +    virtual JSObject* staticScope() const = 0;

(o.O;)

arrrrgleblaaahhhrhg

::: js/src/vm/ScopeObject.h
@@ +49,5 @@
>   * 'x', enclosed by a scope containing only the name 'f'. (This separate scope
>   * is necessary due to the fact that declarations in the function scope shadow
>   * (dynamically, in the case of 'eval') the lambda name.)
> + *
> + * TODOshu: Rewrite static scope explanation

Much appreciated.

@@ +459,5 @@
> +            getReservedSlot(FUNCTION_BOX_SLOT).toPrivate());
> +    }
> +
> +    JSObject* enclosingScopeForStaticScopeIter() {
> +        return getReservedSlot(SCOPE_CHAIN_SLOT).toObjectOrNull();

static_assert here? Since we don't control that directly, it seems unwise to rely on it.

Comment 26

[Eric Faust [:efaust]]

Comment on attachment 8640203 [details] [diff] [review]
Simplify enclosingStaticScope and rename to innermostStaticScope in BCE.

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

This is much nicer, with the function box just becoming the function in the chain.

Comment 27

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4e1ccbab9d76
https://hg.mozilla.org/integration/mozilla-inbound/rev/c7891ce06d73

Comment 28

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4e1ccbab9d76
https://hg.mozilla.org/mozilla-central/rev/c7891ce06d73

Comment 29

[Guilherme Lima]

Apparently these patches or the ones from bug 1191177 regressed Octane-CodeLoad on AWFY.

Comment 30

[Shu-yu Guo [:shu]]

I think the regressing patch is https://hg.mozilla.org/mozilla-central/rev/4e1ccbab9d76

My hunch is that because the static scope chain is hooked up in the parser via these new StaticFunctionBoxScopeObjects, allocating all those static scopes in the parser regressed octane-codeload.

Don't know how to fix yet.

Comment 31

[Shu-yu Guo [:shu]]

Attachment: Hook up FunctionBox directly to the JSFunction being parsed to avoid allocating extra static scopes.

This fixes the regression locally, but is kinda yuck.

Comment 32

[Jan de Mooij [:jandem]]

Comment on attachment 8647818 [details] [diff] [review]
Hook up FunctionBox directly to the JSFunction being parsed to avoid allocating extra static scopes.

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

Thanks for fixing this.

I agree it's not the greatest thing, but OTOH avoiding the extra ScopeObject and switchStaticScopeToFunction is kinda nice though...

Comment 33

[Eric Faust [:efaust]]

Comment on attachment 8647818 [details] [diff] [review]
Hook up FunctionBox directly to the JSFunction being parsed to avoid allocating extra static scopes.

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

This looks like exactly what we discussed on IRC. r=me

Comment 34

[Brian Hackett [Laid off!]]

It looks like 4e1ccbab9d76 changed octane/run.js to only run the zlib benchmark (I would fix this now but the tree is closed....)

Comment 35

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/08e63874f0b4

Comment 36

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/abc5082cc214 for crashes like https://treeherder.mozilla.org/logviewer.html#?job_id=13059698&repo=mozilla-inbound

Comment 37

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(In reply to Wes Kocher (:KWierso) from comment #36)
> Backed out in
> https://hg.mozilla.org/integration/mozilla-inbound/rev/abc5082cc214 for
> crashes like
> https://treeherder.mozilla.org/logviewer.html#?job_id=13059698&repo=mozilla-
> inbound

Looks like this might actually be bustage from a previous push. Feel free to reland whenever the tree reopens. Sorry for the churn.

Comment 38

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/527553e5ca43

Comment 39

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/527553e5ca43