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

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement)

Description

[Dan Gohman [:sunfish]]

Attachment: 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.

Comment 1

[Jeff Walden [:Waldo]]

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.

Comment 2

[Dan Gohman [:sunfish]]

Attachment: no-debugonly-in-structs.patch

Ok. Here's a patch which does the same thing, using #ifdef DEBUG instead.

Comment 3

[Jeff Walden [:Waldo]]

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.

Comment 4

[Dan Gohman [:sunfish]]

(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

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/33141bbd7e29