Last Comment Bug 751818 - Remove DefineGlobal and lots of related stuff
: Remove DefineGlobal and lots of related stuff
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla15
Assigned To: Nicholas Nethercote [:njn] (on vacation until July 11)
:
Mentors:
Depends on:
Blocks: 716843 UntangleFrontEnd 750282
  Show dependency treegraph
 
Reported: 2012-05-03 23:28 PDT by Nicholas Nethercote [:njn] (on vacation until July 11)
Modified: 2012-05-10 08:04 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (7.95 KB, patch)
2012-05-03 23:28 PDT, Nicholas Nethercote [:njn] (on vacation until July 11)
no flags Details | Diff | Review
patch, v2 (52.12 KB, patch)
2012-05-06 20:24 PDT, Nicholas Nethercote [:njn] (on vacation until July 11)
jorendorff: review+
dvander: feedback+
Details | Diff | Review

Description Nicholas Nethercote [:njn] (on vacation until July 11) 2012-05-03 23:28:42 PDT
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.
Comment 1 Jason Orendorff [:jorendorff] 2012-05-04 14:09:18 PDT
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 [:njn] (on vacation until July 11) 2012-05-04 14:54:38 PDT
Mmm, I think you're right.  I'll try this on Monday.
Comment 3 Nicholas Nethercote [:njn] (on vacation until July 11) 2012-05-06 20:24:18 PDT
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.
Comment 4 Nicholas Nethercote [:njn] (on vacation until July 11) 2012-05-06 20:29:24 PDT
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] 2012-05-08 16:37:03 PDT
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] 2012-05-09 08:22:17 PDT
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 [:njn] (on vacation until July 11) 2012-05-09 17:24:34 PDT
Pushed with nits addressed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/84bc64571b96
Comment 8 Ed Morley [:emorley] 2012-05-10 08:04:21 PDT
https://hg.mozilla.org/mozilla-central/rev/84bc64571b96

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