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

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

Description User image Nicholas Nethercote [:njn] 2012-05-03 23:28:42 PDT
Created attachment 620974 [details] [diff] [review]

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 User image 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 User image Nicholas Nethercote [:njn] 2012-05-04 14:54:38 PDT
Mmm, I think you're right.  I'll try this on Monday.
Comment 3 User image Nicholas Nethercote [:njn] 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

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


 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 User image Nicholas Nethercote [:njn] 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 User image 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 User image 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 User image Nicholas Nethercote [:njn] 2012-05-09 17:24:34 PDT
Pushed with nits addressed:
Comment 8 User image Ed Morley [:emorley] 2012-05-10 08:04:21 PDT

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