Closed
Bug 788957
Opened 13 years ago
Closed 13 years ago
Avoid duplication of fields in SharedContext and FunctionBox
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla18
People
(Reporter: n.nethercote, Assigned: n.nethercote)
Details
(Whiteboard: [js:t])
Attachments
(5 files, 3 obsolete files)
9.43 KB,
patch
|
Benjamin
:
review+
|
Details | Diff | Splinter Review |
31.70 KB,
patch
|
n.nethercote
:
review+
|
Details | Diff | Splinter Review |
29.13 KB,
patch
|
Benjamin
:
review+
|
Details | Diff | Splinter Review |
56.54 KB,
patch
|
Benjamin
:
review+
|
Details | Diff | Splinter Review |
4.48 KB,
patch
|
Benjamin
:
review+
|
Details | Diff | Splinter Review |
SharedContext and FunctionBox both have |strictModeState| and |cxFlags|
fields. This is confusing. This bug will remove SharedContext's
copies of these fields.
![]() |
Assignee | |
Comment 1•13 years ago
|
||
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)
![]() |
Assignee | |
Comment 2•13 years ago
|
||
This patch creates two sub-classes of SharedContext: GlobalSharedContext
and FunctionSharedContext.
Attachment #658809 -
Flags: review?(benjamin)
![]() |
Assignee | |
Comment 3•13 years ago
|
||
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 4•13 years ago
|
||
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 5•13 years ago
|
||
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.
![]() |
Assignee | |
Updated•13 years ago
|
Attachment #658800 -
Attachment is obsolete: true
![]() |
Assignee | |
Updated•13 years ago
|
Attachment #658809 -
Attachment is obsolete: true
Attachment #658809 -
Flags: review?(benjamin)
![]() |
Assignee | |
Updated•13 years ago
|
Attachment #658827 -
Attachment is obsolete: true
Attachment #658827 -
Flags: review?(benjamin)
![]() |
Assignee | |
Comment 6•13 years ago
|
||
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)
![]() |
Assignee | |
Comment 7•13 years ago
|
||
This is just a rebased version of the prior, r+'d patch, with the minor
changes made.
![]() |
Assignee | |
Updated•13 years ago
|
Attachment #659953 -
Flags: review+
![]() |
Assignee | |
Comment 8•13 years ago
|
||
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)
![]() |
Assignee | |
Comment 9•13 years ago
|
||
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)
![]() |
Assignee | |
Comment 10•13 years ago
|
||
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 11•13 years ago
|
||
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()
Attachment #659952 -
Flags: review?(benjamin) → review+
Updated•13 years ago
|
Attachment #659956 -
Flags: review?(benjamin) → review+
Comment 12•13 years ago
|
||
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+
Updated•13 years ago
|
Attachment #659960 -
Flags: review?(benjamin) → review+
![]() |
Assignee | |
Comment 13•13 years ago
|
||
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•13 years ago
|
||
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
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in
before you can comment on or make changes to this bug.
Description
•