Closed Bug 788957 Opened 9 years ago Closed 9 years ago

Avoid duplication of fields in SharedContext and FunctionBox


(Core :: JavaScript Engine, defect)

Not set





(Reporter: n.nethercote, Assigned: n.nethercote)


(Whiteboard: [js:t])


(5 files, 3 obsolete files)

SharedContext and FunctionBox both have |strictModeState| and |cxFlags| 
fields.  This is confusing.  This bug will remove SharedContext's 
copies of these fields.
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.
Attachment #658800 - Flags: review?(benjamin)
This patch creates two sub-classes of SharedContext:  GlobalSharedContext 
and FunctionSharedContext.
Attachment #658809 - Flags: review?(benjamin)
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.
Attachment #658827 - Flags: review?(benjamin)
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?
Attachment #658800 - Flags: review?(benjamin) → review+
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.
Attachment #658800 - Attachment is obsolete: true
Attachment #658809 - Attachment is obsolete: true
Attachment #658809 - Flags: review?(benjamin)
Attachment #658827 - Attachment is obsolete: true
Attachment #658827 - Flags: review?(benjamin)
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.
Attachment #659952 - Flags: review?(benjamin)
This is just a rebased version of the prior, r+'d patch, with the minor
changes made.
Attachment #659953 - Flags: review+
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.
Attachment #659956 - Flags: review?(benjamin)
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.
Attachment #659959 - Flags: review?(benjamin)
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().
Attachment #659960 - Flags: review?(benjamin)
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; }

Attachment #659952 - Flags: review?(benjamin) → review+
Attachment #659956 - Flags: review?(benjamin) → review+
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.
Attachment #659959 - Flags: review?(benjamin) → review+
Attachment #659960 - Flags: review?(benjamin) → review+
You need to log in before you can comment on or make changes to this bug.