1093668 - Cleanup jsopcode.h

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

(1) All #define's could be replaced with an enum (or |static const uint32_t| integers).

(2) There are a bunch of macros like SET_UINT24 that should really be functions (see GET_UINT32_INDEX for an example), to avoid typical issues like in this case:

SET_UINT24(pc, index++)

(3) There's probably some dead code we can remove.

Comment 1

[Nicolas B. Pierron [:nbp]]

You can find the jsopcode.h [1] under js/src.  Using dxr.mozilla.org, you can navigate the code and find references of the Macros.

To build and check SpiderMonkey changes, follow the build documentation [2, 3], in case of issue feel free to ask on irc.mozilla.org #jsapi.

[1] http://dxr.mozilla.org/mozilla-central/source/js/src/jsopcode.h
[2] https://wiki.mozilla.org/JavaScript:New_to_SpiderMonkey
[3] https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey

Comment 2

[Rahul]

Attachment: Replaced most of the macros

Hi,

I am a total newbie at open source contribution, and this is my first patch ever.
Regarding the patch, I've changed most of the #define macros to either enumerated type or to functions. 
Actually, I did not get the third point regarding to dead code. Some help would be required for that.

Comment 3

[Jan de Mooij [:jandem]]

Comment on attachment 8518536 [details] [diff] [review]
Replaced most of the macros

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

Welcome :) Thank you for working on this, it looks great.

My comments below are mostly minor style nits. If you can post an updated patch addressing the comments I'll take another look. You can request review from me, the page below explains how this works:

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch#Getting_the_patch_reviewed

If you have any questions feel free to ask, here or on IRC (I'm jandem in #jsapi on Mozilla's IRC server).

