Last Comment Bug 784608 - Clear dead wood from FunctionBox
: Clear dead wood from FunctionBox
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla17
Assigned To: Nicholas Nethercote [:njn]
:
Mentors:
Depends on: 786114
Blocks: LazyBytecode
  Show dependency treegraph
 
Reported: 2012-08-21 23:50 PDT by Nicholas Nethercote [:njn]
Modified: 2012-08-27 18:36 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
(part 1) - Remove FunctionBox::inLoop. (2.21 KB, patch)
2012-08-21 23:51 PDT, Nicholas Nethercote [:njn]
jimb: review+
Details | Diff | Review
(part 2) - Remove FunctionBox::level. (3.17 KB, patch)
2012-08-21 23:53 PDT, Nicholas Nethercote [:njn]
jimb: review+
Details | Diff | Review
(part 3) - Remove FunctionBox::node. (11.61 KB, patch)
2012-08-21 23:54 PDT, Nicholas Nethercote [:njn]
jimb: review+
Details | Diff | Review
(part 4) - Remove FunctionBox::inAnyDynamicScope. (2.14 KB, patch)
2012-08-22 18:01 PDT, Nicholas Nethercote [:njn]
jimb: review+
Details | Diff | Review
(part 5) - Rename some FunctionBox method args. (2.81 KB, patch)
2012-08-22 20:13 PDT, Nicholas Nethercote [:njn]
luke: review+
Details | Diff | Review
(part 6) - Boolify ParseContext::innermostWith. (6.74 KB, patch)
2012-08-22 20:26 PDT, Nicholas Nethercote [:njn]
no flags Details | Diff | Review
(part 7) - Change meaning of FunctionBox::inWith. (2.31 KB, patch)
2012-08-22 20:34 PDT, Nicholas Nethercote [:njn]
no flags Details | Diff | Review
(part 8) - Remove FunctionBox::parent. (2.44 KB, patch)
2012-08-22 20:37 PDT, Nicholas Nethercote [:njn]
no flags Details | Diff | Review
(part 6) - Rename some parsing and emitting variables. (23.74 KB, patch)
2012-08-23 17:41 PDT, Nicholas Nethercote [:njn]
luke: review+
Details | Diff | Review
(part 7) - Change the form and meaning of ParseContext::innermostWith, and do follow-up simplifications. (7.80 KB, patch)
2012-08-23 17:44 PDT, Nicholas Nethercote [:njn]
luke: review+
Details | Diff | Review
(part 8) - Remove FunctionBox::parent. (2.43 KB, patch)
2012-08-23 17:45 PDT, Nicholas Nethercote [:njn]
luke: review+
Details | Diff | Review

Description Nicholas Nethercote [:njn] 2012-08-21 23:50:19 PDT
This bug clears some dead wood from FunctionBox.
Comment 1 Nicholas Nethercote [:njn] 2012-08-21 23:51:44 PDT
Created attachment 654113 [details] [diff] [review]
(part 1) - Remove FunctionBox::inLoop.

This patch removes FunctionBox::inLoop, which is dead.
Comment 2 Nicholas Nethercote [:njn] 2012-08-21 23:53:07 PDT
Created attachment 654114 [details] [diff] [review]
(part 2) - Remove FunctionBox::level.

