Remove DefineGlobal and lots of related stuff

RESOLVED FIXED in mozilla15

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla15
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
Created attachment 620974 [details] [diff] [review]
patch

Parser.cpp has two functions called DefineGlobal and BindTopLevelVar.  They're pretty gross because they do some BytecodeEmitter-specific stuff inside the Parser;  i.e. the Parser needs to know about the BytecodeEmitter.  Hence the asBytecodeEmitter() calls.

They're not necessary for correctness.  And for SunSpider, V8 and Kraken, they make negligible difference to performance.

The comment above DefineGlobal says "The JSOP_GETGVAR, etc., ops speed up interpreted global variable access...", so I suspect they don't matter now that we have a JIT.

This patch removes them.  This means that Parser no longer needs to know anything about BytecodeEmitter.
Attachment #620974 - Flags: review?
(Assignee)

Updated

5 years ago
Attachment #620974 - Flags: review? → review?(dmandelin)
(Assignee)

Updated

5 years ago
Blocks: 750282
Hmm. Doesn't this patch mean nothing is being added to GlobalScope::defs anymore? If so, I think you can remove DefineGlobals from BytecodeCompiler.cpp, and GlobalScope::GlobalDef, and with a little effort you could remove GlobalScope too (I think the only fields in it that would be left alive would be globalObj and bce).

At least, that's how it looked to me. (Rather, I deleted a bunch more stuff than you have, and got it to compile and it seemed to be passing tests. I wasn't really paying close attention to what I was doing though.)
(Assignee)

Comment 2

5 years ago
Mmm, I think you're right.  I'll try this on Monday.
(Assignee)

Updated

5 years ago
Attachment #620974 - Flags: review?(dmandelin)
(Assignee)

Updated

5 years ago
Summary: Remove BytecodeEmitter-specific functions from Parser → Remove DefineGlobal and lots of related stuff
(Assignee)

Comment 3

5 years ago
Created attachment 621492 [details] [diff] [review]
patch, v2

That was a good observation, and led to quite the avalanche of dead code
removal.  I don't understand everything that I removed, I just followed my
nose.  Here's the sequence of things removed.

- With DefineGlobal gone, bce->globalScope->{defs,defsRoot} are never used.
  This allows the first loop in DefineGlobals to be removed, and the last
  part of the worklist loop in the same function.  Also, its |globalScope| arg
  can be removed.  At this point, DefineGlobal's functionality was reduced
  so much that I renamed it.

- With DefineGlobal gone, PND_GVAR is never set and can be removed.
  Definition::isGlobal() now always returns false and can be inlined
  everywhere.

- The inlining of Definition::isGlobal() renders BindKnownGlobal dead, and
  BindGlobal is reduced to a single pn_cookie.makeFree() call, which I
  inlined at the two callsites.

- BindKnownGlobal was the only caller of BytecodeEmitter::addGlobalUse, so
  it can be removed.  This allows BytecodeEmitter::global{Uses,Map} to be
  removed as well.

- BindKnownGlobal's removal also means that every field in GlobalScope
  can be removed except |globalObj|.  (This means that GlobalScope and
  BytecodeEmitter no longer point to each other, which is nice.)

- With BytecodeEmitter::globalUses gone, JSScript doesn't need to record
  globals, which allows GlobalSlotArray and a bunch of related stuff to be
  removed.  The XDR version number need not change because scripts with
  globals were never XDR'd.

Stats:

 8 files changed, 31 insertions(+), 487 deletions(-)

That's as far down the rabbit-hole as I was able to get, but I'd love to
hear about anything else that can be removed.
Attachment #620974 - Attachment is obsolete: true
Attachment #621492 - Flags: review?(jorendorff)
(Assignee)

Updated

5 years ago
Blocks: 716843
(Assignee)

Comment 4

5 years ago
Oh, I measured SunSpider with the JIT *off*, and this patch also made negligible difference in that case.  So this optimization really isn't paying its way.
(Assignee)

Updated

5 years ago
Attachment #621492 - Flags: feedback?(dvander)
Comment on attachment 621492 [details] [diff] [review]
patch, v2

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

Nice find - yeah, this has been dead or ineffective for ages. It was for JSOP_GETGLOBAL which turned out to be subsumed by JSOP_GETGNAME.
Attachment #621492 - Flags: feedback?(dvander) → feedback+
Comment on attachment 621492 [details] [diff] [review]
patch, v2

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

Ahhhhh. :)

::: js/src/frontend/BytecodeEmitter.cpp
@@ +3618,5 @@
>                      return false;
>                  if (!EmitIndex32(cx, JSOP_GETXPROP, atomIndex, bce))
>                      return false;
>              } else if (lhs->isOp(JSOP_SETGNAME)) {
> +                lhs->pn_cookie.makeFree();

Hmm. Shouldn't it already be free? I really don't know, but if so, assert instead. Same thing in EmitFunc.

::: js/src/frontend/Parser.cpp
@@ -1712,5 @@
> -    if (!outertc->inFunction() && bodyLevel && kind == Statement && outertc->compiling()) {
> -        JS_ASSERT(pn->pn_cookie.isFree());
> -        if (!DefineGlobal(pn, outertc->asBytecodeEmitter(), funName))
> -            return NULL;
> -    }

Could leave a JS_ASSERT_IF here.

@@ +2357,1 @@
>          pn->setOp((data->op == JSOP_DEFCONST) ? JSOP_SETCONST : JSOP_SETNAME);

Micro-nit: Note that the else-statement uses ?:. Suggest chained ?:?: instead of the if-statement.
Attachment #621492 - Flags: review?(jorendorff) → review+
(Assignee)

Comment 7

5 years ago
Pushed with nits addressed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/84bc64571b96

Comment 8

5 years ago
https://hg.mozilla.org/mozilla-central/rev/84bc64571b96
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
You need to log in before you can comment on or make changes to this bug.