Closed Bug 784608 Opened 8 years ago Closed 8 years ago

Clear dead wood from FunctionBox

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla17

People

(Reporter: njn, Assigned: njn)

References

Details

Attachments

(8 files, 3 obsolete files)

2.21 KB, patch
jimb
: review+
Details | Diff | Splinter Review
3.17 KB, patch
jimb
: review+
Details | Diff | Splinter Review
11.61 KB, patch
jimb
: review+
Details | Diff | Splinter Review
2.14 KB, patch
jimb
: review+
Details | Diff | Splinter Review
2.81 KB, patch
luke
: review+
Details | Diff | Splinter Review
23.74 KB, patch
luke
: review+
Details | Diff | Splinter Review
7.80 KB, patch
luke
: review+
Details | Diff | Splinter Review
2.43 KB, patch
luke
: review+
Details | Diff | Splinter Review
This bug clears some dead wood from FunctionBox.
Summary: Clear dead wood from FunctionBox. → Clear dead wood from FunctionBox
This patch removes FunctionBox::inLoop, which is dead.
Attachment #654113 - Flags: review?(jimb)
This patch removes FunctionBox::level, which is dead.
Attachment #654114 - Flags: review?(jimb)
This patch removes FunctionBox::node.  Its only use of note is in
functionArguments(), but we can pass the relevant ParseNode directly as an 
argument instead of doing it indirectly via pc->sc->funbox->node.  And once
that's done, the field can be removed.
Attachment #654115 - Flags: review?(jimb)
(In reply to Nicholas Nethercote [:njn] from comment #3)
Hah, nice.
This patch removes FunctionBox::inAnyDynamicScope, which is dead.  That
allows FunctionBox::funHasExtensibleScope to be removed as well.
Attachment #654465 - Flags: review?(jimb)
This patch just renames some args to make things clearer.
Attachment #654499 - Flags: review?(luke)
ParseContext::innermostWith is a ParseNode*, but it's only ever used in 
boolean contexts.  This patch changes it to a boolean.  

I used the name "inWithPC" -- the "PC" part is meant to indicate that it
tracks whether we're within a |with| in this ParseContext, but it ignores
outer ParseContexts.  For example, if we have function |f| within a |with|,
|f|'s ParseContext does not set "inWithPC".  

This non-inheritance is the existing behaviour, but it might be nicer to
just make it inherit, because it's (a) easier to think about, (b) it's
what'll be needed for FunctionBox (see the follow-up patches) and (c) I
think it would be ok for the only other use (in LeaveFunction()), because it
would de-optimize more cases but who cares about that.
Attachment #654503 - Flags: review?(luke)
FunctionBox::inWith only indicates if the function is directly within a 
|with|, without any intervening functions.  (Despite the fact that the field 
comment says "some enclosing scope is a with-statement or E4X 
filter-expression".)  So in needsImplicitThis() we need to traverse all the
ancestor FunctionBoxes as well.

This patch changes it so that FunctionBox::inWith is true if it's within any
|with| (making that comment more correct).  This avoids the need for the
FunctionBox::parent traversal.
Attachment #654504 - Flags: review?(luke)
FunctionBox::parent is now dead and can be removed.
Attachment #654507 - Flags: review?(luke)
With all those patches in place, FunctionBox::{kids,siblings} and ParseContext::functionList are only needed for the recursivelySetStrictMode() call in Parser::setStrictMode()... hmm.
Thinking some more, I think FunctionBox is being abused.  One of its main reasons for existence is to serve as an intermediary -- so that a SharedContext created in the Parser can be re-created for in the BytecodeEmitter.  So why does SharedContext::funbox_ exist?  Hmm.
Comment on attachment 654503 [details] [diff] [review]
(part 6) - Boolify ParseContext::innermostWith.

Actually, I've got a better approach coming soon.
Attachment #654503 - Attachment is obsolete: true
Attachment #654503 - Flags: review?(luke)
Attachment #654504 - Attachment is obsolete: true
Attachment #654504 - Flags: review?(luke)
Attachment #654507 - Attachment is obsolete: true
Attachment #654507 - Flags: review?(luke)
Attachment #654499 - Flags: review?(luke) → review+
This patch renames some variables that are set during parsing/emitting, to
clarify their temporary nature.  This will help make the following patch 
clearer.
Attachment #654873 - Flags: review?(luke)
This patch boolifies |innermostWith|, changing it to |parsingWith|, because
a bool is all that's needed.

Furthermore, it changes its meaning -- |parsingWith| is now inherited from
the parent context, which means its true if we're parsing within a
with-statement in this ParseContext *chain*, not just this ParseContext.  
Consequences of this:

- The meaning of FunctionBox::inWith is changed similarly -- it's now true 
  if we're parsing within any with-statement (but in this case, it's true 
  even if there is an intervening |eval| because of the scopeChain test in 
  FunctionBox()).  Happily, this change makes the comment for
  FunctionBox::inWith accurate.

- As a result, BCE::needsImplicitThis() no longer needs to follow the whole
  FunctionBox chain (and this will allow FunctionBox::parent to be removed 
  subsequently).

- The DeoptimizeUsesWithin() case will happen more often in LeaveFunction(),
  but that shouldn't matter.  Indeed, the comment on that case says "Make
  sure to deoptimize lexical dependencies that are polluted by ... any
  enclosing 'with'" -- the new behaviour matches that comment better than the
  old behaviour, AFAICT.
Attachment #654876 - Flags: review?(luke)
This patch removes FunctionBox::parent, which is dead.
Attachment #654877 - Flags: review?(luke)
Attachment #654873 - Flags: review?(luke) → review+
Attachment #654876 - Flags: review?(luke) → review+
Comment on attachment 654877 [details] [diff] [review]
(part 8) - Remove FunctionBox::parent.

nice
Attachment #654877 - Flags: review?(luke) → review+
Blocks: LazyBytecode
Thanks for the fast reviews, luke!

jimb, you're up next.
Comment on attachment 654113 [details] [diff] [review]
(part 1) - Remove FunctionBox::inLoop.

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

Splendid, splendid.
Attachment #654113 - Flags: review?(jimb) → review+
Comment on attachment 654114 [details] [diff] [review]
(part 2) - Remove FunctionBox::level.

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

They didn't stand a chance.
Attachment #654114 - Flags: review?(jimb) → review+
Comment on attachment 654115 [details] [diff] [review]
(part 3) - Remove FunctionBox::node.

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

They're dropping like flies!
Attachment #654115 - Flags: review?(jimb) → review+
Comment on attachment 654465 [details] [diff] [review]
(part 4) - Remove FunctionBox::inAnyDynamicScope.

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

Hurrah.
Attachment #654465 - Flags: review?(jimb) → review+
Try server run of all eight patches looks green: 
https://tbpl.mozilla.org/?tree=Try&rev=6c8ccb64dfbc
Depends on: 786114
You need to log in before you can comment on or make changes to this bug.