Bug 69521 - Shrink BorderValue
: Shrink BorderValue
Status: RESOLVED FIXED
: WebKit
Layout and Rendering
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
: 69662 69746 81180 81498
:
  Show dependency treegraph
 
Reported: 2011-10-06 05:32 PST by
Modified: 2012-03-18 23:52 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-10-06 05:32:57 PST
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 From 2011-10-06 05:34:16 PST -------
Created an attachment (id=109947) [details]
Proposed patch
------- Comment #2 From 2011-10-06 05:49:35 PST -------
(From update of attachment 109947 [details])
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 From 2011-10-06 05:51:05 PST -------
(From update of attachment 109947 [details])
Attachment 109947 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9978188
------- Comment #4 From 2011-10-06 05:53:14 PST -------
(From update of attachment 109947 [details])
Attachment 109947 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9981210
------- Comment #5 From 2011-10-06 11:28:57 PST -------
Created an attachment (id=109984) [details]
Proposed patch v2
------- Comment #6 From 2011-10-06 11:50:50 PST -------
(From update of attachment 109984 [details])
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 From 2011-10-06 13:57:50 PST -------
(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.

> 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 From 2011-10-07 09:06:28 PST -------
(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] [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 From 2011-10-07 09:23:41 PST -------
Committed r96944: <http://trac.webkit.org/changeset/96944>
------- Comment #10 From 2011-10-07 12:42:39 PST -------
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 From 2011-10-07 13:15:33 PST -------
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 From 2011-10-07 13:20:23 PST -------
I'm sorry but I'm rolling out the patch for now.
------- Comment #13 From 2011-10-07 13:24:23 PST -------
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 From 2011-10-08 00:40:32 PST -------
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 From 2011-10-10 00:51:56 PST -------
Committed r97045: <http://trac.webkit.org/changeset/97045>
------- Comment #16 From 2011-10-10 02:06:05 PST -------
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 From 2011-12-21 16:50:02 PST -------
(From update of attachment 109984 [details])
Need to debug this properly.
------- Comment #18 From 2012-03-14 19:08:31 PST -------
(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 :(
------- Comment #19 From 2012-03-14 19:14:30 PST -------
(In reply to comment #18)
> (From update of attachment 109984 [details] [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 From 2012-03-16 13:28:55 PST -------
Created an attachment (id=132359) [details]
I made a patch.
------- Comment #21 From 2012-03-16 15:03:29 PST -------
(From update of attachment 132359 [details])
Clearing flags on attachment: 132359

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