shrink JSJitInfo slots to the minimum amount of space required.

RESOLVED FIXED in mozilla29

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: froydnj, Unassigned)

Tracking

unspecified
mozilla29
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink])

Attachments

(1 attachment)

(Reporter)

Comment 1

5 years ago
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.
Attachment #8361668 - Flags: review?(efaustbmo)
Attachment #8361668 - Flags: review?(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).
Attachment #8361668 - Flags: review?(bzbarsky) → review+
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.
Attachment #8361668 - Flags: review?(efaustbmo) → review+
(Reporter)

Comment 4

5 years ago
(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.
https://hg.mozilla.org/mozilla-central/rev/2a3b16b9246a
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in before you can comment on or make changes to this bug.