Hook up topStaticScope to StmtInfo and clean it up

RESOLVED FIXED in Firefox 43

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: shu, Assigned: shu)

Tracking

unspecified
mozilla43
Points:
---

Firefox Tracking Flags

(firefox43 fixed)

Details

Attachments

(10 attachments, 1 obsolete attachment)

58.15 KB, patch
efaust
: review+
Details | Diff | Splinter Review
8.50 KB, patch
efaust
: review+
Details | Diff | Splinter Review
62.96 KB, patch
efaust
: review+
Details | Diff | Splinter Review
23.96 KB, patch
efaust
: review+
Details | Diff | Splinter Review
15.09 KB, patch
efaust
: review+
Details | Diff | Splinter Review
47.52 KB, patch
efaust
: review+
Details | Diff | Splinter Review
5.32 KB, patch
efaust
: review+
Details | Diff | Splinter Review
72.32 KB, patch
efaust
: review+
Details | Diff | Splinter Review
8.68 KB, patch
efaust
: review+
Details | Diff | Splinter Review
34.09 KB, patch
efaust
: review+
jandem
: feedback+
Details | Diff | Splinter Review
(Assignee)

Description

3 years ago
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.
(Assignee)

Comment 1

3 years ago
Created attachment 8637583 [details] [diff] [review]
Cleanup: make StmtType an enum class.
Attachment #8637583 - Flags: review?(efaustbmo)
(Assignee)

Comment 2

3 years ago
Created attachment 8637584 [details] [diff] [review]
Cleanup: remove superfluous StmtInfoBase::isNestedScope.
Attachment #8637584 - Flags: review?(efaustbmo)
(Assignee)

Comment 3

3 years ago
Created attachment 8637585 [details] [diff] [review]
Cleanup: use an RAII struct to manage the parser statement stack.
Attachment #8637585 - Flags: review?(efaustbmo)
(Assignee)

Comment 4

3 years ago
Created attachment 8637586 [details] [diff] [review]
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.
Attachment #8637586 - Flags: review?(efaustbmo)
(Assignee)

Updated

3 years ago
Assignee: nobody → shu
Status: NEW → ASSIGNED
(Assignee)

Comment 5

3 years ago
Created attachment 8638863 [details] [diff] [review]
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.
Attachment #8638863 - Flags: review?(efaustbmo)
(Assignee)

Comment 6

3 years ago
Created attachment 8638864 [details] [diff] [review]
Cleanup: Rename top -> innermost, down -> enclosing in StmtInfoStack.
Attachment #8638864 - Flags: review?(efaustbmo)
(Assignee)

Comment 7

3 years ago
Created attachment 8640069 [details] [diff] [review]
Cleanup: Remove dead argument to Parser::parse
Attachment #8640069 - Flags: review?(efaustbmo)
(Assignee)

Comment 8

3 years ago
Created attachment 8640070 [details] [diff] [review]
Hook up the static scope chain in the Parser and replace SharedContext walking with scope walking.
Attachment #8640070 - Flags: review?(efaustbmo)

Comment 9

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

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

APPROVED.
Attachment #8637583 - Flags: review?(efaustbmo) → review+
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.
Attachment #8637584 - Flags: review?(efaustbmo) → review+
(Assignee)

Comment 11

3 years ago
Created attachment 8640198 [details] [diff] [review]
Simplify enclosingStaticScope and rename to innermostStaticScope in BCE.
Attachment #8640198 - Flags: review?(efaustbmo)
(Assignee)

Comment 12

3 years ago
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.
Attachment #8640198 - Attachment is obsolete: true
Attachment #8640198 - Flags: review?(efaustbmo)
(Assignee)

Comment 13

3 years ago
Created attachment 8640203 [details] [diff] [review]
Simplify enclosingStaticScope and rename to innermostStaticScope in BCE.
Attachment #8640203 - Flags: review?(efaustbmo)
(Assignee)

Updated

3 years ago
Keywords: leave-open
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.
Attachment #8637585 - Flags: review?(efaustbmo) → review+
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?
Attachment #8637586 - Flags: review?(efaustbmo) → review+
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.
Attachment #8638863 - Flags: review?(efaustbmo) → review+
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.
Attachment #8638864 - Flags: review?(efaustbmo) → review+
Comment on attachment 8640069 [details] [diff] [review]
Cleanup: Remove dead argument to Parser::parse

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

APPROVED.
Attachment #8640069 - Flags: review?(efaustbmo) → review+
(Assignee)

Updated

3 years ago
Blocks: 1191177
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.
Attachment #8640070 - Flags: review?(efaustbmo) → review+
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.
Attachment #8640203 - Flags: review?(efaustbmo) → review+

Comment 29

3 years ago
Apparently these patches or the ones from bug 1191177 regressed Octane-CodeLoad on AWFY.
Flags: needinfo?(shu)
(Assignee)

Comment 30

3 years ago
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.
Flags: needinfo?(shu)
(Assignee)

Comment 31

3 years ago
Created attachment 8647818 [details] [diff] [review]
Hook up FunctionBox directly to the JSFunction being parsed to avoid allocating extra static scopes.

This fixes the regression locally, but is kinda yuck.
Attachment #8647818 - Flags: review?(efaustbmo)
Attachment #8647818 - Flags: feedback?(jdemooij)
(Assignee)

Updated

3 years ago
Keywords: leave-open
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...
Attachment #8647818 - Flags: feedback?(jdemooij) → feedback+
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
Attachment #8647818 - Flags: review?(efaustbmo) → review+
It looks like 4e1ccbab9d76 changed octane/run.js to only run the zlib benchmark (I would fix this now but the tree is closed....)
(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.
(Assignee)

Updated

3 years ago
Flags: needinfo?(shu)
https://hg.mozilla.org/mozilla-central/rev/527553e5ca43
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox43: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.