Bug 69331 – Shrink StyleRareNonInheritedData.

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Andreas Kling]

We can shrink StyleRareNonInheritedData by two CPU words if we rearrange its members.

This would cut memory usage by 140 kB (on 64-bit) when loading the full HTML5 spec.

[Andreas Kling]

Created attachment 109606 [details]
Proposed patch

[Early Warning System Bot]

Comment on attachment 109606 [details]
Proposed patch

Attachment 109606 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9944281

[Antti Koivisto]

Comment on attachment 109606 [details]
Proposed patch

r=me if you fix whatever Qt bot is whining about.

[WebKit Review Bot]

Comment on attachment 109606 [details]
Proposed patch

Attachment 109606 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9936528

New failing tests:
plugins/fullscreen-plugins-dont-reload.html

[Andreas Kling]

Comment on attachment 109606 [details]
Proposed patch

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

> Source/WebCore/rendering/style/StyleRareNonInheritedData.cpp:69
> +    printf("sizeof(StyleRareNonInheritedData) = %lu\n", sizeof(StyleRareNonInheritedData));

Looks like I pulled an Antti here. :)

[Andreas Kling]

Created attachment 109619 [details]
Patch for landing (r=anttik)

[WebKit Review Bot]

Comment on attachment 109619 [details]
Patch for landing (r=anttik)

Clearing flags on attachment: 109619

Committed r96594: <http://trac.webkit.org/changeset/96594>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 109606 [details]
Proposed patch

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

> Source/WebCore/rendering/style/StyleRareNonInheritedData.h:132
> +    RegionOverflow m_regionOverflow : 1;

Won’t we run into signed/unsigned problems with this with the Windows compiler?

> Source/WebCore/rendering/style/StyleRareNonInheritedData.h:140
> +    PageSizeType m_pageSizeType : 2;
> +    ETransformStyle3D m_transformStyle3D : 1;
> +    EBackfaceVisibility m_backfaceVisibility : 1;

Won’t we run into signed/unsigned problems with these with the Windows compiler?

[Andreas Kling]

(In reply to comment #9)
> (From update of attachment 109606 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=109606&action=review
> 
> > Source/WebCore/rendering/style/StyleRareNonInheritedData.h:132
> > +    RegionOverflow m_regionOverflow : 1;
> 
> Won’t we run into signed/unsigned problems with this with the Windows compiler?
> 
> > Source/WebCore/rendering/style/StyleRareNonInheritedData.h:140
> > +    PageSizeType m_pageSizeType : 2;
> > +    ETransformStyle3D m_transformStyle3D : 1;
> > +    EBackfaceVisibility m_backfaceVisibility : 1;
> 
> Won’t we run into signed/unsigned problems with these with the Windows compiler?

Will we? I was not aware of this issue. CC'ing Adam who might know more.

[Antti Koivisto]

(In reply to comment #9)
> Won’t we run into signed/unsigned problems with these with the Windows compiler?

Oh is that the reason why we have so many enum fields specified as "unsigned"? I didn't know.

[Darin Adler]

(In reply to comment #11)
> (In reply to comment #9)
> > Won’t we run into signed/unsigned problems with these with the Windows compiler?
> 
> Oh is that the reason why we have so many enum fields specified as "unsigned"?

Yes.

[Adam Roben (:aroben)]

http://trac.webkit.org/changeset/25329 has a little more info.

[Andreas Kling]

Created attachment 109648 [details]
Follow-up patch to make enum bitfields explicitly unsigned.

Here we go. Thanks a bunch for pointing this out!

[Vangelis Kokkevis]

(In reply to comment #10)
> (In reply to comment #9)
> > (From update of attachment 109606 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=109606&action=review
> > 
> > > Source/WebCore/rendering/style/StyleRareNonInheritedData.h:132
> > > +    RegionOverflow m_regionOverflow : 1;
> > 
> > Won’t we run into signed/unsigned problems with this with the Windows compiler?
> > 
> > > Source/WebCore/rendering/style/StyleRareNonInheritedData.h:140
> > > +    PageSizeType m_pageSizeType : 2;
> > > +    ETransformStyle3D m_transformStyle3D : 1;
> > > +    EBackfaceVisibility m_backfaceVisibility : 1;
> > 
> > Won’t we run into signed/unsigned problems with these with the Windows compiler?
> 
> Will we? I was not aware of this issue. CC'ing Adam who might know more.

Definitely an issue with the windows compiler.  It did break Chrome on windows where the preserves3d property stopped working due to a (-1 != 1 comparison).  Thanks for checking in a fix.

[Andreas Kling]

Committed r96691: <http://trac.webkit.org/changeset/96691>