RESOLVED FIXED 87322
improve StyleRareNonInheritedData bit packing on Windows
https://bugs.webkit.org/show_bug.cgi?id=87322
Summary improve StyleRareNonInheritedData bit packing on Windows
Tony Chang
Reported 2012-05-23 16:45:11 PDT
improve StyleRareNonInheritedData bit packing on Windows
Attachments
Patch (8.13 KB, patch)
2012-05-23 16:48 PDT, Tony Chang
no flags
Tony Chang
Comment 1 2012-05-23 16:48:40 PDT
Tony Chang
Comment 2 2012-05-23 16:49:37 PDT
I didn't add a compile assert because it's OK for this struct to grow slowly.
Eric Seidel (no email)
Comment 3 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.
Ryosuke Niwa
Comment 4 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?
Tony Chang
Comment 5 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.
Ryosuke Niwa
Comment 6 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.
WebKit Review Bot
Comment 7 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>
WebKit Review Bot
Comment 8 2012-05-24 11:34:32 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.