::: js/src/jsopcode.h
@@ +32,4 @@
>  /*
>   * JS bytecode formats.
>   */
> +enum{

Nit: add a space between 'enum' and '{'

@@ +35,5 @@
> +enum{
> +    JOF_BYTE = 0,               /* single bytecode, no immediates */
> +    JOF_JUMP = 1,               /* signed 16-bit jump offset immediate */
> +    JOF_ATOM = 2,               /* unsigned 16-bit constant index */
> +    JOF_UINT16 = 3,             /* unsigned 16-bit immediate operand */

Nit: please format it like this:

>    JOF_BYTE        = 0,
>    JOF_JUMP        = 1,
>    JOF_ATOM        = 2,
>    JOF_UINT16      = 3,
>    JOF_TABLESWITCH = 4,

etc.

@@ +65,5 @@
> +    /* (1U<<11) is unused*/
> +    /* (1U<<12) is unused*/
> +    /* (1U<<13) is unused*/
> +    JOF_DETECTING = (1U<<14),   /* object detection for warning-quelling */
> +    /* (1U<<15) is unused*/ 

Nit: remove the trailing whitespace on this line

@@ +76,1 @@
>                                       JSOP_NEW, JSOP_EVAL */

Nit: fix the indentation of the second line

@@ +87,5 @@
> +    /* (1U<<24) is unused */
> +    JOF_GNAME = (1U<<25),       /* predicted global name */
> +    JOF_TYPESET = (1U<<26),     /* has an entry in a script's type sets */
> +    JOF_ARITH = (1U<<27)       /* unary or binary arithmetic opcode */
> +}; //Enum termination

Nit: I think you can remove the comment.

@@ +96,4 @@
>  
>  /* Shorthands for mode from format and mode from opcode. */
> +#define JOF_MODE(fmt) ((fmt) & JOF_MODEMASK)
> +#define JOF_OPMODE(op) JOF_MODE(js_CodeSpec[op].format)

Nit: please rewrite these macros as MOZ_ALWAYS_INLINE functions as well. |fmt| can be a uint32_t argument.

@@ +118,3 @@
>  #define UINT16_LEN              2
>  #define UINT16_HI(i)            ((jsbytecode)((i) >> 8))
>  #define UINT16_LO(i)            ((jsbytecode)(i))

Nit: I'd just remove these macros and inline them directly below. They are pretty trivial and the macros just complicate things.

@@ +135,1 @@
>  #define UINT16_LIMIT            ((unsigned)1 << 16)

This could be:

static const unsigned UINT16_LIMIT = 1 << 16;

@@ +174,3 @@
>  #define UINT24_HI(i)            ((jsbytecode)((i) >> 16))
>  #define UINT24_MID(i)           ((jsbytecode)((i) >> 8))
>  #define UINT24_LO(i)            ((jsbytecode)(i))

Same here, let's get rid of the macros and use the shift directly.

@@ +191,2 @@
>  
>  #define GET_INT8(pc)            (int8_t((pc)[1]))

Nit: please turn this into a function as well.

@@ +195,5 @@
> +static MOZ_ALWAYS_INLINE int32_t
> +GET_INT32(const jsbytecode *pc)
> +{
> +    return  (uint32_t((pc)[1]) << 24) | (uint32_t((pc)[2]) << 16) | 
> +            (uint32_t((pc)[3]) << 8)  |  (uint32_t((pc)[4]));

Nit: because it's a function now, we no longer need the parentheses around "pc". I'd write it like this:

> return (uint32_t(pc[1]) << 24) |
>        (uint32_t(pc[2]) << 16) |
>        (uint32_t(pc[3]) << 8) |
>        (uint32_t(pc[4]));

@@ +200,5 @@
> +}
> +
> +static MOZ_ALWAYS_INLINE void
> +SET_INT32(jsbytecode *pc, int32_t i)
> +{          

Nit: some trailing whitespace here as well. If you use mercurial, you can install the "color" extension: it will highlight trailing whitespace when you do a |hg diff| or something.

@@ +210,3 @@
>  
>  /* Index limit is determined by SN_4BYTE_OFFSET_FLAG, see frontend/BytecodeEmitter.h. */
>  #define INDEX_LIMIT_LOG2        31

we could rewrite these lines as:

static const uint32_t INDEX_LIMIT_LOG2 = 31;
static const uint32_t INDEX_LIMIT = uint32_t(1) << INDEX_LIMIT_LOG2;

And also rewrite the ARGC*, SCOPECOORD* macros below. Are you interested in fixing those too? :)

Comment 4

[Jeff Walden [:Waldo]]

(In reply to Jan de Mooij [:jandem] from comment #3)
> Nit: please format it like this:
> 
> >    JOF_BYTE        = 0,
> >    JOF_JUMP        = 1,
> >    JOF_ATOM        = 2,
> >    JOF_UINT16      = 3,
> >    JOF_TABLESWITCH = 4,

Given this breaks any time a longer enum is added, or the longest one is removed, I've been trying to get rid of these sorts of vertical alignments when I see them.  Superficially attractive nonsense, in my view.

> >  /* Shorthands for mode from format and mode from opcode. */
> > +#define JOF_MODE(fmt) ((fmt) & JOF_MODEMASK)
> > +#define JOF_OPMODE(op) JOF_MODE(js_CodeSpec[op].format)
> 
> Nit: please rewrite these macros as MOZ_ALWAYS_INLINE functions as well.
> |fmt| can be a uint32_t argument.

Just make it an inline function.  We shouldn't use MOZ_ALWAYS_INLINE unless there's some demonstrated reason to do so, because the (presumed) instantaneously-correct decision may not be correct over time.  We haven't demonstrated anything here, and these are simple enough I would expect them to be inlined basically no matter what.  I trust the compiler to get it right more often than the occasional human spot-check would (and the latter is what would happen if we tagged this MOZ_ALWAYS_INLINE).

Comment 5

[Rahul]

(In reply to Jan de Mooij [:jandem] from comment #3)
> Comment on attachment 8518536 [details] [diff] [review]
> Replaced most of the macros
> 
> Review of attachment 8518536 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Welcome :) Thank you for working on this, it looks great.
> 
> My comments below are mostly minor style nits. If you can post an updated
> patch addressing the comments I'll take another look. You can request review
> from me, the page below explains how this works:
> 
> https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/
> How_to_Submit_a_Patch#Getting_the_patch_reviewed

First of all thanks for reviewing the patch. I will follow the guidelines from next time. 

> > +#define JOF_MODE(fmt) ((fmt) & JOF_MODEMASK)
> > +#define JOF_OPMODE(op) JOF_MODE(js_CodeSpec[op].format)
> 
> Nit: please rewrite these macros as MOZ_ALWAYS_INLINE functions as well.
> |fmt| can be a uint32_t argument.

After changing the macro to function, I am getting the error "‘js_CodeSpec’ was not declared in this scope", as js_CodeSpec is declared below. So should I leave it as macro or take the definition of js_CodeSpec to top?

> And also rewrite the ARGC*, SCOPECOORD* macros below. Are you interested in
> fixing those too? :)

Yeah sure, I am working on it. Could you please assign the bug against my name.

Comment 6

[Mike Hoye [:mhoye]]

Hi, Rahul - Are you still making progress? If so, I'll assign the bug to you directlty.

Comment 7

[Rahul]

Right now, I am busy with my exams. If anyone else wants to work on it he/she may take it. Else, I'll be working on it post my exams.(In reply to Mike Hoye [:mhoye] from comment #6)
> Hi, Rahul - Are you still making progress? If so, I'll assign the bug to you
> directlty.

Hi Mike, right now, I am busy with my exams. If anyone else wants to work on it he/she may take it. Else, I'll be working on it post my exams.

Comment 8

[Mike Hoye [:mhoye]]

Thanks, Rahul - I'll leave it with you. Good luck with exams!

Comment 9

[Iskandar Rafikov]

Can I work on this bug?

Comment 10

[Mike Hoye [:mhoye]]

Hi, Iskandar - Thanks for your interest, but it looks like Rahul is on top of this bug. Can I ask you to take a look at bug 738549 - That bug is also a good first C++ bug, and it looks like it's lying idle right now.

Comment 11

[Iskandar Rafikov]

Sorry, It's solaris-specific bug. I don't have machine with this os installed.

Comment 12

[Rahul]

Hi Iskandar,
If you want to work on this bug, you can have it. If you find any difficulty in setting up things you can ask here.

Comment 13

[Mike Hoye [:mhoye]]

Rahul, would you like to take over as mentor for this bug?

Comment 14

[Rahul]

Thanks Mike, but I can't be mentoring this bug. As I've told, I am a newcomer here. If someone is working on this, I can help him/her with a few things, else I will be working on the patch myself.

Comment 15

[Ying Ruei Liang [:KK]]

Hi,
I'm new here and want to give this bug a try. Is there anyone works on this bug?

Comment 16

[Rahul]

Hi Ying,
Right now, I am working on this bug. I've almost created another patch, just some minor glitches. So I'll have to ask you to find another bug. You can have a look at bug 1104647. This is also good first bug.

Comment 17

[Rahul]

Attachment: B-1093668 P-2

I've tried to address most of the changes suggested in comment 3, but still, if there are any more nits, please let me know.

I am still in doubt about JOF_OPTYPE and JOF_OPMODE, as they are using js_CodeSpec which is declared below. I was getting build errors due to that. So, should I leave them as macros or take the definition of js_CodeSpec and corresponding structure above those macros or anything else which I am not aware of.

Sorry if the doubt is too silly. :)

