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.
Created attachment 109947 [details] Proposed patch
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 on attachment 109947 [details] Proposed patch Attachment 109947 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/9978188
Comment on attachment 109947 [details] Proposed patch Attachment 109947 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9981210
Created attachment 109984 [details] Proposed patch v2
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 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);
(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.
Committed r96944: <http://trac.webkit.org/changeset/96944>
This patch appears to have caused many tests to crash: http://build.webkit.org/builders/SnowLeopard%20Intel%20Release%20%28Tests%29/builds/33723
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.
I'm sorry but I'm rolling out the patch for now.
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
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
Committed r97045: <http://trac.webkit.org/changeset/97045>
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 on attachment 109984 [details] Proposed patch v2 Need to debug this properly.
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 :(
(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. :)
Created attachment 132359 [details] I made a patch.
Comment on attachment 132359 [details] I made a patch. Clearing flags on attachment: 132359 Committed r111076: <http://trac.webkit.org/changeset/111076>
All reviewed patches have been landed. Closing bug.
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.
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
Wait what? Which compiler version of this?
(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.
I filed a new bug report to fix the build - https://bugs.webkit.org/show_bug.cgi?id=81498