952777 - reorder JSJitInfo slots to pack better

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nathan Froyd [:froydnj]]

We have a lot of unnecessarily wasted space in JSJitInfo, even on
32-bit platforms.  Let's at least reorder the fields of JSJitInfo
to pack better.

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: reorder JSJitInfo slots to pack better

Prior to this, on Android, we had a layout like:

struct JSJitInfo {
	union {
		JSJitGetterOp      getter;               /*           4 */
		JSJitSetterOp      setter;               /*           4 */
		JSJitMethodOp      method;               /*           4 */
	};                                               /*     0     4 */
	uint32_t                   protoID;              /*     4     4 */
	uint32_t                   depth;                /*     8     4 */
	enum OpType                type;                 /*    12     4 */
	bool                       isInfallible;         /*    16     1 */
	bool                       isMovable;            /*    17     1 */

	/* XXX 2 bytes hole, try to pack */

	enum AliasSet              aliasSet;             /*    20     4 */
	bool                       isInSlot;             /*    24     1 */

	/* XXX 3 bytes hole, try to pack */

	size_t                     slotIndex;            /*    28     4 */
	enum JSValueType           returnType;           /*    32     1 */

	/* XXX 3 bytes hole, try to pack */

	const enum ArgType  *const argTypes;             /*    36     4 */
	JSParallelNative           parallelNative;       /*    40     4 */
private:


	/* size: 44, cachelines: 1, members: 12 */
	/* sum members: 36, holes: 3, sum holes: 8 */
	/* last cacheline: 44 bytes */
};

The numbers in the rightmost comments are the current byte offset and
the size of the current field, respectively.

After the patch, we have a layout like:

struct JSJitInfo {
	union {
		JSJitGetterOp      getter;               /*           4 */
		JSJitSetterOp      setter;               /*           4 */
		JSJitMethodOp      method;               /*           4 */
	};                                               /*     0     4 */
	uint32_t                   protoID;              /*     4     4 */
	uint32_t                   depth;                /*     8     4 */
	enum OpType                type;                 /*    12     4 */
	bool                       isInfallible;         /*    16     1 */
	bool                       isMovable;            /*    17     1 */
	bool                       isInSlot;             /*    18     1 */
	enum JSValueType           returnType;           /*    19     1 */
	enum AliasSet              aliasSet;             /*    20     4 */
	size_t                     slotIndex;            /*    24     4 */
	const enum ArgType  *const argTypes;             /*    28     4 */
	JSParallelNative           parallelNative;       /*    32     4 */
private:


	/* size: 36, cachelines: 1, members: 12 */
	/* last cacheline: 36 bytes */
};

which, you'll notice, has decreased our space usage by 8 bytes.  Doesn't
sound like much, but since we have 6000+ instances of these in
dom/bindings/, eliminating 8 bytes saves 50K of space, or about 3% of
the total .data usage for dom/bindings/.

Further improvements are coming, but they require more groundwork than
just reordering fields.

(WDYT about packing protoID/depth into one word, with a 24-bit protoID
and 8-bit depth?  And can we come up with *something* smarter than
wasting a parallelNative field for all but a small handful of operations
in the JIT?)

Will hand this off to bz once he gets back from vacation.

Comment 2

[Nicolas B. Pierron [:nbp]]