Comment 18

[Jan de Mooij [:jandem]]

Comment on attachment 8538343 [details] [diff] [review]
B-1093668 P-2

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

Loosk great. A few more nits, if you can post an updated patch I'll r+ and land it.

::: js/src/jsopcode.h
@@ +41,1 @@
>  /* 5 is unused */

Nit: I'd indent this line too, same for 16, 20 and the other ones below.

@@ +105,4 @@
>  static MOZ_ALWAYS_INLINE uint8_t
>  GET_UINT8(jsbytecode *pc)
>  {
> +    return (uint8_t)pc[1];

Nit: prefer c++ cast: uint8_t(pc[1]);

And everywhere below.

@@ +113,5 @@
> +    pc[1] = (jsbytecode)u;
> +}
> +
> +/* Common uint16_t immediate format helpers. */
> +static const unsigned UINT16_LEN        = 2;

Nit: i'd move this down to before the UINT16_LIMIT definition.

@@ +126,5 @@
> +    return (jsbytecode)i;
> +}
> +
> +static MOZ_ALWAYS_INLINE unsigned
> +GET_UINT16(const jsbytecode *pc)

Nit: this function could return uint16_t right?

@@ +225,5 @@
> +
> +static MOZ_ALWAYS_INLINE void
> +SET_INT32(jsbytecode *pc, uint32_t i)
> +{
> +    pc[1] = (jsbytecode)(uint32_t(i) >> 24);

Nit: c++ cast: jsbytecode(...)

@@ +260,5 @@
> +}
> +static inline void
> +SET_ARGNO(jsbytecode *pc, uint16_t argno)
> +{
> +    SET_UINT16(pc,argno);

Nit: add a space between "," and argno, also a few times below.

Comment 19

[Jan de Mooij [:jandem]]

(In reply to Rahul from comment #17)
> I am still in doubt about JOF_OPTYPE and JOF_OPMODE, as they are using
> js_CodeSpec which is declared below. I was getting build errors due to that.
> So, should I leave them as macros or take the definition of js_CodeSpec and
> corresponding structure above those macros or anything else which I am not
> aware of.

I'd convert them to inline functions and move them after the js_CodeSpec definition. Let me know if that doesn't work :)

Comment 20

[Rahul]

Attachment: B-1093668 P-3

Thanks jandem. Does this patch seems fine to you? Do let me know if there are anymore changes required.

Comment 21

[Rahul]

As (In reply to Jan de Mooij [:jandem] from comment #19)
> I'd convert them to inline functions and move them after the js_CodeSpec
> definition. Let me know if that doesn't work :)

As the JSCodeSpec structure is using the function JOF_TYPE, this cannot be moved below js_CodeSpec definition. So should JOF_TYPE be moved above the structure's definition and leave the rest below the structure? My last patch would give build error due to that.

Comment 22

[Jan de Mooij [:jandem]]

Comment on attachment 8543162 [details] [diff] [review]
B-1093668 P-3

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

This looks really nice! I found a small bug and a few more nits (sorry, should have seen those last time!). If you can post an updated patch I'll land it :)

::: js/src/jsopcode.h
@@ +108,5 @@
>  
>  /* Common uint16_t immediate format helpers. */
> +
> +static inline uint16_t
> +UINT16_HI(uint16_t i)

Nit: these HI and LO functions can return jsbytecode, same for the UINT24_HI/MID/LO and ARGC_HI/LO ones.

@@ +276,5 @@
> +
> +static inline void
> +SET_LOCALNO(jsbytecode *pc, unsigned varno)
> +{
> +    SET_UINT16(pc, varno);

This should be SET_UINT24

@@ +363,5 @@
>  
> +/* Shorthands for type from format and type from opcode. */
> +
> +static inline uint32_t
> +JOF_TYPE(uint32_t fmt)

Good point about JSCodeSpec using JOF_TYPE! How about we move JOF_TYPE and JOF_MODE to their old location, and keep JOF_OPTYPE here?

@@ +369,5 @@
> +    return fmt & JOF_TYPEMASK;
> +}
> +
> +static inline uint32_t
> +JOF_OPTYPE(unsigned op)

Nit: this can be JSOp instead of unsigned.

@@ +383,5 @@
> +    return fmt & JOF_MODEMASK;
> +}
> +
> +static inline uint32_t
> +JOF_OPMODE(unsigned op)

Nit: this one isn't used anywhere so we can remove it :)

Comment 23

[Rahul]

Attachment: B-1093668 P-4

Comment 24

[Jan de Mooij [:jandem]]

Thanks for the updated patch! However if I apply it the shell no longer starts, it triggers an assertion failure. Will try to track it down...

Comment 25

[Jan de Mooij [:jandem]]

OK, found it (ARGNO_LIMIT should be UINT16_LIMIT instead of 2).

Comment 26

[Jan de Mooij [:jandem]]

Comment on attachment 8545931 [details] [diff] [review]
B-1093668 P-4

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

Looks great, thanks! I fixed the issue in comment 25 and pushed it to our Try server:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=47c77ae240f1

Once all tests pass on Linux64 I'll land it :)

Comment 27

[Jan de Mooij [:jandem]]

Pushed to inbound :)

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

Comment 28

[Carsten Book [:Tomcat]]

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