1191177 - Kill UpvarCookie and static level

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Shu-yu Guo [:shu]]

UpvarCookie is confusing. It unions two different pieces of information: it either stores an absolute "static level" for definitions, or a hop count for the number of dynamic scopes to skip for uses.

I plan to kill it in favor of using the static scope chain to compute the hop count.

In lieu of UpvarCookie, each ParseNode would always store a ScopeCoordinate. This ScopeCoordinate can have both an unknown hop count and slot. An unknown hop count means that the BCE should compute the correct hop count. An unknown slot means the reference is free.

Definitions start off in the Parser with an unknown hop count but a known slot. Uses start off in the Parser being free. The BCE then compute the correct hop count using the static scope chain for both definitions and uses. The BCE also computes the slot for uses. Name binding logic will be refactored to use the static scope chain instead of dealing with block scope ad-hoc.

To compute the hop count, instead of the Parser and BCE trying to compute it up front, each blockid will have a static scope object associated with it. The hop count will be computed in the BCE by counting the number of hops on the static scope chain with dynamic scope objects from the innermost static scope chain to the static scope of the definition block's static scope.

In lieu of static level, the static scope chain will be used.

Comment 1

[Shu-yu Guo [:shu]]

Attachment: Kill UpvarCookie.

Comment 2

[Shu-yu Guo [:shu]]

Attachment: Kill staticLevel.

Comment 3

[Eric Faust [:efaust]]

Comment on attachment 8643482 [details] [diff] [review]
Kill UpvarCookie.

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

I still don't understand why we had to change everything to take a Parser. Can you revisit that before landing?

::: js/src/frontend/BytecodeEmitter.cpp
@@ +1266,5 @@
> +        // The definition for x is emitted when the block scope for the catch
> +        // is innermost. Moreover, that block scope has aliased bindings, so
> +        // there is a non-0 hop count.
> +        if (pn->pn_scopecoord.isHopsUnknown()) {
> +            BytecodeEmitter* bceOfDef;

This looks like it should be DebugOnly in this scope, if that even works. Certainly going to warn in an opt build as written.

@@ +1274,5 @@
> +                return false;
> +        }
> +
> +#ifdef DEBUG
> +        BytecodeEmitter* bceOfDef;

This bceOfDef should be declared = this, and be outside the #ifdef. The computeDefinitionIsAlias() call below will not compile in opt builds as written.

@@ +1293,5 @@
>      }
>  
> +#ifdef DEBUG
> +    BytecodeEmitter* bceOfDef;
> +    (void) computeHops(pn, &bceOfDef);

The fact that we are calling this for the outparam could maybe use a comment, here.

@@ +1415,5 @@
>        case Definition::VAR:
>        case Definition::GLOBALCONST:
> +        MOZ_ASSERT_IF(sc->allLocalsAliased(),
> +                      bceOfDef->script->localIsAliased(pn->pn_scopecoord.slot()));
> +        return bceOfDef->script->localIsAliased(pn->pn_scopecoord.slot());

I don't see why we need to lookup script from bceOfDef if we short circuit above. Perhaps this should just stay the way it was?

