960109 - shrink JSJitInfo slots to the minimum amount of space required.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nathan Froyd [:froydnj]]

See:

https://bugzilla.mozilla.org/show_bug.cgi?id=952777#c30

for the plan.

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: convert JSJitInfo fields to bitfields

OK, last patch along these lines.  The patch is big because of the renaming of
the fields and adding accessor methods to satisfy static typing requirements.
Ideally the new static_assert will cause people to think twice about blithely
adding fields.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8361668 [details] [diff] [review]
convert JSJitInfo fields to bitfields

r=me assuming that file-level-in-a-header and class-level static_assert works (our internal static assert macros were only ok in functions or at file level in a .cpp).

Comment 3

[Eric Faust [:efaust]]

Comment on attachment 8361668 [details] [diff] [review]
convert JSJitInfo fields to bitfields

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

This looks pretty good. We now have the disagreement between the use of static_assert() and JS_STATIC_ASSERT() in JSJitInfo::staticAsserts(). I don't know to what extent this matters, but perhaps it's worth a moment of thought. r=me

::: js/src/jsfriendapi.h
@@ +1597,5 @@
>          JS_STATIC_ASSERT(Any & Null);
>      }
>  };
>  
> +static_assert(sizeof(JSJitInfo) == (sizeof(void*) + 2 * sizeof(uint32_t)),

Nice to see this here.

Comment 4

[Nathan Froyd [:froydnj]]

(In reply to Eric Faust [:efaust] from comment #3)
> This looks pretty good. We now have the disagreement between the use of
> static_assert() and JS_STATIC_ASSERT() in JSJitInfo::staticAsserts(). I
> don't know to what extent this matters, but perhaps it's worth a moment of
> thought. r=me

I can fix that in a followup.  Waldo said on IRC that the reason mass conversion hasn't happened is that static_assert requires some sort of explanatory comment and writing explanatory comments takes some time and thought.  But surely fixing up these six JS_STATIC_ASSERT calls should not be hard.

Comment 5

[Nathan Froyd [:froydnj]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2a3b16b9246a

Comment 6

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/2a3b16b9246a