WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
Tony Chang
Comment 1
2012-05-23 16:48:40 PDT
Created
attachment 143678
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug