685315 - Remove JSOP_GETGLOBAL

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

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

Attachment: 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] - inactive, e-mail if emergency]

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

Attachment: Remove the GETGLOBAL opcode

Addresses feedback by bumping XDR magic version number to 12.

Comment 4

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

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

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

Attachment: Updated to this morning's tree.

Updated to current tip.

Comment 7

[Ed Morley [:emorley]]

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 8

[Ed Morley [:emorley]]

https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=87ba0b8cbf99

https://hg.mozilla.org/integration/mozilla-inbound/rev/dfe8e0b73453

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/dfe8e0b73453

Comment 10

[Terrence Cole [:terrence]]

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

(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! :-)