Bug 87322 - improve StyleRareNonInheritedData bit packing on Windows
Summary: improve StyleRareNonInheritedData bit packing on Windows
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tony Chang
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-05-23 16:45 PDT by Tony Chang
Modified: 2012-05-24 11:34 PDT (History)
3 users (show)

See Also:


Attachments
Patch (8.13 KB, patch)
2012-05-23 16:48 PDT, Tony Chang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tony Chang 2012-05-23 16:45:11 PDT
improve StyleRareNonInheritedData bit packing on Windows
Comment 1 Tony Chang 2012-05-23 16:48:40 PDT
Created attachment 143678 [details]
Patch
Comment 2 Tony Chang 2012-05-23 16:49:37 PDT
I didn't add a compile assert because it's OK for this struct to grow slowly.
Comment 3 Eric Seidel (no email) 2012-05-23 17:15:34 PDT
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.
Comment 4 Ryosuke Niwa 2012-05-23 17:17:53 PDT
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?
Comment 5 Tony Chang 2012-05-24 09:35:15 PDT
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.
Comment 6 Ryosuke Niwa 2012-05-24 10:14:23 PDT
(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.
Comment 7 WebKit Review Bot 2012-05-24 11:34:28 PDT
Comment on attachment 143678 [details]
Patch

Clearing flags on attachment: 143678

Committed r118396: <http://trac.webkit.org/changeset/118396>
Comment 8 WebKit Review Bot 2012-05-24 11:34:32 PDT
All reviewed patches have been landed.  Closing bug.