753657 - Remove the remaining flags from TreeContextFlags

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

In this bug I will remove the remaining eight flags from TreeContextFlags.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Patch 1: remove TCF_FUN_FLAGS

All the remaining TCF_* flags are in TCF_FUN_FLAGS, which means we can get rid
of TCF_FUN_FLAGS.

Some of the places where TCF_FUN_FLAGS were used can be further simplified
by observing that all the flags in TCF_FUN_FLAGS are set but never cleared.
This means that in functionBody(), condExpr1() and bracketedExpr(), we don't
need |oldflags|.  In other words this is a no-op:

    uint32_t oldflags = tc->sc->flags;
    ...
    tc->sc->flags = oldflags | tc->sc->flags;

(I checked this with assertions before modifying the code, just to be safe.)

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: Patch 2: simplify FunctionBox::tcflags setting

TCF_STRICT_MODE presents an annoying wrinkle.  In EnterFunction, we call
newFunctionBox, and funbox->tcflags inherits TCF_STRICT_MODE_CODE from the
parent tc->sc->flags.  Back in EnterFunction, we then propagate that bit
from funbox->tcflags into the funtc->sc->flags (via a full assignment, but
TCF_STRICT_MODE_CODE is the only bit that can be set in funbox->tcflags).

This is obtuse.  This patch changes things so that:

- funbox->tcflags isn't set until LeaveFunction;

- funtc->sc->flags inherits TCF_STRICT_MODE_CODE directly from tc->sc->flags.

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: Patch 3: remove the remaining TCF_* flags

This replaces all the remaining TCF_* flags with fields in SharedContext and
FunctionBox.  Notable things:

- I did some slight renaming so that all flags that apply only to functions
  (six of them) start with "fun".

- I removed the existing getters/setters -- bindingsAccessedDynamically(), 
  noteBindingsAccessedDynamically(), etc.  They seem less necessary than
  they used to be, and direct field access is consistent with the other
  flags I've been creating.

- I added some assertions in NewScriptFromEmitter relating to flags that
  should/shouldn't be set for |inFunction| scripts.

- JSOPTION_STRICT_MODE is now consulted in SharedContext's constructor,
  rather than TreeContext::init().

Comment 4

[Luke Wagner [:luke]]

For patch 3, I was wondering: what if we did something like:

struct FunctionFlags {
  // nice comments
  bool funIsHeavyweight : 1;
  ...
};

and then embed FunctionFlags in both SharedContext and FunctionBox?  The main benefits I see:
 1. single definition of fields
 2. no manual per-member copy to and from SharedContext/FunctionBox

Also, this would encourage further abstraction like private fields with memfuns that assert/enforce invariants.

Comment 5

[Nicholas Nethercote [inactive]]

I considered and rejected that because we'd have a lot of things like this:

  if (bce->sc->flags->inStrictMode())

instead of this:

  if (bce->sc->inStrictMode)

but I can do it if you feel strongly.  I guess I could put the getters/setters on SharedContext to get this:

  if (bce->sc->inStrictMode())

Comment 6

[Luke Wagner [:luke]]

Alternatively, you could have BytecodeEmitter derive FunctionFlags.  (Goofy, but symmetric with BytecodeEmitter deriving TreeContext.)

Comment 7

[Nicholas Nethercote [inactive]]

Attachment: Patch 3, v2: remove the remaining TCF_* flags

New version.  Changes:

- I added ContextFlags, which encapsulates the eight flags.  I didn't call it
  FunctionFlags because it can be used with global (i.e. non-Function) code.
  Both SharedContext and FunctionBox have a ContextFlags.
  
- SharedContext and FunctionBox provide getters/setters as necessary;  
  SharedContext has them for all flags, FunctionBox for just a few.
  I used "set" as the setter prefix instead of "note", because "set" seems 
  to be more standard (e.g. the existing setStrictMode()).

Comment 8

[Luke Wagner [:luke]]

Comment on attachment 622988 [details] [diff] [review]
Patch 3, v2: remove the remaining TCF_* flags

>@@ -5457,22 +5458,30 @@ Parser::generatorExpr(ParseNode *kid)
>-        gensc.flags |= TCF_FUN_IS_GENERATOR | outertc->sc->flags;
>+        if (outertc->sc->inStrictMode())                gensc.setInStrictMode();
>+        if (outertc->sc->bindingsAccessedDynamically()) gensc.setBindingsAccessedDynamically();
>+        if (outertc->sc->funIsHeavyweight())            gensc.setFunIsHeavyweight();
>+                                                        gensc.setFunIsGenerator();
>+        if (outertc->sc->funMightAliasLocals())         gensc.setFunMightAliasLocals();
>+        if (outertc->sc->funHasExtensibleScope())       gensc.setFunHasExtensibleScope();
>+        if (outertc->sc->funArgumentsHasLocalBinding()) gensc.setFunArgumentsHasLocalBinding();
>+        if (outertc->sc->funDefinitelyNeedsArgsObj())   gensc.setFunDefinitelyNeedsArgsObj();

Can this be gensc.cxFlags = outertc->sc->cxFlags?  Otherwise, it is likely anyone adding new flags will forget to add them here (and adding them here seems to be the conservative default).  After the copy there can be the fixup which does setFunIsGenerator() etc.

Comment 9

[Nicholas Nethercote [inactive]]

> Can this be gensc.cxFlags = outertc->sc->cxFlags?  Otherwise, it is likely
> anyone adding new flags will forget to add them here (and adding them here
> seems to be the conservative default).  After the copy there can be the
> fixup which does setFunIsGenerator() etc.

I don't think it can be, unfortunately -- you don't want to overwrite any of the already-set flags with |false|.

However, in practice I think at this point inStrictMode is the only one that can be set, but knowing that requires understanding the preceding code.  If I was going to rely on that I'd want to have assertions like:

  JS_ASSERT(!gensc.bindingsAccessedDynamically())
  JS_ASSERT(!getsc.funIsHeavyweight())
  ...

which is just as verbose and also prone to being overlooked if a new flag is added.

I'll take another look and see if any other options occur to me.

Comment 10

[Luke Wagner [:luke]]

Ah, good point; I assumed gensc was fresh.  Asserting only strictness is set would be good if its true.  If that isn't possible, could we avoid the future hazard (when adding a new flag) by adding an explicit ContextFlags::propgateFlagsToGenExpr(ContextFlags *dst) method to ContextFlags?

Comment 11

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d5a02a324330
https://hg.mozilla.org/integration/mozilla-inbound/rev/e1acc0dd12a8
https://hg.mozilla.org/integration/mozilla-inbound/rev/2cde430809e3

> Ah, good point; I assumed gensc was fresh.  Asserting only strictness is set
> would be good if its true.  If that isn't possible, could we avoid the
> future hazard (when adding a new flag) by adding an explicit
> ContextFlags::propgateFlagsToGenExpr(ContextFlags *dst) method to
> ContextFlags?

I ended up tweaking EnterFunction -- moving the isInStrict setting out of it -- so that gensc was fresh in this instance.  I think that makes things better.

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/d5a02a324330
https://hg.mozilla.org/mozilla-central/rev/e1acc0dd12a8
https://hg.mozilla.org/mozilla-central/rev/2cde430809e3