845596 - Keep track of free variables during syntax parsing

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Hackett [Laid off!]]

Attachment: patch

In order to generate bytecode for an outer function without also generating code for its inner functions, all free variables within those inner functions need to be known.  This allows the outer function to correctly choose LOCAL/ARG or ALIASEDVAR ops for its locals.  Free variables can be computed during the syntax parse, and when bytecode is lazily generated the representation for an inner function that has not been compiled will be its set of free variables and a few related bits.

The attached patch makes this change, extending the machinery used during full parses to handle the case where there are no parse nodes.  Testing on jQuery, this slows down syntax parses by about 15%, so that syntax parses are about 3.3x faster than a full parse/emit, rather than 3.8x.  This degrades the percentages in bug 835587 comment 1 by 4-5%, so not huge, and there isn't any more expensive work that the syntax parse still needs to do.  This patch hasn't been exhaustively tested, but seems to do the right thing on small examples (the lack of handling for 'let' simplifies things a lot).

Comment 1

[Jason Orendorff [:jorendorff]]

Comment on attachment 718764 [details] [diff] [review]
patch

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

::: js/src/frontend/FullParseHandler.h
@@ +60,5 @@
>          pn->setOp(JSOP_NAME);
>          return pn;
>      }
> +    Definition *newPlaceholder(ParseNode *pn, ParseContext<FullParseHandler> *pc)
> +    {

Style nit: brace goes on the previous line.

::: js/src/frontend/Parser.cpp
@@ +935,5 @@
>          if (pc->sc->needStrictChecks()) {
>              for (AtomDefnListMap::Range r = pc->decls().all(); !r.empty(); r.popFront()) {
>                  DefinitionList &dlist = r.front().value();
>                  for (DefinitionList::Range dr = dlist.all(); !dr.empty(); dr.popFront()) {
> +                    Definition *dn = dr.front<FullParseHandler>();

Is this template parameter necessary?

::: js/src/frontend/SyntaxParseHandler.h
@@ +173,5 @@
> +
> +    static uintptr_t definitionToBits(DefinitionNode dn) {
> +        // Use a shift, as DefinitionList tags the lower bit of its associated union.
> +        return uintptr_t(dn << 1);
> +    }

This is all so horrible. But I don't see a good way around it.

Comment 2

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/104ba36f44c0

Comment 3

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/adb1bbf756b9 for assertion failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=21780656&tree=Mozilla-Inbound

Comment 4

[Phil Ringnalda (:philor)]

Dunno whether the failures in https://tbpl.mozilla.org/php/getParsedLog.php?id=21781062&tree=Mozilla-Inbound are the same thing, or another.

Comment 5

[Brian Hackett [Laid off!]]

Sorry, forgot to tryserver this first.  Try push:

https://tbpl.mozilla.org/?tree=Try&rev=94740085e31e

And repush:

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

Comment 6

[Ryan VanderMeulen [:RyanVM]]

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

Comment 7

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

This has broken jsfunfuzz severely with bug 862228.