(In reply to Nathan Froyd (:froydnj) (AFKish through 3 January 2014) from comment #1)
> which, you'll notice, has decreased our space usage by 8 bytes.  Doesn't
> sound like much, but since we have 6000+ instances of these in
> dom/bindings/, eliminating 8 bytes saves 50K of space, or about 3% of
> the total .data usage for dom/bindings/.

Nice work.

> 
> struct JSJitInfo {
> 	union {
> 		JSJitGetterOp      getter;               /*           4 */
> 		JSJitSetterOp      setter;               /*           4 */
> 		JSJitMethodOp      method;               /*           4 */
> 	};                                               /*     0     4 */
> 	uint32_t                   protoID;              /*     4     4 */
> 	uint32_t                   depth;                /*     8     4 */

> 	enum OpType                type;                 /*    12     4 */
> 	bool                       isInfallible;         /*    16     1 */
> 	bool                       isMovable;            /*    17     1 */
> 	bool                       isInSlot;             /*    18     1 */
> 	enum JSValueType           returnType;           /*    19     1 */
> 	enum AliasSet              aliasSet;             /*    20     4 */

All these fields should be packed using bit fields.  These data are only used by the JIT and they are not critical in terms of access speed.

> 	size_t                     slotIndex;            /*    24     4 */

I do not expect us to generate slot indexes which are way above 1024, even for the window object.

> 	const enum ArgType  *const argTypes;             /*    28     4 */

This one could be reduced by making an index of JS function prototypes, and adding a function to recover the pointer.

> 	JSParallelNative           parallelNative;       /*    32     4 */
> private:
> 
> 
> 	/* size: 36, cachelines: 1, members: 12 */
> 	/* last cacheline: 36 bytes */
> };

Comment 3

[Nathan Froyd [:froydnj]]

(In reply to Nicolas B. Pierron [:nbp] from comment #2)
> (In reply to Nathan Froyd (:froydnj) (AFKish through 3 January 2014) from
> comment #1)
> > which, you'll notice, has decreased our space usage by 8 bytes.  Doesn't
> > sound like much, but since we have 6000+ instances of these in
> > dom/bindings/, eliminating 8 bytes saves 50K of space, or about 3% of
> > the total .data usage for dom/bindings/.
> 
> Nice work.

Thanks.

> > struct JSJitInfo {
> > 	union {
> > 		JSJitGetterOp      getter;               /*           4 */
> > 		JSJitSetterOp      setter;               /*           4 */
> > 		JSJitMethodOp      method;               /*           4 */
> > 	};                                               /*     0     4 */
> > 	uint32_t                   protoID;              /*     4     4 */
> > 	uint32_t                   depth;                /*     8     4 */
> 
> > 	enum OpType                type;                 /*    12     4 */
> > 	bool                       isInfallible;         /*    16     1 */
> > 	bool                       isMovable;            /*    17     1 */
> > 	bool                       isInSlot;             /*    18     1 */
> > 	enum JSValueType           returnType;           /*    19     1 */
> > 	enum AliasSet              aliasSet;             /*    20     4 */
> 
> All these fields should be packed using bit fields.  These data are only
> used by the JIT and they are not critical in terms of access speed.
> 
> > 	size_t                     slotIndex;            /*    24     4 */
> 
> I do not expect us to generate slot indexes which are way above 1024, even
> for the window object.

I have another patch to reduce the space of OpType and AliasSet to 1 byte each and decrease |slotIndex| to |uint16_t| for another 8 bytes of savings.  I suppose we could squash |isInfallible|, |isMovable|, |isInSlot|, and |slot_index| together for another 4 bytes of savings.  (That'd be ~75K more space savings across the bindings.)

Squashing the enums into bitfields would be nice, too.

> > 	const enum ArgType  *const argTypes;             /*    28     4 */
> 
> This one could be reduced by making an index of JS function prototypes, and
> adding a function to recover the pointer.

That would be great, especially because we wouldn't have to relocate the pointer anymore.  But I don't see a good way to inform the JIT about where that array of function prototypes is stored.  It would also require some gymnastics to make that work across all the bindings.

> > 	JSParallelNative           parallelNative;       /*    32     4 */

Would it make sense to do something like:

struct JSJitInfo {
    ...
    uint8_t flags;  /* Has a bit set if is a JSParallelJitInfo structure */
    ...
};

struct JSParallelJitInfo : public JSJitInfo {
    JSParallelNative parallelNative;
};

?

That would save a lot of space for the common case of the bindings.

Comment 4

[Nicolas B. Pierron [:nbp]]

(In reply to Nathan Froyd (:froydnj) (AFKish through 3 January 2014) from comment #3)
> (In reply to Nicolas B. Pierron [:nbp] from comment #2)
> > (In reply to Nathan Froyd (:froydnj) (AFKish through 3 January 2014) from
> > comment #1)
> > > 	const enum ArgType  *const argTypes;             /*    28     4 */
> > 
> > This one could be reduced by making an index of JS function prototypes, and
> > adding a function to recover the pointer.
> 
> That would be great, especially because we wouldn't have to relocate the
> pointer anymore.  But I don't see a good way to inform the JIT about where
> that array of function prototypes is stored.  It would also require some
> gymnastics to make that work across all the bindings.

We could register this list of prototype during the initialization of the JSRuntime.

> > > 	JSParallelNative           parallelNative;       /*    32     4 */
> 
> Would it make sense to do something like:
> 
> struct JSJitInfo {
>     ...
>     uint8_t flags;  /* Has a bit set if is a JSParallelJitInfo structure */
>     ...
> };
> 
> struct JSParallelJitInfo : public JSJitInfo {
>     JSParallelNative parallelNative;
> };
> 
> ?
> 
> That would save a lot of space for the common case of the bindings.

Yes, it would make sense, as I guess most bindings are only sequential.

Also, I am not sure if this is still true or not, but I read a comment in the source which suggested that Windows' compiler does not pack bit fields if the you have different types.

Comment 5

[Eric Faust [:efaust]]

(In reply to Nathan Froyd (:froydnj) (AFKish through 3 January 2014) from comment #3)
> Would it make sense to do something like:
> 
> struct JSJitInfo {
>     ...
>     uint8_t flags;  /* Has a bit set if is a JSParallelJitInfo structure */
>     ...
> };
> 
> struct JSParallelJitInfo : public JSJitInfo {
>     JSParallelNative parallelNative;
> };
> 
> ?
> 
> That would save a lot of space for the common case of the bindings.

Not only would this improve the sizes in the common case. It also separates the two wildly different use cases in the type system, which can never be bad. Note that we've already kind of done this with isDOMJitinfo().

Barring that, is JSParallelNative way more complicated than I think it is (A quick mxr search shows a typedef for a function pointer)? If that's the case, then why is it not in the union with the other function pointers? We can at least make them the same size. The parallel case should never use that union field, as far as I know, and the OpType field will keep us from doing anything incorrect with that field in the JIT from the DOM point of view.

Comment 6

[Nathan Froyd [:froydnj]]

(In reply to Eric Faust [:efaust] from comment #5)
> Barring that, is JSParallelNative way more complicated than I think it is (A
> quick mxr search shows a typedef for a function pointer)? If that's the
> case, then why is it not in the union with the other function pointers? We
> can at least make them the same size. The parallel case should never use
> that union field, as far as I know, and the OpType field will keep us from
> doing anything incorrect with that field in the JIT from the DOM point of
> view.

This is actually something that I was confused about when reading the code.  You have things like JS_JITINFO_NATIVE_PARALLEL:

http://mxr.mozilla.org/mozilla-central/source/js/src/jsfriendapi.h#1547

which uses OpType_None (?)...but the documentation for the |parallelNative| field makes it sound like it's some sort of additional way to do parallel operations.  But then why OpType_None?  I guess the use case is if you had some sort of high-level sequence/array operation like |reduce|?  Some clarification here would be nice.

Comment 7

[Nathan Froyd [:froydnj]]

(In reply to Nathan Froyd (:froydnj) (AFKish through 3 January 2014) from comment #6)
> Some clarification here would be nice.

In particular, can we have methods that also have parallel implementations or is that not a reasonable thing to talk about?  Eliminating |argTypes| for getters and setters seems useful.

Comment 8

[Nathan Froyd [:froydnj]]

Attachment: part 2 - reorder and explicitly size enums in JSJitInfo for better packing

This is the aforementioned patch for properly-sized enums.  The layout
now looks like:

struct JSJitInfo {
	union {
		JSJitGetterOp      getter;               /*           4 */
		JSJitSetterOp      setter;               /*           4 */
		JSJitMethodOp      method;               /*           4 */
	};                                               /*     0     4 */
	uint32_t                   protoID;              /*     4     4 */
	uint32_t                   depth;                /*     8     4 */
	enum OpType                type;                 /*    12     1 */
	enum AliasSet              aliasSet;             /*    13     1 */
	uint16_t                   slotIndex;            /*    14     2 */
	bool                       isInfallible;         /*    16     1 */
	bool                       isMovable;            /*    17     1 */
	bool                       isInSlot;             /*    18     1 */
	enum JSValueType           returnType;           /*    19     1 */
	const enum ArgType  *const argTypes;             /*    20     4 */
	JSParallelNative           parallelNative;       /*    24     4 */
private:


	/* size: 28, cachelines: 1, members: 12 */
	/* last cacheline: 28 bytes */
};

Comment 9

[Boris Zbarsky [:bzbarsky, bz on IRC]]

> and they are not critical in terms of access speed.

As long as startup intialization is fast, at least.

I agree that putting in the parallelNative with the existing ops is probably a good idea given how it's used.  However, note that in the long term we will want to be able to have some sort of setup where there's an op (e.g. a setter) but also one or more specialized ops based on the arg types (e.g. we want that for canvas fillStyle setters). I think the right way to go for that sort of thing will be a bit in the JSJitInfo indicating it has more members and then a subclass that has those members.

In fact, we can do the same thing with argTypes; in the common case argTypes is null, so we could have a subclass plus bit setup there too.

Comment 10

[Nathan Froyd [:froydnj]]

Attachment: part 1 - reorder JSJitInfo slots to pack better

Comment 11

[Nathan Froyd [:froydnj]]

Attachment: part 2 - use explicitly typed enums to shrink JSJitInfo further

Comment 12

[Nathan Froyd [:froydnj]]

Attachment: part 3 - use bitfields for integer fields in JSJitInfo

Comment 13

[Nathan Froyd [:froydnj]]

Attachment: part 4 - move JSParallelNative into the union

Comment 14

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Comment on attachment 8356801 [details] [diff] [review]
part 1 - reorder JSJitInfo slots to pack better

Please move the two XXXbz comments with isInSlot, since they belong with that member...

Comment 15

[Nathan Froyd [:froydnj]]

Attachment: part 5 - give JSJitInfo proper constructors

Comment 16

[Nathan Froyd [:froydnj]]

Attachment: part 6 - move JSJitInfo::argTypes to a separate JSTypedMethodJitInfo subclass

Comment 17

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Comment on attachment 8356807 [details] [diff] [review]
part 4 - move JSParallelNative into the union

>+        return isNative() && jitInfo() && !!jitInfo()->hasParallelNative();

Kill the !!?

Comment 18

[Nathan Froyd [:froydnj]]

The attached patches get us to:

struct JSJitInfo {
	union Op                   op;                   /*     0     4 */
	uint32_t                   protoID;              /*     4     4 */
	uint32_t                   depth;                /*     8     4 */
	enum OpType                type;                 /*    12     1 */
	enum JSValueType           returnType;           /*    13     1 */
	uint16_t                   isInfallible:1;       /*    14:15  2 */
	uint16_t                   isMovable:1;          /*    14:14  2 */
	uint16_t                   isInSlot:1;           /*    14:13  2 */
	uint16_t                   slotIndex:13;         /*    14: 0  2 */
	enum AliasSet              aliasSet;             /*    16     1 */

	/* XXX 1 byte hole, try to pack */

	enum Flags                 flags;                /*    18     2 */
private:


	/* size: 20, cachelines: 1, members: 11 */
	/* sum members: 19, holes: 1, sum holes: 1 */
	/* last cacheline: 20 bytes */
};

Since we have a little over 7K JSJitInfos defined by the bindings code, these changes result in a ~170Kish space savings.

I will try to sort out who should review what later tonight or early tomorrow morning, and provide rationales for things like parts 4 and 5; the other parts should be pretty straightforward.

Comment 19

[Nathan Froyd [:froydnj]]

(In reply to Nathan Froyd (:froydnj) from comment #18)
> Since we have a little over 7K JSJitInfos defined by the bindings code,
> these changes result in a ~170Kish space savings.

...on ARM Android; I'd expect x86 and B2G to be similar.  I haven't looked at what happens on x86-64, but I'm less worried about the effects there.

Comment 20

[Nathan Froyd [:froydnj]]

Comment on attachment 8356805 [details] [diff] [review]
part 3 - use bitfields for integer fields in JSJitInfo

I didn't try to line up everything here for 64-bit and non-MOZ_ENUM_TYPE compilers (mainly B2G, sadly).  I could try to do that in a followup.

Comment 21

[Nathan Froyd [:froydnj]]

Comment on attachment 8356807 [details] [diff] [review]
part 4 - move JSParallelNative into the union

I know moving |parallelNative| into its separate subclass was mentioned, but I think this is less invasive and more appropriate for how OpType_None is currently used.  Creating a subclass down the road should not be difficult.

Comment 22

[Nathan Froyd [:froydnj]]

Comment on attachment 8356810 [details] [diff] [review]
part 5 - give JSJitInfo proper constructors

I originally tried just making JSTypedMethodJitInfo a struct that inherited from JSJitInfo and had an extra field.  You need a constructor to be able to brace-initialize such structures.  (JSJitInfo didn't need a constructor previously because it was an aggregate, which has special rules for initialization; JSTypedMethodJitInfo would not be an aggregate because it inherits from another class.)

And if JSTypedMethodJitInfo has a constructor, then JSJitInfo needs a constructor.  And if JSJitInfo has a constructor, then the union it contains needs a constructor, so it can be initiated from JSJitInfo's structure.

And so you have the patch you see before you.  While the OpType initializations didn't strictly need to be moved into the constructors, I think they are an improvement, as OpType/Op consistency is one less thing to think about.  Dispensing with weird casts is also, IMHO, an improvement.  (One could imagine dispensing with other fields based on the constructor, e.g. eliminating the necessity for the slot* bits for JSParallelNatives, etc., but I'm not sure that's worth the bindings codegen complications...)

Comment 23

[Nathan Froyd [:froydnj]]

Comment on attachment 8356811 [details] [diff] [review]
part 6 - move JSJitInfo::argTypes to a separate JSTypedMethodJitInfo subclass

Finally, the interesting bits.  Pretty straightforward.

Comment 24

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Comment on attachment 8356801 [details] [diff] [review]
part 1 - reorder JSJitInfo slots to pack better

See comment 14.  r=me with that fixed.

It might be nice to change the Python there to replace with a dict, so that it's not order-dependent like this if we reorder things in the future.  Followup is fine for that.

Comment 25

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Comment on attachment 8356805 [details] [diff] [review]
part 3 - use bitfields for integer fields in JSJitInfo

r=me, though we might be able to squeeze the alias set, optype, and returnType into 2 bytes instead of the 3 (effectively 4) bytes they'll end up taking when you're done, I think...

Comment 26

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Comment on attachment 8356807 [details] [diff] [review]
part 4 - move JSParallelNative into the union

It might make sense to rename OpType_None to ParallelNative.

r=me with that and the nit from comment 17.

Comment 27

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Comment on attachment 8356810 [details] [diff] [review]
part 5 - give JSJitInfo proper constructors

The MOZ_CONSTEXPR bits are what makes this not generate static ctors for the brace-init, I assume?

Would it be simpler to just make JSTypedMethodJitInfo be a POD struct whose first member is a JSJitInfo and skip doing all the constructor bits?  We'd need a reinterpret_cast to go between them, instead of a static_cast, but apart from that...

Comment 28

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Comment on attachment 8356811 [details] [diff] [review]
part 6 - move JSJitInfo::argTypes to a separate JSTypedMethodJitInfo subclass

Why do we need a 16-bit flag enum to represent a boolean state?  Please just steal another bit from the slot index instead.

Comment 29

[Boris Zbarsky [:bzbarsky, bz on IRC]]

If we do take the ctor approach, I do like its type-safety properties.  We should then do a followup to simplify the ways we initialize jitinfos to pass in the fewest number or args applicable.

Comment 30

[Nathan Froyd [:froydnj]]

(In reply to Boris Zbarsky [:bz] from comment #25)
> r=me, though we might be able to squeeze the alias set, optype, and
> returnType into 2 bytes instead of the 3 (effectively 4) bytes they'll end
> up taking when you're done, I think...

(In reply to Boris Zbarsky [:bz] from comment #28)
> Why do we need a 16-bit flag enum to represent a boolean state?  Please just
> steal another bit from the slot index instead.

I used the flag enum because we had the space available anyway (after part 3, there was a 3-byte hole at the end of the structure) and because comment 9 suggests we may need more flags someday.

I guess we could abandon enums entirely inside the struct and have something like:

  uint32_t type : 4;         // OpType, properly typed through an accessor.
  uint32_t aliasSet : 4;     // AliasSet, properly typed through an accessor.
  uint32_t returnType : 8;   // JSValueType, properly typed through an accessor.
  uint32_t isInfallible : 1;
  uint32_t isMovable : 1;
  uint32_t isInSlot : 1;
  uint32_t isTypedMethod : 1;
  uint32_t slotIndex : 12;

And then we could steal bits from |type| and |aliasSet| if we came up with other flags.  That would get us down to 16 bytes per jitinfo and make it easier to reason about space consumption on platforms that don't support MOZ_ENUM_TYPE.

Comment 31

[Nathan Froyd [:froydnj]]

(In reply to Boris Zbarsky [:bz] from comment #27)
> The MOZ_CONSTEXPR bits are what makes this not generate static ctors for the
> brace-init, I assume?

Yes.  (I never tried without MOZ_CONSTEXPR, but I wouldn't expect that removing it would have good effects.)

> Would it be simpler to just make JSTypedMethodJitInfo be a POD struct whose
> first member is a JSJitInfo and skip doing all the constructor bits?  We'd
> need a reinterpret_cast to go between them, instead of a static_cast, but
> apart from that...

I think that would be simpler, but I like the safety gained from using constructors.

One thing I haven't done is make sure this all compiles elsewhere (particularly MSVC); I'm assuming the ctor approach works there, but if it doesn't, POD types may win by default.

Comment 32

[Boris Zbarsky [:bzbarsky, bz on IRC]]

> because comment 9 suggests we may need more flags someday

I think in practice the setup comment 9 describes would just use the typed method flag.

I think the setup you describe in comment 30, combined with static asserts that the number of bits we assign to type and aliasSet is big enough to hold their values, is the way to go (and I assume you meant stealing bits from returnType and slotIndex if we need more flags, right?).

One other space-saving measure we can consider as a followup (because this bug is getting pretty involved): in practice depth is never too big (e.g. right now the maximum value is 8 or so) and protoID is also not too big (under 1000 so far).  So we could make them both uint16 with no trouble and have codegen spit out static asserts that the max proto id and depth (which codegen knows) fit in those fields.  That should let us save 4 more bytes easily.

Comment 33

[Nathan Froyd [:froydnj]]

(In reply to Boris Zbarsky [:bz] from comment #32)
> > because comment 9 suggests we may need more flags someday
> 
> I think in practice the setup comment 9 describes would just use the typed
> method flag.

Aha.  OK, that makes sense.

> I think the setup you describe in comment 30, combined with static asserts
> that the number of bits we assign to type and aliasSet is big enough to hold
> their values, is the way to go (and I assume you meant stealing bits from
> returnType and slotIndex if we need more flags, right?).

Well, the structure in comment 30 over-allocates space for |type| and |aliasSet|, so I did meant to steal from |type| and |aliasSet|.  Stealing from |slotIndex| is fine too.  I'd rather not steal from |returnType| unless we really need to, since the other three fields are more closely associated with JSJitInfo, and stealing from |returnType| requires some fiddling with JSValueType.

Whatever we choose, I think we a) have enough bits to steal for followups, and b) need a big fat comment describing what's going on.

Comment 34

[Eric Faust [:efaust]]

Comment on attachment 8356801 [details] [diff] [review]
part 1 - reorder JSJitInfo slots to pack better

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

Looks Good. Is it worth trying to add some documentation to keep these from getting sparse again as people blindly add new fields?

Comment 35

[Eric Faust [:efaust]]

Comment on attachment 8356802 [details] [diff] [review]
part 2 - use explicitly typed enums to shrink JSJitInfo further

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

::: js/src/jsfriendapi.h
@@ +1525,5 @@
>                                 That's covered by argTypes and runtime analysis
>                                 of the actual argument types being passed in. */
>      // XXXbz should we have a JSGetterJitInfo subclass or something?
>      // XXXbz should we have a JSValueType for the type of the member?
> +    uint16_t slotIndex;     /* If isInSlot is true, the index of the slot to get

This is probably OK. We can maybe even go smaller?

Boris, we can only do this optimization out of fixed slots, right? So we can probably fit this in a single byte?

Comment 36

[Nicholas Nethercote [:njn]]

> Is it worth trying to add some documentation to keep these from
> getting sparse again as people blindly add new fields?

A static assertion would be even better, if it's possible!

Comment 37

[Eric Faust [:efaust]]

Comment on attachment 8356805 [details] [diff] [review]
part 3 - use bitfields for integer fields in JSJitInfo

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

::: js/src/jsfriendapi.h
@@ +1518,5 @@
> +                                  not be enough (e.g. in cases when it can
> +                                  throw). */
> +    uint16_t isInSlot : 1;     /* True if this is a getter that can get a member
> +                                  from a slot of the "this" object directly. */
> +    uint16_t slotIndex : 13;   /* If isInSlot is true, the index of the slot to

Oh, you're way ahead of me :)

Comment 38

[Boris Zbarsky [:bzbarsky, bz on IRC]]

> Boris, we can only do this optimization out of fixed slots, right? 

For now yes, though I have some patches floating around that would allow it in general for reserved slots.

But in any case, we don't support more than about 10 bits worth of slot index for reserved slots... not like anything in the JS engine enforces that; we just get silent fail, iirc.  :(

Comment 39

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Comment on attachment 8356810 [details] [diff] [review]
part 5 - give JSJitInfo proper constructors

>+                # We need the cast here because JSJitGetterOp has a "void* self"
>+                # while we have the right type.
>+                setter = "(JSJitSetterOp)set_%s" % self.member.identifier.name

Comment should talk about JSJitSetterOp.

I wish there were a way to not copy/paste the ctors so much, but I don't know a way.

And as you probably discovered on try, layout builds with -Wshadow -Werror, so you need to not have shadowing stuff here.  :(

r=me with the comment fixed and once this builds.

Comment 40

[Eric Faust [:efaust]]

Comment on attachment 8356810 [details] [diff] [review]
part 5 - give JSJitInfo proper constructors

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

I am not sure I fully understand what we even win from doing this. Can you elaborate on the motivations? More type safety?

::: dom/bindings/Codegen.py
@@ +6361,5 @@
>              if (not self.member.readonly or
>                  self.member.getExtendedAttribute("PutForwards") is not None or
>                  self.member.getExtendedAttribute("Replaceable") is not None):
>                  setterinfo = ("%s_setterinfo" % self.member.identifier.name)
> +                # We need the cast here because JSJitGetterOp has a "void* self"

nit: incorrect type in comments.

Comment 41

[Eric Faust [:efaust]]

Comment on attachment 8356811 [details] [diff] [review]
part 6 - move JSJitInfo::argTypes to a separate JSTypedMethodJitInfo subclass

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

I think we can also even go further with this. Does it make sense to split out the things that matter to only various combinations of optypes? How far do we want to push things?

No change in review status. I want to hear more on this topic.

::: js/src/jsfriendapi.h
@@ +1493,5 @@
>          // in the system.  Most things fall in this bucket.
>          AliasEverything
>      };
>  
> +    enum Flags MOZ_ENUM_TYPE(uint16_t) {

I also find this strange. We can steal more bits than we are likely to have derivative types of JitInfo. If we desperately need to split this out later, we can.

Comment 42

[Nathan Froyd [:froydnj]]

Attachment: part 5a - rename JSJitInfo's members to avoid -Wshadow warnings in layout/

This is the bit that's required to make everything compile on releng
infrastructure, since we have to respect -Wshadow.  I'll roll this into
part 5 for committing.

Comment 43

[Nathan Froyd [:froydnj]]

Attachment: part 6 - move JSJitInfo::argTypes to a separate JSTypedMethodJitInfo subclass

This version steals a bit from |slotIndex| for an |isTypedMethod| flag, as
suggested by bz in review.

I can implement the more fine-grained bitfield scheme discussed earlier to
replace parts 2 and 3, but I'd kind of like to move that to a followup bug.
One reason is because this patch series is somewhat large; a more selfish
reason is that doing that to parts 2 and 3 results in rebase pain for
everything else.  I'll do it if people feel it's clearer, though.

I think any sort of "only specify these relevant fields for this JitInfo
subkind" work should be a followup bug.

Comment 44

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Comment on attachment 8358184 [details] [diff] [review]
part 5a - rename JSJitInfo's members to avoid -Wshadow warnings in layout/

I guess, but this uglifies up all the users pretty badly.

Worse yet, we can't add pretty accessors, because _those_ would be shadowed too.  :(

Maybe we can just suffix "Arg" onto all the ctor arguments instead?

r=me either way...

Comment 45

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Comment on attachment 8358185 [details] [diff] [review]
part 6 - move JSJitInfo::argTypes to a separate JSTypedMethodJitInfo subclass

r=me, though those XXX comments should still get moved... followup is ok.

Comment 46

[Eric Faust [:efaust]]

Comment on attachment 8358185 [details] [diff] [review]
part 6 - move JSJitInfo::argTypes to a separate JSTypedMethodJitInfo subclass

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

r=me.

I am also OK with a followup.

Comment 47

[Eric Faust [:efaust]]

(In reply to Boris Zbarsky [:bz] from comment #44)
> Maybe we can just suffix "Arg" onto all the ctor arguments instead?
> 

yeah, this is more traditional, and will keep things much cleaner. I think we should do that, but it's not a blocking issue, really.

Comment 48

[Nathan Froyd [:froydnj]]

(In reply to Eric Faust [:efaust] from comment #47)
> (In reply to Boris Zbarsky [:bz] from comment #44)
> > Maybe we can just suffix "Arg" onto all the ctor arguments instead?
> > 
> 
> yeah, this is more traditional, and will keep things much cleaner. I think
> we should do that, but it's not a blocking issue, really.

We could just prefix them all with "a"... ;)

I'll suffix "Arg", that's an easier change to make and won't churn a bunch of stuff.

Comment 49

[Boris Zbarsky [:bzbarsky, bz on IRC]]

> We could just prefix them all with "a"... ;)

froydnj++, would review again!  ;)

Comment 50

[Nathan Froyd [:froydnj]]

Ah, MSVC, a little behind on implementing C++:

https://tbpl.mozilla.org/php/getParsedLog.php?id=32834797&tree=Try

Guess we have to do things the old-school C-style way.

Comment 51

[Nathan Froyd [:froydnj]]

Attachment: part 5 - move JSJitInfo::argTypes to a separate JSTypedMethodJitInfo subclass

So...a try push indicated that MSVC doesn't like brace-initialized non-aggregate
objects; that must be a C++ version $NEW feature that MSVC doesn't support yet.
And thinking about it some more made me realize that B2G's compiler doesn't
support constexpr, so all these structures are going to be creating static
constructors, the size of which would probably swamp any savings we might obtain
from trimming bits.

Thus the following somewhat ugly patch.  I don't particularly like doing things
the old-school C way, but I don't think we have much choice in this instance.
On the plus side, no more fiddling around with -Wshadow workarounds.

This is green on Try, except that the B2G ICS builds don't seem to like
something about the way JS_JITINFO_NATIVE_PARALLEL is defined.  Which is
mystifying to me, as the B2G JB builds are just fine with it and I expect that
they are using the same compiler...?  Going to try to talk to the B2G folks
about that one.

Comment 52

[Eric Faust [:efaust]]

Comment on attachment 8359390 [details] [diff] [review]
part 5 - move JSJitInfo::argTypes to a separate JSTypedMethodJitInfo subclass

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

Looks good to me!

::: js/src/jsfriendapi.h
@@ +1505,5 @@
>      }
>  
> +    bool isTypedMethodJitInfo() const
> +    {
> +        return isTypedMethod != 0;

hyper nit: is |return isTypedMethod;| different for some reason? It seems stylistically better somehow.

@@ +1540,5 @@
>                                 alias set; in particular for a method it does not
>                                 include whatever argument conversions might do.
>                                 That's covered by argTypes and runtime analysis
>                                 of the actual argument types being passed in. */
>      // XXXbz should we have a JSGetterJitInfo subclass or something?

this XXX can probably go away. Bz would know for sure.

@@ +1579,2 @@
>  #define JS_JITINFO_NATIVE_PARALLEL(op)                                         \
> +    {{(JSJitGetterOp)(op)},0,0,JSJitInfo::ParallelNative,JSVAL_TYPE_MISSING,false,false,false,false,0,JSJitInfo::AliasEverything}

I know this isn't "your fault", but is this because of the first type in the union, or something? It's clearly not a GetterOp. Can we add a comment here?

Comment 53

[Jeff Walden [:Waldo]]

(In reply to Nathan Froyd (:froydnj) from comment #51)
> This is green on Try, except that the B2G ICS builds don't seem to like
> something about the way JS_JITINFO_NATIVE_PARALLEL is defined.  Which is
> mystifying to me, as the B2G JB builds are just fine with it and I expect
> that
> they are using the same compiler...?  Going to try to talk to the B2G folks
> about that one.

As far as I can tell, the B2G ICS (only, I think?) compiler is just weird.  Tinderbox logs say it's a 4.4.x beast.  But it has to be some sort of weirdly-patched, even possibly very-patched compiler, because it has -Werror=conversion-null in it, which didn't land until gcc 4.5.  (I ran into this working on bug 953296.)  So I wouldn't rely on any documentation the ostensible compiler version number might suggest, or expect it to be like anything else.  :-\

Comment 54

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Comment on attachment 8359390 [details] [diff] [review]
part 5 - move JSJitInfo::argTypes to a separate JSTypedMethodJitInfo subclass

>+                    jitinfo = ("(const JSJitInfo*)&%s_methodinfo" % accessor)

Can that be a reinterpret_cast, just to make it clear?

>+    const JSTypedMethodJitInfo *methodInfo =
>+        (const JSTypedMethodJitInfo*)jitInfo;

And here.

>+        return isTypedMethod != 0;

Just return isTypedMethod.

>     // XXXbz should we have a JSGetterJitInfo subclass or something?

This comment can probably go away now that there is nothing really getter-specific here.

r=me

Comment 55

[Nathan Froyd [:froydnj]]

Attachment: part 5a - compensate for broken b2g compiler

The B2G GCC 4.4 compiler doesn't seem to want to accept:

 { (type)TemplateFunction<Function> }

as a suitable initialization for the union member of JSJitInfo.  I think something
like this is required to placate it, but this solution is...ugly.  I wanted to try
and localize most of the damage to the JS_JITINFO_NATIVE_PARALLEL macro so when
we have non-broken compilers everywhere, changing back should be pretty simple.

Thoughts?  (Not asking for review yet until I see whether this works on try; if it
doesn't, I'm not sure what to try next.)

Comment 56

[Nathan Froyd [:froydnj]]

(In reply to Nathan Froyd (:froydnj) from comment #55)
> Thoughts?  (Not asking for review yet until I see whether this works on try;
> if it
> doesn't, I'm not sure what to try next.)

This does work: https://tbpl.mozilla.org/?tree=Try&rev=9d7978378dff =/

Comment 57

[Nathan Froyd [:froydnj]]

Attachment: part 5a - compensate for broken b2g compiler

This is the same patch that worked on try, except for an explanatory comment,
which also addresses the need for the reinterpret_cast.  We also re-wrap the
serial op with JSParallelNativeThreadSafeWrapper so things are as close as
possible to the way they previously were.  (Yo, I heard you like wrappers, so
I wrapped up your wrapper...anyway...)

efaust said on IRC he couldn't think of anything better, either.

Comment 58

[Nathan Froyd [:froydnj]]

(In reply to Nathan Froyd (:froydnj) from comment #57)
> Created attachment 8360416 [details] [diff] [review]
> part 5a - compensate for broken b2g compiler

Also should note that I will be folding this into part 4, since that's where it really belongs; it would break bisection otherwise.

Comment 59

[Eric Faust [:efaust]]

Comment on attachment 8360416 [details] [diff] [review]
part 5a - compensate for broken b2g compiler

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

Ugly, but whatchu gon' do? r=me

::: js/src/jsfriendapi.h
@@ +1586,5 @@
> + * (We need the reinterpret_cast because we must initialize the union with
> + * a datum of the type of the union's first member.)
> + *
> + * Presumably this has something to do with template instantiation.
> + * Initializing with a normal function pointer seems to work fine. Hence

I vastly prefer this with the comment. Thanks for adding it :)

Comment 60

[Nathan Froyd [:froydnj]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b3d04b01c319
https://hg.mozilla.org/integration/mozilla-inbound/rev/51d099fe7403
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc45b7c4369b
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f5cb5e810e5
https://hg.mozilla.org/integration/mozilla-inbound/rev/a18cfe2cdc55
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c39a7047e96

I moved the comments as requested.  Any large explanatory comment for JSJitInfo will have to wait for a followup, likewise any fiddling with static_assert for the size of the structure.  Will work on the followup bug tomorrow.

Comment 61

[Nathan Froyd [:froydnj]]

Attachment: part 5b - update for ForkJoin.cpp changes

Landing this patch series last night broke the tree, because ForkJoin.cpp had added a new JS_JITINFO_PARALLEL_NATIVE that didn't need JSParallelNativeThreadSafeWrapper.  So, for better or for worse, we now need two versions of the macro.

Comment 62

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/524be0420e79

Comment 63

[Nathan Froyd [:froydnj]]

Whoops, forgot a [leave open] here after backing out last night. =/

Comment 64

[Eric Faust [:efaust]]

Comment on attachment 8361106 [details] [diff] [review]
part 5b - update for ForkJoin.cpp changes

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

Oh boy! Sure. OK. r=me

Comment 65

[Nathan Froyd [:froydnj]]

Let's try this again; had a green try push (https://tbpl.mozilla.org/?tree=Try&rev=c6489cc39963) and built locally x86-64 Linux debug with inbound tip before push.

https://hg.mozilla.org/integration/mozilla-inbound/rev/b6efb990e7ba
https://hg.mozilla.org/integration/mozilla-inbound/rev/2aad3ddd1a3e
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8ce486a47c7
https://hg.mozilla.org/integration/mozilla-inbound/rev/d17caf3f9294
https://hg.mozilla.org/integration/mozilla-inbound/rev/e7aa4248135d
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e77fda6a5e0

Comment 66

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b6efb990e7ba
https://hg.mozilla.org/mozilla-central/rev/2aad3ddd1a3e
https://hg.mozilla.org/mozilla-central/rev/e8ce486a47c7
https://hg.mozilla.org/mozilla-central/rev/d17caf3f9294
https://hg.mozilla.org/mozilla-central/rev/e7aa4248135d
https://hg.mozilla.org/mozilla-central/rev/1e77fda6a5e0