Bug 69521 – Shrink BorderValue

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

[Andreas Kling]

BorderValue is currently bloated up to 12 bytes, and 8 bytes of that is the Color class. We could "unroll" that member and pack the color validity flag with the rest of BorderValue's bits, reducing BorderValue to 8 bytes.

Patch idea coming that reduces memory consumption by 760 kB when loading the full HTML5 spec.

[Andreas Kling]

Created attachment 109947 [details]
Proposed patch

[Andreas Kling]

Comment on attachment 109947 [details]
Proposed patch

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

> Source/WebCore/rendering/style/BorderValue.h:73
> +    const Color& color() const { return Color(m_rgba, m_validColor); }

Shame on me, this should not return a const-reference.

[Early Warning System Bot]

Comment on attachment 109947 [details]
Proposed patch

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

[WebKit Review Bot]

Comment on attachment 109947 [details]
Proposed patch

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

[Andreas Kling]

Created attachment 109984 [details]
Proposed patch v2

[Antti Koivisto]

Comment on attachment 109984 [details]
Proposed patch v2

r=me, seems like a nice win.

I think the new temporaries will largely get optimized out but it would still be good to verify that we are not regressing performance here (with sampling or even by looking into the assembly output) before landing.

[Darin Adler]

Comment on attachment 109984 [details]
Proposed patch v2

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

> Source/WebCore/css/CSSStyleApplyProperty.cpp:222
> -    typedef const Color& (RenderStyle::*GetterFunction)() const;
> +    typedef const Color (RenderStyle::*GetterFunction)() const;

The const here should be removed. The return type can and should just be Color. Almost every case where this patch says const Color it should just say Color.

> Source/WebCore/platform/graphics/Color.h:81
> -    Color(RGBA32 col) : m_color(col), m_valid(true) { }
> +    Color(RGBA32 col, bool valid = true) : m_color(col), m_valid(valid) { }

If you are touching this line I suggest changing the name to “color”.

I also suggest asserting that m_valid is only false if the color is zero.

    ASSERT(!m_color || m_valid);

[Andreas Kling]