This patch removes FunctionBox::level, which is dead.
Comment 3 Nicholas Nethercote [:njn] 2012-08-21 23:54:30 PDT
Created attachment 654115 [details] [diff] [review]
(part 3) - Remove FunctionBox::node.

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.
Comment 4 Luke Wagner [:luke] 2012-08-22 08:52:04 PDT
(In reply to Nicholas Nethercote [:njn] from comment #3)
Hah, nice.
Comment 5 Nicholas Nethercote [:njn] 2012-08-22 18:01:57 PDT
Created attachment 654465 [details] [diff] [review]
(part 4) - Remove FunctionBox::inAnyDynamicScope.

This patch removes FunctionBox::inAnyDynamicScope, which is dead.  That
allows FunctionBox::funHasExtensibleScope to be removed as well.
Comment 6 Nicholas Nethercote [:njn] 2012-08-22 20:13:13 PDT
Created attachment 654499 [details] [diff] [review]
(part 5) - Rename some FunctionBox method args.

This patch just renames some args to make things clearer.
Comment 7 Nicholas Nethercote [:njn] 2012-08-22 20:26:50 PDT
Created attachment 654503 [details] [diff] [review]
(part 6) - Boolify ParseContext::innermostWith.

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.
Comment 8 Nicholas Nethercote [:njn] 2012-08-22 20:34:24 PDT
Created attachment 654504 [details] [diff] [review]
(part 7) - Change meaning of FunctionBox::inWith.

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.
Comment 9 Nicholas Nethercote [:njn] 2012-08-22 20:37:56 PDT
Created attachment 654507 [details] [diff] [review]
(part 8) - Remove FunctionBox::parent.

FunctionBox::parent is now dead and can be removed.
Comment 10 Nicholas Nethercote [:njn] 2012-08-22 20:49:59 PDT
With all those patches in place, FunctionBox::{kids,siblings} and ParseContext::functionList are only needed for the recursivelySetStrictMode() call in Parser::setStrictMode()... hmm.
Comment 11 Nicholas Nethercote [:njn] 2012-08-22 21:51:49 PDT
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 12 Nicholas Nethercote [:njn] 2012-08-22 23:46:44 PDT
Comment on attachment 654503 [details] [diff] [review]
(part 6) - Boolify ParseContext::innermostWith.

Actually, I've got a better approach coming soon.
Comment 13 Nicholas Nethercote [:njn] 2012-08-23 17:41:25 PDT
Created attachment 654873 [details] [diff] [review]
(part 6) - Rename some parsing and emitting variables.

This patch renames some variables that are set during parsing/emitting, to
clarify their temporary nature.  This will help make the following patch 
clearer.
Comment 14 Nicholas Nethercote [:njn] 2012-08-23 17:44:21 PDT
Created attachment 654876 [details] [diff] [review]
(part 7) - Change the form and meaning of ParseContext::innermostWith, and do follow-up simplifications.

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.
Comment 15 Nicholas Nethercote [:njn] 2012-08-23 17:45:44 PDT
Created attachment 654877 [details] [diff] [review]
(part 8) - Remove FunctionBox::parent.

This patch removes FunctionBox::parent, which is dead.
Comment 16 Luke Wagner [:luke] 2012-08-23 18:20:07 PDT
Comment on attachment 654877 [details] [diff] [review]
(part 8) - Remove FunctionBox::parent.

nice
Comment 17 Nicholas Nethercote [:njn] 2012-08-23 18:42:14 PDT
Thanks for the fast reviews, luke!

jimb, you're up next.
Comment 18 Jim Blandy :jimb 2012-08-23 19:36:29 PDT
Comment on attachment 654113 [details] [diff] [review]
(part 1) - Remove FunctionBox::inLoop.

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

Splendid, splendid.
Comment 19 Jim Blandy :jimb 2012-08-23 19:37:23 PDT
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.
Comment 20 Jim Blandy :jimb 2012-08-23 19:56:26 PDT
Comment on attachment 654115 [details] [diff] [review]
(part 3) - Remove FunctionBox::node.

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

They're dropping like flies!
Comment 21 Jim Blandy :jimb 2012-08-23 19:57:22 PDT
Comment on attachment 654465 [details] [diff] [review]
(part 4) - Remove FunctionBox::inAnyDynamicScope.

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

Hurrah.
Comment 22 Nicholas Nethercote [:njn] 2012-08-26 15:53:59 PDT
Try server run of all eight patches looks green: 
https://tbpl.mozilla.org/?tree=Try&rev=6c8ccb64dfbc

Note You need to log in before you can comment on or make changes to this bug.