Bug 69521 - Shrink BorderValue
: Shrink BorderValue
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Andreas Kling
:
Depends on: 69662 69746 81180 81498
Blocks:
  Show dependency treegraph
 
Reported: 2011-10-06 05:32 PDT by Andreas Kling
Modified: 2012-03-18 23:52 PDT (History)
4 users (show)

See Also:


Attachments
Proposed patch (8.64 KB, patch)
2011-10-06 05:34 PDT, Andreas Kling
no flags Details | Formatted Diff | Diff
Proposed patch v2 (19.55 KB, patch)
2011-10-06 11:28 PDT, Andreas Kling
no flags Details | Formatted Diff | Diff
I made a patch. (7.98 KB, patch)
2012-03-16 13:28 PDT, Andreas Kling
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andreas Kling 2011-10-06 05:32:57 PDT
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.
Comment 1 Andreas Kling 2011-10-06 05:34:16 PDT
Created attachment 109947 [details]
Proposed patch
Comment 2 Andreas Kling 2011-10-06 05:49:35 PDT
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.
Comment 3 Early Warning System Bot 2011-10-06 05:51:05 PDT
Comment on attachment 109947 [details]
Proposed patch

Attachment 109947 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9978188
Comment 4 WebKit Review Bot 2011-10-06 05:53:14 PDT
Comment on attachment 109947 [details]
Proposed patch

Attachment 109947 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9981210
Comment 5 Andreas Kling 2011-10-06 11:28:57 PDT
Created attachment 109984 [details]
Proposed patch v2
Comment 6 Antti Koivisto 2011-10-06 11:50:50 PDT
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.
Comment 7 Darin Adler 2011-10-06 13:57:50 PDT
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);
Comment 8 Andreas Kling 2011-10-07 09:06:28 PDT
(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.
Comment 9 Andreas Kling 2011-10-07 09:23:41 PDT
Committed r96944: <http://trac.webkit.org/changeset/96944>
Comment 10 Ryosuke Niwa 2011-10-07 12:42:39 PDT
This patch appears to have caused many tests to crash:
http://build.webkit.org/builders/SnowLeopard%20Intel%20Release%20%28Tests%29/builds/33723
Comment 11 Ryosuke Niwa 2011-10-07 13:15:33 PDT
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.
Comment 12 Ryosuke Niwa 2011-10-07 13:20:23 PDT
I'm sorry but I'm rolling out the patch for now.
Comment 13 Ryosuke Niwa 2011-10-07 13:24:23 PDT
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
Comment 14 Ryosuke Niwa 2011-10-08 00:40:32 PDT
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
Comment 15 Andreas Kling 2011-10-10 00:51:56 PDT
Committed r97045: <http://trac.webkit.org/changeset/97045>
Comment 16 Andreas Kling 2011-10-10 02:06:05 PDT
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. :(
Comment 17 Andreas Kling 2011-12-21 16:50:02 PST
Comment on attachment 109984 [details]
Proposed patch v2

Need to debug this properly.
Comment 18 Ryosuke Niwa 2012-03-14 19:08:31 PDT
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 :(
Comment 19 Andreas Kling 2012-03-14 19:14:30 PDT
(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. :)
Comment 20 Andreas Kling 2012-03-16 13:28:55 PDT
Created attachment 132359 [details]
I made a patch.
Comment 21 Andreas Kling 2012-03-16 15:03:29 PDT
Comment on attachment 132359 [details]
I made a patch.

Clearing flags on attachment: 132359

Committed r111076: <http://trac.webkit.org/changeset/111076>
Comment 22 Andreas Kling 2012-03-16 15:03:34 PDT
All reviewed patches have been landed.  Closing bug.
Comment 23 Csaba Osztrogonác 2012-03-18 15:47:55 PDT
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.
Comment 24 Csaba Osztrogonác 2012-03-18 15:48:31 PDT
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
Comment 25 Andreas Kling 2012-03-18 18:53:15 PDT
Wait what? Which compiler version of this?
Comment 26 Csaba Osztrogonác 2012-03-18 23:09:35 PDT
(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.
Comment 27 Csaba Osztrogonác 2012-03-18 23:52:01 PDT
I filed a new bug report to fix the build - https://bugs.webkit.org/show_bug.cgi?id=81498