Bug 87322 – improve StyleRareNonInheritedData bit packing on Windows

• Component: New Bugs
• Status: RESOLVED FIXED

[Tony Chang]

improve StyleRareNonInheritedData bit packing on Windows

[Tony Chang]

Created attachment 143678 [details]
Patch

[Tony Chang]

I didn't add a compile assert because it's OK for this struct to grow slowly.

[Eric Seidel (no email)]

Comment on attachment 143678 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=143678&action=review

LGTM.

> Source/WebCore/rendering/style/StyleRareNonInheritedData.cpp:150
>  bool StyleRareNonInheritedData::operator==(const StyleRareNonInheritedData& o) const

Presumably we could optimize the comparison order.  Do you believe that was attempted previously?  I'm OK with this kept in this order if you think it's unlikely to matter for perf.

[Ryosuke Niwa]

Comment on attachment 143678 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=143678&action=review

> Source/WebCore/rendering/style/StyleRareNonInheritedData.h:176
> -    bool m_hasAspectRatio : 1; // Whether or not an aspect ratio has been specified.
>  #if USE(ACCELERATED_COMPOSITING)
> -    bool m_runningAcceleratedAnimation : 1;
> +    unsigned m_runningAcceleratedAnimation : 1;
>  #endif
> +    unsigned m_hasAspectRatio : 1; // Whether or not an aspect ratio has been specified.

Could you please add a compile assertion?

[Tony Chang]

Comment on attachment 143678 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=143678&action=review

>> Source/WebCore/rendering/style/StyleRareNonInheritedData.cpp:150
>>  bool StyleRareNonInheritedData::operator==(const StyleRareNonInheritedData& o) const
> 
> Presumably we could optimize the comparison order.  Do you believe that was attempted previously?  I'm OK with this kept in this order if you think it's unlikely to matter for perf.

I don't think perf was a concern.  Looking at the history of the header file, members have been reordered multiple times for bit packing reasons.  I bet it was just an oversight that this wasn't reordered.

>> Source/WebCore/rendering/style/StyleRareNonInheritedData.h:176
>> +    unsigned m_hasAspectRatio : 1; // Whether or not an aspect ratio has been specified.
> 
> Could you please add a compile assertion?

Do you think it's worth it given that this object can grow?  It's already 448 bytes on my 64 bit machine.

[Ryosuke Niwa]

(In reply to comment #5)
> >> Source/WebCore/rendering/style/StyleRareNonInheritedData.h:176
> >> +    unsigned m_hasAspectRatio : 1; // Whether or not an aspect ratio has been specified.
> > 
> > Could you please add a compile assertion?
> 
> Do you think it's worth it given that this object can grow?  It's already 448 bytes on my 64 bit machine.

Oh, I see. Maybe not then.

[WebKit Review Bot]

Comment on attachment 143678 [details]
Patch

Clearing flags on attachment: 143678

Committed r118396: <http://trac.webkit.org/changeset/118396>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.