Remove JSOP_GETGLOBAL

RESOLVED FIXED in mozilla9

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
2 years ago

People

(Reporter: dvander, Assigned: terrence)

Tracking

Trunk
mozilla9
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

6 years ago
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.
(Assignee)

Comment 1

6 years ago
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.
Attachment #559726 - Flags: feedback?(dvander)
(Reporter)

Comment 2

6 years ago
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.
Attachment #559726 - Flags: feedback?(dvander) → feedback+
Assignee: general → terrence
Status: NEW → ASSIGNED
(Assignee)

Comment 3

6 years ago
Created attachment 560987 [details] [diff] [review]
Remove the GETGLOBAL opcode

Addresses feedback by bumping XDR magic version number to 12.
Attachment #559726 - Attachment is obsolete: true
Attachment #560987 - Flags: review?(dvander)
(Reporter)

Comment 4

6 years ago
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?
Attachment #560987 - Flags: review?(dvander)
(Assignee)

Comment 5

6 years ago
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.
Attachment #560987 - Attachment is obsolete: true
Attachment #561215 - Flags: review?(dvander)
(Reporter)

Updated

6 years ago
Attachment #561215 - Flags: review?(dvander) → review+
(Assignee)

Comment 6

6 years ago
Created attachment 561516 [details] [diff] [review]
Updated to this morning's tree.

Updated to current tip.
Attachment #561215 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
Version: unspecified → Trunk
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 :-)
Flags: in-testsuite-
Keywords: checkin-needed
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=87ba0b8cbf99

https://hg.mozilla.org/integration/mozilla-inbound/rev/dfe8e0b73453
Target Milestone: --- → mozilla9
https://hg.mozilla.org/mozilla-central/rev/dfe8e0b73453
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Assignee)

Comment 10

6 years ago
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.  :-)
(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! :-)

Updated

2 years ago
Duplicate of this bug: 628735
You need to log in before you can comment on or make changes to this bug.