788957 - Avoid duplication of fields in SharedContext and FunctionBox

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [:njn] (On Long Service Leave)]

SharedContext and FunctionBox both have |strictModeState| and |cxFlags| 
fields.  This is confusing.  This bug will remove SharedContext's 
copies of these fields.

Comment 1

[Nicholas Nethercote [:njn] (On Long Service Leave)]

Attachment: Split ContextFlags into AnyContextFlags and FunctionContextFlags.

This patch splits ContextFlags into AnyContextFlags (which apply to any
context) and FunctionContextFlags (which only apply to function contexts).

As a result, FunctionBox is now the only place where function-specific 
context flags are stored.

Comment 2

[Nicholas Nethercote [:njn] (On Long Service Leave)]

Attachment: (part 2) - Create GlobalSharedContext and FunctionSharedContext sub-classes.

This patch creates two sub-classes of SharedContext:  GlobalSharedContext 
and FunctionSharedContext.

Comment 3

[Nicholas Nethercote [:njn] (On Long Service Leave)]

Attachment: (part 3) - Remove |anyCxFlags| and |strictModeState| from SharedContext.

This patch removes |anyContextFlags| and |strictModeState| from 
SharedContext;  the corresponding fields in GlobalSharedContext and 
FunctionBox are now always used.  This avoids some flag copying and some 
flag-equivalence assertions.

Comment 4

[:Benjamin Peterson]

Comment on attachment 658800 [details] [diff] [review]
Split ContextFlags into AnyContextFlags and FunctionContextFlags.

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

This is definitely an improvement. It would be interesting to see if the now 2-fielded AnyContext class can be killed.

::: js/src/frontend/Parser.cpp
@@ +399,5 @@
>      strictModeState(sms),
>      inWith(false),                  // initialized below
>      inGenexpLambda(false),
> +    anyCxFlags(),
> +    funCxFlags()                    // the cxFlags are set in LeaveFunction

Comment needs to be updated.

