Remove the remaining flags from TreeContextFlags

RESOLVED FIXED in mozilla15

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla15
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

5 years ago
In this bug I will remove the remaining eight flags from TreeContextFlags.
(Assignee)

Comment 1

5 years ago
Created attachment 622635 [details] [diff] [review]
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.)
Attachment #622635 - Flags: review?(luke)
(Assignee)

Comment 2

5 years ago
Created attachment 622637 [details] [diff] [review]
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.
Attachment #622637 - Flags: review?(luke)
(Assignee)

Comment 3

5 years ago
Created attachment 622638 [details] [diff] [review]
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().
Attachment #622638 - Flags: review?(luke)

Updated

5 years ago
Attachment #622635 - Flags: review?(luke) → review+

Updated

5 years ago
Attachment #622637 - Flags: review?(luke) → review+

Comment 4

5 years ago
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.
(Assignee)

Comment 5

5 years ago
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

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

Comment 7

5 years ago
Created attachment 622988 [details] [diff] [review]
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()).
Attachment #622638 - Attachment is obsolete: true
Attachment #622638 - Flags: review?(luke)
Attachment #622988 - Flags: review?(luke)

Comment 8

5 years ago
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.
Attachment #622988 - Flags: review?(luke) → review+
(Assignee)

Comment 9

5 years ago
> 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.
(Assignee)

Updated

5 years ago
Blocks: 754179

Comment 10

5 years ago
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?
(Assignee)

Comment 11

5 years ago
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.
https://hg.mozilla.org/mozilla-central/rev/d5a02a324330
https://hg.mozilla.org/mozilla-central/rev/e1acc0dd12a8
https://hg.mozilla.org/mozilla-central/rev/2cde430809e3
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
You need to log in before you can comment on or make changes to this bug.