Last Comment Bug 685315 - Remove JSOP_GETGLOBAL
: Remove JSOP_GETGLOBAL
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla9
Assigned To: Terrence Cole [:terrence]
:
Mentors:
: 628735 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-09-07 14:27 PDT by David Anderson [:dvander]
Modified: 2015-10-08 09:04 PDT (History)
2 users (show)
emorley: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Minimal removal of getglobal. (12.05 KB, patch)
2011-09-10 16:58 PDT, Terrence Cole [:terrence]
dvander: feedback+
Details | Diff | Splinter Review
Remove the GETGLOBAL opcode (13.36 KB, patch)
2011-09-19 12:43 PDT, Terrence Cole [:terrence]
no flags Details | Diff | Splinter Review
Remove the GETGLOBAL opcode and orphaned code. (19.42 KB, patch)
2011-09-20 09:25 PDT, Terrence Cole [:terrence]
dvander: review+
Details | Diff | Splinter Review
Updated to this morning's tree. (19.45 KB, patch)
2011-09-21 10:43 PDT, Terrence Cole [:terrence]
no flags Details | Diff | Splinter Review

Description David Anderson [:dvander] 2011-09-07 14:27:02 PDT
This was a nice and simple way to get a perf boost in the interpreter and JM, but JSOP_GETGNAME has the same information and more. TI doesn't need it, JM technically doesn't, and IM won't either. We can just do a normal property lookup during compilation.
Comment 1 Terrence Cole [:terrence] 2011-09-10 16:58:04 PDT
Created attachment 559726 [details] [diff] [review]
Minimal removal of getglobal.

This is a minimal removal of GETGLOBAL.

Result: 1.004x slower execution on both SS and v8 with this patch applied.
Comment 2 David Anderson [:dvander] 2011-09-10 17:43:03 PDT
Comment on attachment 559726 [details] [diff] [review]
Minimal removal of getglobal.

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

Looks good to me, I think this is landable with just a bump to the XDR magic number in jsxdrapi.h.
Comment 3 Terrence Cole [:terrence] 2011-09-19 12:43:28 PDT
Created attachment 560987 [details] [diff] [review]
Remove the GETGLOBAL opcode

Addresses feedback by bumping XDR magic version number to 12.
Comment 4 David Anderson [:dvander] 2011-09-19 13:13:08 PDT
Comment on attachment 560987 [details] [diff] [review]
Remove the GETGLOBAL opcode

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

::: js/src/jsxdrapi.h
@@ +211,5 @@
>  #define JSXDR_MAGIC_SCRIPT_9        0xdead0009
>  #define JSXDR_MAGIC_SCRIPT_10       0xdead000a
>  #define JSXDR_MAGIC_SCRIPT_11       0xdead000b
> +#define JSXDR_MAGIC_SCRIPT_12       0xdead000c
> +#define JSXDR_MAGIC_SCRIPT_CURRENT  JSXDR_MAGIC_SCRIPT_12

Ah, sorry, this is the one you want to edit:

#define JSXDR_BYTECODE_VERSION      (0xb973c0de - 94)

Just bump the subtrahend there.

::: js/src/methodjit/Compiler.cpp
@@ -2754,5 @@
>            END_CASE(JSOP_UNBRANDTHIS)
>  
> -          BEGIN_CASE(JSOP_GETGLOBAL)
> -          BEGIN_CASE(JSOP_CALLGLOBAL)
> -            jsop_getglobal(GET_SLOTNO(PC));

Can you remove the implementation of jsop_getglobal too?
Comment 5 Terrence Cole [:terrence] 2011-09-20 09:25:53 PDT
Created attachment 561215 [details] [diff] [review]
Remove the GETGLOBAL opcode and orphaned code.

(In reply to David Anderson [:dvander] from comment #4)
> Comment on attachment 560987 [details] [diff] [review]
> Remove the GETGLOBAL opcode
> 
> Review of attachment 560987 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jsxdrapi.h
> @@ +211,5 @@
> >  #define JSXDR_MAGIC_SCRIPT_9        0xdead0009
> >  #define JSXDR_MAGIC_SCRIPT_10       0xdead000a
> >  #define JSXDR_MAGIC_SCRIPT_11       0xdead000b
> > +#define JSXDR_MAGIC_SCRIPT_12       0xdead000c
> > +#define JSXDR_MAGIC_SCRIPT_CURRENT  JSXDR_MAGIC_SCRIPT_12
> 
> Ah, sorry, this is the one you want to edit:
> 
> #define JSXDR_BYTECODE_VERSION      (0xb973c0de - 94)
> 
> Just bump the subtrahend there.

Yes indeed, and should have been quite clear from the comment.  That's what I get for skimming. :-)
 
> ::: js/src/methodjit/Compiler.cpp
> @@ -2754,5 @@
> >            END_CASE(JSOP_UNBRANDTHIS)
> >  
> > -          BEGIN_CASE(JSOP_GETGLOBAL)
> > -          BEGIN_CASE(JSOP_CALLGLOBAL)
> > -            jsop_getglobal(GET_SLOTNO(PC));
> 
> Can you remove the implementation of jsop_getglobal too?

Good catch!  There is quite a bit, in addition to this, that can be trimmed off easily now.  Less minimal patch with more -s attached.
Comment 6 Terrence Cole [:terrence] 2011-09-21 10:43:18 PDT
Created attachment 561516 [details] [diff] [review]
Updated to this morning's tree.

Updated to current tip.
Comment 7 Ed Morley [:emorley] 2011-09-22 04:34:16 PDT
In my queue with a few other bits that are being sent to try first and then onto inbound. Thanks for the nicely formatted patch and detailed commit message :-)
Comment 9 Ed Morley [:emorley] 2011-09-22 17:33:12 PDT
https://hg.mozilla.org/mozilla-central/rev/dfe8e0b73453
Comment 10 Terrence Cole [:terrence] 2011-09-23 10:45:23 PDT
Thanks for the checkin Ed!  It looks as if I may finally be getting the hang of the process.  I think I'll submit a few more patches, just to be sure.  :-)
Comment 11 Ed Morley [:emorley] 2011-09-23 17:26:14 PDT
(In reply to Terrence Cole [:terrence] from comment #10)
> I think I'll submit a few more patches, just to be sure. :-)

Hopefully more than a few! ;-)

Thanks for the patches! :-)
Comment 12 André Bargull 2015-10-08 08:57:55 PDT
*** Bug 628735 has been marked as a duplicate of this bug. ***

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