Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Clear dead wood from FunctionBox

RESOLVED FIXED in mozilla17

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla17
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(8 attachments, 3 obsolete attachments)

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
(Assignee)

Description

5 years ago
This bug clears some dead wood from FunctionBox.
(Assignee)

Updated

5 years ago
Summary: Clear dead wood from FunctionBox. → Clear dead wood from FunctionBox
(Assignee)

Comment 1

5 years ago
Created attachment 654113 [details] [diff] [review]
(part 1) - Remove FunctionBox::inLoop.

This patch removes FunctionBox::inLoop, which is dead.
Attachment #654113 - Flags: review?(jimb)
(Assignee)

Comment 2

5 years ago
Created attachment 654114 [details] [diff] [review]
(part 2) - Remove FunctionBox::level.

This patch removes FunctionBox::level, which is dead.
Attachment #654114 - Flags: review?(jimb)
(Assignee)

Comment 3

5 years ago
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.
Attachment #654115 - Flags: review?(jimb)

Comment 4

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

Comment 5

5 years ago
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.
Attachment #654465 - Flags: review?(jimb)
(Assignee)

Comment 6

5 years ago
Created attachment 654499 [details] [diff] [review]
(part 5) - Rename some FunctionBox method args.

This patch just renames some args to make things clearer.
Attachment #654499 - Flags: review?(luke)
(Assignee)

Comment 7

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

Comment 8

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

Comment 9

5 years ago
Created attachment 654507 [details] [diff] [review]
(part 8) - Remove FunctionBox::parent.

FunctionBox::parent is now dead and can be removed.
Attachment #654507 - Flags: review?(luke)
(Assignee)

Comment 10

5 years ago
With all those patches in place, FunctionBox::{kids,siblings} and ParseContext::functionList are only needed for the recursivelySetStrictMode() call in Parser::setStrictMode()... hmm.
(Assignee)

Comment 11

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

Comment 12

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

Updated

5 years ago
Attachment #654504 - Attachment is obsolete: true
Attachment #654504 - Flags: review?(luke)
(Assignee)

Updated

5 years ago
Attachment #654507 - Attachment is obsolete: true
Attachment #654507 - Flags: review?(luke)

Updated

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

Comment 13

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

Comment 14

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

Comment 15

5 years ago
Created attachment 654877 [details] [diff] [review]
(part 8) - Remove FunctionBox::parent.

This patch removes FunctionBox::parent, which is dead.
Attachment #654877 - Flags: review?(luke)

Updated

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

Updated

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

Comment 16

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

nice
Attachment #654877 - Flags: review?(luke) → review+
(Assignee)

Updated

5 years ago
Blocks: 678037
(Assignee)

Comment 17

5 years ago
Thanks for the fast reviews, luke!

jimb, you're up next.

Comment 18

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

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

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

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

Comment 22

5 years ago
Try server run of all eight patches looks green: 
https://tbpl.mozilla.org/?tree=Try&rev=6c8ccb64dfbc
(Assignee)

Comment 23

5 years ago
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
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
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17

Updated

5 years ago
Depends on: 786114
You need to log in before you can comment on or make changes to this bug.