@@ +2182,5 @@
>      StmtInfoPC *stmt = LexicalLookup(pc, name, NULL, (StmtInfoPC *)NULL);
>  
>      if (stmt && stmt->type == STMT_WITH) {
>          pn->pn_dflags |= PND_DEOPTIMIZED;
>          if (pc->sc->inFunction()) 

It looks like there's trailing whitespace here. Can you kill it, while you're at it?

::: js/src/frontend/SharedContext.h
@@ +307,5 @@
> +    bool funIsGenerator()              const { return funCxFlags.funIsGenerator; }
> +    bool funMightAliasLocals()         const { return funCxFlags.funMightAliasLocals; }
> +    bool funHasExtensibleScope()       const { return funCxFlags.funHasExtensibleScope; }
> +    bool funArgumentsHasLocalBinding() const { return funCxFlags.funArgumentsHasLocalBinding; }
> +    bool funDefinitelyNeedsArgsObj()   const { return funCxFlags.funDefinitelyNeedsArgsObj; }

Now that these are always on the FunctionBox, how about dropping the "fun" prefix?

Comment 5

[:Benjamin Peterson]

Comment on attachment 658827 [details] [diff] [review]
(part 3) - Remove |anyCxFlags| and |strictModeState| from SharedContext.

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

I'm not sure about this patch. Yes, it avoids some flag copying and assertions but at the cost of the strange indirection in SharedContext.

The small size of AnyContext makes me wonder if SharedContext needs to exist at all. It would be great if everything could be moved to the parse tree. explicitUseStrict is only necessary on function nodes, since if a global scope is strict, it obviously had a strict declaration. (explicitUseStrict could be also determined just by looking at the parse tree.) bindingsAccessedDynamically could perhaps become a flag on some node in the parse tree. strictModeState should only not be STRICT or NOTSTRICT during parsing, so it would be more pure to put strictModeState on ParseContext and a binary flag for strictness somewhere on the parse tree.

Comment 6

[Nicholas Nethercote [:njn] (On Long Service Leave)]

Attachment: (part 1) - Change FunctionBox so it has an ObjectBox rather than is an ObjectBox.

FunctionBox is a sub-class of ObjectBox.  This patch changes things so that
FunctionBox instead contains an ObjectBox, which in isolation isn't much of
a win (nor a loss), but will allow me to make FunctionBox a sub-class of
SharedContext without introducing multiple inheritance.

Comment 7

[Nicholas Nethercote [:njn] (On Long Service Leave)]

Attachment: (part 2) - Split ContextFlags into AnyContextFlags and FunctionContextFlags.

This is just a rebased version of the prior, r+'d patch, with the minor
changes made.

Comment 8

[Nicholas Nethercote [:njn] (On Long Service Leave)]

Attachment: (part 3) - Change SharedContext::inFunction() to SharedContext::isFunction.

This patch changes SharedContext::inFunction() to SharedContext::isFunction.
I like the new name better, and making it a field instead of a method will 
fit in with the upcoming SharedContext split.

Comment 9

[Nicholas Nethercote [:njn] (On Long Service Leave)]

Attachment: (part 4) - Add GlobalSharedContext, a sub-class of SharedContext, and also make FunctionBox a sub-class of SharedContext.

This patch creates GlobalSharedContext, which is a sub-class of 
SharedContext.  It also makes FunctionBox a sub-class of SharedContext.  
This removes the remaining field duplication between SharedContext and
FunctionBox, hooray!

My only concern is the names -- FunctionBox doesn't really fit with 
SharedContext and SharedGlobalContext.  But I'm also not in the mood for a 
mass renaming of any of those types, so I'm inclined to live with it.

Comment 10

[Nicholas Nethercote [:njn] (On Long Service Leave)]

Attachment: (part 5) - Remove unused arguments from NoteLValue().

This doesn't really fit with the other stuff, but I've had it in my queue 
for a while.  It removes some unused arguments from NoteLValue().

Comment 11

[:Benjamin Peterson]

Comment on attachment 659952 [details] [diff] [review]
(part 1) - Change FunctionBox so it has an ObjectBox rather than is an ObjectBox.

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

Looks fine. There's one assertion I think can be removed and a few pre-existing style issues that can be fixed.

::: js/src/frontend/Parser.cpp
@@ +394,5 @@
>  
>      return objbox;
>  }
>  
> +FunctionBox::FunctionBox(ObjectBox* traceListHead, JSFunction *fun, ParseContext *outerpc,

Adjust "*" on ObjectBox.

@@ +457,3 @@
>  {
> +    JS_ASSERT(fun && !IsPoisonedPtr(fun));
> +    JS_ASSERT(fun->isFunction());

If you're passing a JSFunction pointer, isn't this assertion is unnecessary?

::: js/src/frontend/SharedContext.h
@@ +298,5 @@
>      bool            inGenexpLambda:1;       /* lambda from generator expression */
>  
>      ContextFlags    cxFlags;
>  
> +    FunctionBox(ObjectBox* traceListHead, JSFunction *fun, ParseContext *pc, StrictMode sms);

Might as well move the "*" of ObjectBox to the variable.

@@ +303,4 @@
>  
>      bool funIsGenerator() const { return cxFlags.funIsGenerator; }
>  
> +    JSFunction *fun() const { return (JSFunction *) objbox.object; }

objbox.object->toFunction()

Comment 12

[:Benjamin Peterson]

Comment on attachment 659959 [details] [diff] [review]
(part 4) - Add GlobalSharedContext, a sub-class of SharedContext, and also make FunctionBox a sub-class of SharedContext.

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

Nit: "subclass" doesn't need a hyphen.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +1378,5 @@
>           */
>          if (dn->pn_cookie.level() != bce->script->staticLevel)
>              return true;
>  
> +        RootedFunction fun(cx, bce->sc->asFunbox()->fun());

You're rooting the function here, but not in EmitAliasedVarOp() even though no gc can occur in either case.

::: js/src/frontend/Parser.cpp
@@ +1813,5 @@
>      if (pc->sc->strictModeState != StrictMode::UNKNOWN) {
>          // Strict mode was inherited.
>          JS_ASSERT(pc->sc->strictModeState == StrictMode::STRICT);
>          if (pc->sc->isFunction) {
> +            JS_ASSERT(pc->sc->asFunbox()->strictModeState == pc->sc->strictModeState);

This assertion is vacuous now.

::: js/src/frontend/SharedContext-inl.h
@@ +26,5 @@
>  inline bool
>  SharedContext::inStrictMode()
>  {
>      JS_ASSERT(strictModeState != StrictMode::UNKNOWN);
> +    JS_ASSERT_IF(isFunction, asFunbox()->strictModeState == strictModeState);

This assertion is vacuous now.

Comment 13

[Nicholas Nethercote [:njn] (On Long Service Leave)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/50c9565a5f3d
https://hg.mozilla.org/integration/mozilla-inbound/rev/85be098d321d
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7faae8e1d2b
https://hg.mozilla.org/integration/mozilla-inbound/rev/96fc70c99ed1
https://hg.mozilla.org/integration/mozilla-inbound/rev/d6d70984c0f6

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/50c9565a5f3d
https://hg.mozilla.org/mozilla-central/rev/85be098d321d
https://hg.mozilla.org/mozilla-central/rev/b7faae8e1d2b
https://hg.mozilla.org/mozilla-central/rev/96fc70c99ed1
https://hg.mozilla.org/mozilla-central/rev/d6d70984c0f6