(In reply to comment #6)
> I think the new temporaries will largely get optimized out but it would still be good to verify that we are not regressing performance here (with sampling or even by looking into the assembly output) before landing.

Dug through the assembly output on Lion and the temporaries get nicely inlined in release mode.

(In reply to comment #7)
> (From update of attachment 109984 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=109984&action=review
> 
> > Source/WebCore/css/CSSStyleApplyProperty.cpp:222
> > -    typedef const Color& (RenderStyle::*GetterFunction)() const;
> > +    typedef const Color (RenderStyle::*GetterFunction)() const;
> 
> The const here should be removed. The return type can and should just be Color. Almost every case where this patch says const Color it should just say Color.

Noted, will tweak before landing.

> > Source/WebCore/platform/graphics/Color.h:81
> > -    Color(RGBA32 col) : m_color(col), m_valid(true) { }
> > +    Color(RGBA32 col, bool valid = true) : m_color(col), m_valid(valid) { }
> 
> If you are touching this line I suggest changing the name to “color”.
> 
> I also suggest asserting that m_valid is only false if the color is zero.
> 
>     ASSERT(!m_color || m_valid);

Sure, will do as well.

[Andreas Kling]

Committed r96944: <http://trac.webkit.org/changeset/96944>

[Ryosuke Niwa]

This patch appears to have caused many tests to crash:
http://build.webkit.org/builders/SnowLeopard%20Intel%20Release%20%28Tests%29/builds/33723

[Ryosuke Niwa]

Could someone investigate failures? If not, I'm rolling out the patch soon since we're losing significant test coverage due to nrwt exiting early after 20 tests crashed.

[Ryosuke Niwa]

I'm sorry but I'm rolling out the patch for now.

[Ryosuke Niwa]

Reopen the bug since the patch was rolled out in http://trac.webkit.org/changeset/96976.

Here is a link to crash logs:
http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r96944%20(33723)/results.html

[Ryosuke Niwa]

Looking at the following two builds, it appears that we just need to touch config.h to trigger a full rebuild:
http://build.webkit.org/builders/SnowLeopard%20Intel%20Release%20%28Tests%29/builds/33736
http://build.webkit.org/builders/SnowLeopard%20Intel%20Release%20%28Tests%29/builds/33737

[Andreas Kling]

Committed r97045: <http://trac.webkit.org/changeset/97045>

[Andreas Kling]

Reopening since it was rolled out again. Touching config.h did not help.

This is the backtrace on SL testers:
0   com.apple.WebCore             	0x000000010140bd3c WTF::RefCountedBase::ref() + 16 (RefCounted.h:47)
1   com.apple.WebCore             	0x000000010140be5d void WTF::refIfNotNull<WebCore::StyleImage>(WebCore::StyleImage*) + 41 (PassRefPtr.h:53)
2   com.apple.WebCore             	0x0000000101eb39be WTF::RefPtr<WebCore::StyleImage>::RefPtr(WTF::RefPtr<WebCore::StyleImage> const&) + 42 (RefPtr.h:44)
3   com.apple.WebCore             	0x000000010143ee4f WebCore::NinePieceImage::NinePieceImage(WebCore::NinePieceImage const&) + 29 (NinePieceImage.h:37)
4   com.apple.WebCore             	0x00000001020447b3 WebCore::BorderData::BorderData(WebCore::BorderData const&) + 159 (BorderData.h:35)
5   com.apple.WebCore             	0x0000000102044596 WebCore::StyleSurroundData::StyleSurroundData(WebCore::StyleSurroundData const&) + 178 (StyleSurroundData.cpp:40)
6   com.apple.WebCore             	0x0000000101221bc2 WebCore::StyleSurroundData::copy() const + 48 (StyleSurroundData.h:38)
7   com.apple.WebCore             	0x0000000101412fd5 WebCore::DataRef<WebCore::StyleSurroundData>::access() + 57 (DataRef.h:41)
8   com.apple.WebCore             	0x0000000101442176 WebCore::RenderStyle::setMarginTop(WebCore::Length) + 62 (RenderStyle.h:1059)
9   com.apple.WebCore             	0x0000000101404432 WebCore::ApplyPropertyDefaultBase<WebCore::Length, WebCore::Length, WebCore::Length>::setValue(WebCore::RenderStyle*, WebCore::Length) const + 108 (CSSStyleApplyProperty.cpp:110)
10  com.apple.WebCore             	0x0000000101410026 WebCore::ApplyPropertyLength<(WebCore::LengthAuto)1, (WebCore::LengthIntrinsic)0, (WebCore::LengthMinIntrinsic)0, (WebCore::LengthNone)0, (WebCore::LengthUndefined)0, (WebCore::LengthFlexDirection)0>::applyValue(WebCore::CSSStyleSelector*, WebCore::CSSValue*) const + 334 (CSSStyleApplyProperty.cpp:345)
11  com.apple.WebCore             	0x000000010141d375 WebCore::CSSStyleSelector::applyProperty(int, WebCore::CSSValue*) + 447 (CSSStyleSelector.cpp:2434)
12  com.apple.WebCore             	0x00000001014486b7 void WebCore::CSSStyleSelector::applyDeclarations<false>(bool, int, int) + 189 (CSSStyleSelector.cpp:2210)
13  com.apple.WebCore             	0x000000010142da92 WebCore::CSSStyleSelector::styleForElement(WebCore::Element*, WebCore::RenderStyle*, bool, bool, bool) + 3750 (CSSStyleSelector.cpp:1284)
14  com.apple.WebCore             	0x00000001015ecef6 WebCore::Element::styleForRenderer() + 118 (Element.cpp:1057)
15  com.apple.WebCore             	0x0000000101d18890 WebCore::NodeRendererFactory::createRendererIfNeeded() + 294 (NodeRenderingContext.cpp:325)
16  com.apple.WebCore             	0x0000000101d06b36 WebCore::Node::createRendererIfNeeded() + 34 (Node.cpp:1475)
17  com.apple.WebCore             	0x00000001015edc5b WebCore::Element::attach() + 35 (Element.cpp:967)
18  com.apple.WebCore             	0x000000010172b309 WTF::PassRefPtr<WebCore::Element> WebCore::HTMLConstructionSite::attach<WebCore::Element>(WebCore::ContainerNode*, WTF::PassRefPtr<WebCore::Element>) + 499 (HTMLConstructionSite.cpp:112)
19  com.apple.WebCore             	0x00000001017292c0 WebCore::HTMLConstructionSite::attachToCurrent(WTF::PassRefPtr<WebCore::Element>) + 66 (HTMLConstructionSite.cpp:263)
20  com.apple.WebCore             	0x00000001017298f5 WebCore::HTMLConstructionSite::insertHTMLBodyElement(WebCore::AtomicHTMLToken&) + 117 (HTMLConstructionSite.cpp:276)
21  com.apple.WebCore             	0x00000001017c18c4 WebCore::HTMLTreeBuilder::processStartTag(WebCore::AtomicHTMLToken&) + 1056 (HTMLTreeBuilder.cpp:1198)
22  com.apple.WebCore             	0x00000001017c3b57 WebCore::HTMLTreeBuilder::defaultForAfterHead() + 103 (HTMLTreeBuilder.cpp:2715)
23  com.apple.WebCore             	0x00000001017c57dd WebCore::HTMLTreeBuilder::processEndOfFile(WebCore::AtomicHTMLToken&) + 653 (HTMLTreeBuilder.cpp:2612)
24  com.apple.WebCore             	0x00000001017c3d0e WebCore::HTMLTreeBuilder::processToken(WebCore::AtomicHTMLToken&) + 248 (HTMLTreeBuilder.cpp:496)
25  com.apple.WebCore             	0x00000001017c8a10 WebCore::HTMLTreeBuilder::constructTreeFromAtomicToken(WebCore::AtomicHTMLToken&) + 30 (HTMLTreeBuilder.cpp:467)
26  com.apple.WebCore             	0x00000001017c8b05 WebCore::HTMLTreeBuilder::constructTreeFromToken(WebCore::HTMLToken&) + 81 (HTMLTreeBuilder.cpp:454)
27  com.apple.WebCore             	0x00000001017410de WebCore::HTMLDocumentParser::pumpTokenizer(WebCore::HTMLDocumentParser::SynchronousMode) + 610 (HTMLDocumentParser.cpp:279)
28  com.apple.WebCore             	0x0000000101741427 WebCore::HTMLDocumentParser::pumpTokenizerIfPossible(WebCore::HTMLDocumentParser::SynchronousMode) + 161 (HTMLDocumentParser.cpp:178)
29  com.apple.WebCore             	0x0000000101741b8f WebCore::HTMLDocumentParser::prepareToStopParsing() + 117 (HTMLDocumentParser.cpp:144)
30  com.apple.WebCore             	0x000000010173fc9c WebCore::HTMLDocumentParser::attemptToEnd() + 58 (HTMLDocumentParser.cpp:403)
31  com.apple.WebCore             	0x000000010173ff34 WebCore::HTMLDocumentParser::finish() + 54 (HTMLDocumentParser.cpp:430)
32  com.apple.WebCore             	0x00000001014e7c9e WebCore::DocumentWriter::endIfNotLoadingMainResource() + 206 (DocumentWriter.cpp:236)
33  com.apple.WebCore             	0x00000001014e7ce5 WebCore::DocumentWriter::end() + 41 (DocumentWriter.cpp:215)
34  com.apple.WebCore             	0x0000000101680d3b WebCore::FrameLoader::init() + 735 (FrameLoader.cpp:240)
35  com.apple.WebKit              	0x0000000100c61e78 WebCore::Frame::init() + 28 (Frame.h:276)
36  com.apple.WebKit              	0x0000000100c5e94b +[WebFrame(WebInternal) _createFrameWithPage:frameName:frameView:ownerElement:] + 775 (WebFrame.mm:279)
37  com.apple.WebKit              	0x0000000100c5de1c +[WebFrame(WebInternal) _createMainFrameWithPage:frameName:frameView:] + 80 (WebFrame.mm:286)
38  com.apple.WebKit              	0x0000000100d1775a -[WebView(WebPrivate) _commonInitializationWithFrameName:groupName:] + 2891 (WebView.mm:744)
39  com.apple.WebKit              	0x0000000100d145fe -[WebView(WebPrivate) _initWithFrame:frameName:groupName:usesDocumentViews:] + 361 (WebView.mm:823)
40  com.apple.WebKit              	0x0000000100d0f5b8 -[WebView initWithFrame:frameName:groupName:] + 262 (WebView.mm:3059)
41  DumpRenderTree                	0x000000010001151a createWebViewAndOffscreenWindow() + 405 (DumpRenderTree.mm:447)
42  DumpRenderTree                	0x0000000100013d7e dumpRenderTree(int, char const**) + 102 (DumpRenderTree.mm:772)
43  DumpRenderTree                	0x000000010001406a main + 97 (DumpRenderTree.mm:833)
44  DumpRenderTree                	0x000000010000213c start + 52

I can't reproduce locally on Lion. :(

[Andreas Kling]

Comment on attachment 109984 [details]
Proposed patch v2

Need to debug this properly.

[Ryosuke Niwa]

Comment on attachment 109984 [details]
Proposed patch v2

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

> Source/WebCore/rendering/style/BorderValue.h:84
> +    bool m_validColor : 1;

Ugh... please use unsigned here. Or else we'll end up allocating 4 extra byte on Windows :(

[Andreas Kling]

(In reply to comment #18)
> (From update of attachment 109984 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=109984&action=review
> 
> > Source/WebCore/rendering/style/BorderValue.h:84
> > +    bool m_validColor : 1;
> 
> Ugh... please use unsigned here. Or else we'll end up allocating 4 extra byte on Windows :(

Yeah I will. This patch is pretty moldy, it predates my knowledge of the MSVC bitfield stupidity. :)

[Andreas Kling]

Created attachment 132359 [details]
I made a patch.

[Andreas Kling]

Comment on attachment 132359 [details]
I made a patch.

Clearing flags on attachment: 132359

Committed r111076: <http://trac.webkit.org/changeset/111076>

[Andreas Kling]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

Reopen, because it broke the Qt 32 bit build. I don't know how, I don't know why only the 32 bit build. Unfortunately it was hidden build break, because http://trac.webkit.org/changeset/111075 broke the build previously and only http://trac.webkit.org/changeset/111106 fixed it.

[Csaba Osztrogonác]

cc1plus: warnings being treated as errors
In file included from ../../../../Source/JavaScriptCore/wtf/HashMap.h:25,
                 from ../../../../Source/WebCore/platform/network/HTTPHeaderMap.h:32,
                 from ../../../../Source/WebCore/platform/network/ResourceResponseBase.h:31,
                 from ../../../../Source/WebCore/platform/network/qt/ResourceResponse.h:30,
                 from ../../../../Source/WebCore/platform/network/AuthenticationChallengeBase.h:31,
                 from ../../../../Source/WebCore/platform/network/qt/AuthenticationChallenge.h:29,
                 from ../../../../Source/WebCore/platform/network/ResourceHandle.h:30,
                 from ../../../../Source/WebCore/loader/ResourceLoaderOptions.h:35,
                 from ../../../../Source/WebCore/loader/cache/CachedResource.h:31,
                 from ../../../../Source/WebCore/loader/cache/CachedImage.h:27,
                 from ../../../../Source/WebCore/rendering/RenderObject.h:30,
                 from ../../../../Source/WebCore/rendering/RenderBoxModelObject.h:28,
                 from ../../../../Source/WebCore/rendering/RenderBox.h:27,
                 from ../../../../Source/WebCore/rendering/RenderBlock.h:30,
                 from ../../../../Source/WebCore/rendering/RenderTable.h:31,
                 from ../../../../Source/WebCore/rendering/RenderTableSection.h:29,
                 from ../../../../Source/WebCore/rendering/RenderTableSection.cpp:28:
../../../../Source/JavaScriptCore/wtf/HashTraits.h: In member function 'void WTF::HashTable<Key, Value, Extractor, HashFunctions, Traits, KeyTraits>::rehash(int) [with Key = std::pair<const WebCore::RenderTableCell*, int>, Value = std::pair<std::pair<const WebCore::RenderTableCell*, int>, WebCore::CollapsedBorderValue>, Extractor = WTF::PairFirstExtractor<std::pair<std::pair<const WebCore::RenderTableCell*, int>, WebCore::CollapsedBorderValue> >, HashFunctions = WTF::PairHash<const WebCore::RenderTableCell*, int>, Traits = WTF::PairHashTraits<WTF::HashTraits<std::pair<const WebCore::RenderTableCell*, int> >, WTF::HashTraits<WebCore::CollapsedBorderValue> >, KeyTraits = WTF::HashTraits<std::pair<const WebCore::RenderTableCell*, int> >]':
../../../../Source/JavaScriptCore/wtf/HashTraits.h:153: error: '__y' may be used uninitialized in this function
../../../../Source/JavaScriptCore/wtf/HashTraits.h:153: note: '__y' was declared here
In file included from ../../../../Source/WebCore/platform/network/HTTPHeaderMap.h:32,
                 from ../../../../Source/WebCore/platform/network/ResourceResponseBase.h:31,
                 from ../../../../Source/WebCore/platform/network/qt/ResourceResponse.h:30,
                 from ../../../../Source/WebCore/platform/network/AuthenticationChallengeBase.h:31,
                 from ../../../../Source/WebCore/platform/network/qt/AuthenticationChallenge.h:29,
                 from ../../../../Source/WebCore/platform/network/ResourceHandle.h:30,
                 from ../../../../Source/WebCore/loader/ResourceLoaderOptions.h:35,
                 from ../../../../Source/WebCore/loader/cache/CachedResource.h:31,
                 from ../../../../Source/WebCore/loader/cache/CachedImage.h:27,
                 from ../../../../Source/WebCore/rendering/RenderObject.h:30,
                 from ../../../../Source/WebCore/rendering/RenderBoxModelObject.h:28,
                 from ../../../../Source/WebCore/rendering/RenderBox.h:27,
                 from ../../../../Source/WebCore/rendering/RenderBlock.h:30,
                 from ../../../../Source/WebCore/rendering/RenderTable.h:31,
                 from ../../../../Source/WebCore/rendering/RenderTableSection.h:29,
                 from ../../../../Source/WebCore/rendering/RenderTableSection.cpp:28:
../../../../Source/WebCore/rendering/RenderTableSection.cpp: In member function 'std::pair<typename WTF::HashMap<KeyArg, MappedArg, HashArg, KeyTraitsArg, MappedTraitsArg>::iterator, bool> WTF::HashMap<KeyArg, MappedArg, HashArg, KeyTraitsArg, MappedTraitsArg>::set(const typename KeyTraitsArg::TraitType&, typename MappedTraitsArg::PassInType) [with KeyArg = std::pair<const WebCore::RenderTableCell*, int>, MappedArg = WebCore::CollapsedBorderValue, HashArg = WTF::PairHash<const WebCore::RenderTableCell*, int>, KeyTraitsArg = WTF::HashTraits<std::pair<const WebCore::RenderTableCell*, int> >, MappedTraitsArg = WTF::HashTraits<WebCore::CollapsedBorderValue>]':
../../../../Source/WebCore/rendering/RenderTableSection.cpp:1424: error: '__y' may be used uninitialized in this function
../../../../Source/JavaScriptCore/wtf/HashTraits.h:153: note: '__y' was declared here

[Andreas Kling]

Wait what? Which compiler version of this?

[Csaba Osztrogonác]

(In reply to comment #25)
> Wait what? Which compiler version of this?
gcc version 4.4.5 (Debian 4.4.5-8)

But 64 bit bots have the same gcc version and everything works with it.

[Csaba Osztrogonác]

I filed a new bug report to fix the build - https://bugs.webkit.org/show_bug.cgi?id=81498