751818 - Remove DefineGlobal and lots of related stuff

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

Attachment: 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.

Comment 1

[Jason Orendorff [:jorendorff]]

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.)

Comment 2

[Nicholas Nethercote [inactive]]

Mmm, I think you're right.  I'll try this on Monday.

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: 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.

Comment 4

[Nicholas Nethercote [inactive]]

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.

Comment 5

[David Anderson [:dvander] - inactive, e-mail if emergency]

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.

Comment 6

[Jason Orendorff [:jorendorff]]

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.

Comment 7

[Nicholas Nethercote [inactive]]

Pushed with nits addressed:

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

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/84bc64571b96