Don't use DebugOnly for members of structs when size matters

RESOLVED FIXED in mozilla29

Status

()

--
enhancement
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: sunfish, Assigned: sunfish)

Tracking

unspecified
mozilla29
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
Created attachment 8336161 [details] [diff] [review]
no-debugonly-in-structs.patch

ICStubCompiler, InlineForwardList (and thus LiveInterval), and SafepointIndex are all classes which are allocated in decent numbers, and all contain DebugOnly data members. While DebugOnly contains no data in non-debug mode, C++ rules require that it still be allocated its own byte of memory, and alignment padding in these structs pushes this up to 4 or 8 bytes, depending on the platform.

Attached is a patch which introduces a new MOZ_DEBUG_ONLY macro, which is similar to the DebugOnly utility, but doesn't use any memory when used in structs. However, it is admittedly less pretty.
Attachment #8336161 - Flags: review?(jwalden+bmo)

Comment 1

5 years ago
Comment on attachment 8336161 [details] [diff] [review]
no-debugonly-in-structs.patch

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

jorendorff and I both think that #ifdef DEBUG / FieldType field_; / #endif, and similar for the few assignments in this patch, is preferable to a macro that hides this stuff.

::: mfbt/DebugOnly.h
@@ +28,5 @@
>   * DebugOnly instances can only be coerced to T in debug builds.  In release
>   * builds they don't have a value, so type coercion is not well defined.
> + *
> + * Note that DebugOnly instances still take up one byte of space, plus padding,
> + * when used as members of structs.

This seems like a good comment to add even if MOZ_DEBUG_ONLY isn't wanted.
Attachment #8336161 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 2

5 years ago
Created attachment 8344869 [details] [diff] [review]
no-debugonly-in-structs.patch

Ok. Here's a patch which does the same thing, using #ifdef DEBUG instead.
Assignee: nobody → sunfish
Attachment #8336161 - Attachment is obsolete: true
Attachment #8344869 - Flags: review?(jwalden+bmo)

Comment 3

5 years ago
Comment on attachment 8344869 [details] [diff] [review]
no-debugonly-in-structs.patch

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

Beware bug 948099 if it lands before this.

::: js/src/jit/BaselineIC.h
@@ +999,5 @@
>  
>    protected:
> +#ifdef DEBUG
> +    bool entersStubFrame_;
> +#endif

I'd prefer moving this to the end of the struct, after |kind|, myself.

@@ +1019,5 @@
> +      : suppressGC(cx)
> +#ifdef DEBUG
> +      , entersStubFrame_(false)
> +#endif
> +      , cx(cx), kind(kind)

The move means only the |, entersStubFrame_(false)| needs a comma at the start of a line.
Attachment #8344869 - Flags: review?(jwalden+bmo) → review+
(Assignee)

Comment 4

5 years ago
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #3)
> Comment on attachment 8344869 [details] [diff] [review]
> no-debugonly-in-structs.patch
> 
> Review of attachment 8344869 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Beware bug 948099 if it lands before this.
> 
> ::: js/src/jit/BaselineIC.h
> @@ +999,5 @@
> >  
> >    protected:
> > +#ifdef DEBUG
> > +    bool entersStubFrame_;
> > +#endif
> 
> I'd prefer moving this to the end of the struct, after |kind|, myself.

Works for me; done.

> @@ +1019,5 @@
> > +      : suppressGC(cx)
> > +#ifdef DEBUG
> > +      , entersStubFrame_(false)
> > +#endif
> > +      , cx(cx), kind(kind)
> 
> The move means only the |, entersStubFrame_(false)| needs a comma at the
> start of a line.

Done.

https://hg.mozilla.org/integration/mozilla-inbound/rev/33141bbd7e29
https://hg.mozilla.org/mozilla-central/rev/33141bbd7e29
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.