784608 - Clear dead wood from FunctionBox

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

This bug clears some dead wood from FunctionBox.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Remove FunctionBox::inLoop.

This patch removes FunctionBox::inLoop, which is dead.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Remove FunctionBox::level.

This patch removes FunctionBox::level, which is dead.

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: (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]]

(In reply to Nicholas Nethercote [:njn] from comment #3)
Hah, nice.

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: (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 [inactive]]

Attachment: (part 5) - Rename some FunctionBox method args.

This patch just renames some args to make things clearer.

Comment 7

[Nicholas Nethercote [inactive]]

Attachment: (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 [inactive]]

Attachment: (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 [inactive]]

Attachment: (part 8) - Remove FunctionBox::parent.

FunctionBox::parent is now dead and can be removed.

Comment 10

[Nicholas Nethercote [inactive]]

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 [inactive]]

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 [inactive]]

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 [inactive]]

Attachment: (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 [inactive]]

Attachment: (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 [inactive]]

Attachment: (part 8) - Remove FunctionBox::parent.

This patch removes FunctionBox::parent, which is dead.

Comment 16

[Luke Wagner [:luke]]

Comment on attachment 654877 [details] [diff] [review]
(part 8) - Remove FunctionBox::parent.

nice

Comment 17

[Nicholas Nethercote [inactive]]

Thanks for the fast reviews, luke!

jimb, you're up next.

Comment 18

[Jim Blandy :jimb]

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]

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]

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]

Comment on attachment 654465 [details] [diff] [review]
(part 4) - Remove FunctionBox::inAnyDynamicScope.

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

Hurrah.

Comment 22

[Nicholas Nethercote [inactive]]

Try server run of all eight patches looks green: 
https://tbpl.mozilla.org/?tree=Try&rev=6c8ccb64dfbc

Comment 23

[Nicholas Nethercote [inactive]]

I don't think I've ever written eight patches without having a single modification suggested from reviews.  I must be working on things that are too easy :P

https://hg.mozilla.org/integration/mozilla-inbound/rev/49c35b696124
https://hg.mozilla.org/integration/mozilla-inbound/rev/fdc5717ca867
https://hg.mozilla.org/integration/mozilla-inbound/rev/52cf0f9db2f9
https://hg.mozilla.org/integration/mozilla-inbound/rev/12cca7e45cce
https://hg.mozilla.org/integration/mozilla-inbound/rev/45ac09b4bcb4
https://hg.mozilla.org/integration/mozilla-inbound/rev/878eb7f978b6
https://hg.mozilla.org/integration/mozilla-inbound/rev/660d18ddffcd
https://hg.mozilla.org/integration/mozilla-inbound/rev/d178a49c979c

Comment 24

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/49c35b696124
https://hg.mozilla.org/mozilla-central/rev/fdc5717ca867
https://hg.mozilla.org/mozilla-central/rev/52cf0f9db2f9
https://hg.mozilla.org/mozilla-central/rev/12cca7e45cce
https://hg.mozilla.org/mozilla-central/rev/45ac09b4bcb4
https://hg.mozilla.org/mozilla-central/rev/878eb7f978b6
https://hg.mozilla.org/mozilla-central/rev/660d18ddffcd
https://hg.mozilla.org/mozilla-central/rev/d178a49c979c