@@ +1435,5 @@
> +    } else if (isAliasedName(bceOfDef, dn)) {
> +        // Translate the frame slot to a slot on the dynamic scope
> +        // object. Aliased block bindings do not need adjusting; see
> +        // computeAliasedSlots.
> +        uint32_t slot = pn->pn_scopecoord.slot();

Shouldn't we be changing |dn| and not |pn|? |pn| could be a use here, not the def.

@@ +1440,5 @@
> +        if (blockScopeOfDef(dn)->is<JSFunction>()) {
> +            MOZ_ASSERT(IsArgOp(*op) || slot < bceOfDef->script->bindings.numBodyLevelLocals());
> +            MOZ_ALWAYS_TRUE(bceOfDef->lookupAliasedName(bceOfDef->script, pn->name(), &slot));
> +        }
> +        if (!pn->pn_scopecoord.setSlot(parser->tokenStream, slot))

same as above.

@@ +1630,5 @@
>   * BindNameToSlotHelper attempts to optimize name gets and sets to stack slot
>   * loads and stores, given the compile-time information in |this| and a PNK_NAME
>   * node pn.  It returns false on error, true on success.
>   *
> + * The caller can test pn->pn_scopecoord.isBound() to tell whether

So, we actually landed on isFree() right? So this comment is incorrect

@@ +1849,5 @@
>       */
> +    if (bceOfDef != this && !bceOfDef->sc->isFunctionBox())
> +        return true;
> +
> +    if (!pn->pn_scopecoord.set(parser->tokenStream, hops, slot))

If we have this, I guess I'm supposed to gripe about the two-liner above with setHops and setSlot...I can't really be bothered to care, to be honest.

@@ +1857,5 @@
> +    // have been emitted yet, and we need to manually check if this access is
> +    // aliased. Otherwise, the definition would have already been emitted by
> +    // emitVarOp, and, if aliased, would have had its JSOp changed to an
> +    // aliased op.
> +    if (!computeDefinitionIsAliased(bceOfDef, dn, &op))

If we are always going to call this with pn->isDefn() true, can computeDefinitionIsAliased be changed to just take a Definition*? Seems like we should be able to use the typesystem to save us from ourselves.

::: js/src/frontend/ParseNode.h
@@ +61,1 @@
>          if (newSlot >= SCOPECOORD_SLOT_LIMIT)

Shouldn't this be UNKNOWN_SLOT if it's SCOPECOORD_SLOT_LIMIT - 1? We'll hit it first, to be sure, and we should call makeFree() to set it unknown directly, right?

::: js/src/frontend/Parser.cpp
@@ +284,5 @@
>      if (kind == Definition::ARG) {
>          if (!args_.append((Definition*) nullptr))
>              return false;
>          if (args_.length() >= ARGNO_LIMIT) {
> +            parser.tokenStream.reportError(JSMSG_TOO_MANY_FUN_ARGS);

I think you can just parser.reportError, with a bunch more args? But what's more, I'm not sure we need to change the signature here at all. Can you remember why?

@@ +3071,5 @@
>      // when creating Bindings. See ParseContext::generateFunctionBindings and
>      // AppendPackedBindings.
> +    //
> +    // As with all bindings, the number of hops is always 0 at the point of
> +    // definition.

This comment is wrong, as you pointed out.

@@ +3077,3 @@
>          return false;
>  
> +

nit: random empty line

Comment 4

[Eric Faust [:efaust]]

Comment on attachment 8643993 [details] [diff] [review]
Kill staticLevel.

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

LGTM. That check is a little scary, but I think I'm satisfied.

::: js/src/frontend/Parser.cpp
@@ +2380,5 @@
>              if (!parser->tokenStream.seek(position, tokenStream))
>                  return false;
>  
>              ParseContext<SyntaxParseHandler> funpc(parser, outerpc, SyntaxParseHandler::null(),
> +                                                   funbox, newDirectives, 

nit: whitespace after trailing comma

::: js/src/frontend/Parser.h
@@ +298,5 @@
>  
>      // True if this is the ParseContext for the body of a function created by
>      // the Function constructor.
>      bool isFunctionConstructorBody() const {
> +        return sc->isFunctionBox() && !parent && sc->asFunctionBox()->function()->isLambda();

OK, so this check is a little scary. I think it does differentiate the case you want from ASMJS, but I am dubious that it will be OK in the face of JS::CompileFunction() in jsapi. I guess that case probably gets bunched with the current staticLevel 0 also.

Comment 5

[Eric Faust [:efaust]]

Comment on attachment 8643993 [details] [diff] [review]
Kill staticLevel.

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

LGTM. That check is a little scary, but I think I'm satisfied.

::: js/src/frontend/Parser.cpp
@@ +2380,5 @@
>              if (!parser->tokenStream.seek(position, tokenStream))
>                  return false;
>  
>              ParseContext<SyntaxParseHandler> funpc(parser, outerpc, SyntaxParseHandler::null(),
> +                                                   funbox, newDirectives, 

nit: whitespace after trailing comma

::: js/src/frontend/Parser.h
@@ +298,5 @@
>  
>      // True if this is the ParseContext for the body of a function created by
>      // the Function constructor.
>      bool isFunctionConstructorBody() const {
> +        return sc->isFunctionBox() && !parent && sc->asFunctionBox()->function()->isLambda();

OK, so this check is a little scary. I think it does differentiate the case you want from ASMJS, but I am dubious that it will be OK in the face of JS::CompileFunction() in jsapi. I guess that case probably gets bunched with the current staticLevel 0 also.

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cdfe078bc9d6
https://hg.mozilla.org/integration/mozilla-inbound/rev/4aca9861fa7b

Comment 7

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

https://hg.mozilla.org/mozilla-central/rev/cdfe078bc9d6
https://hg.mozilla.org/mozilla-central/rev/4aca9861fa7b

Comment 8

[Jason Orendorff [:jorendorff]]

Comment on attachment 8643482 [details] [diff] [review]
Kill UpvarCookie.

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

Abdicating this one - I agree with the sentiment but never looked at the details.