Reduce CSSProperty's StylePropertyMetadata memory footprint by half when used inside a ImmutableStylePropertySet.
Created attachment 204853 [details] Patch
(In reply to comment #1) > Created an attachment (id=204853) [details] > Patch #117481 is needed for it to entirely work but I'd like to get a measurement on what are the improvements on internal Apple membuster if possible (as-is it should work for testing purposes). Anybody to help me please?
Comment on attachment 204853 [details] Patch Attachment 204853 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/857435 New failing tests: fast/css/transform-inline-style.html transitions/color-transition-rounding.html fast/dom/CSSStyleDeclaration/transition-property-names.html fast/css/transform-inline-style-remove.html media/video-zoom.html transitions/transitions-parsing.html fast/repaint/table-cell-collapsed-border-scroll.html fast/css/shorthand-mismatched-list-crash.html
Created attachment 204861 [details] Archive of layout-test-results from webkit-ews-04 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-04 Port: mac-mountainlion Platform: Mac OS X 10.8.3
(In reply to comment #3) > (From update of attachment 204853 [details]) > Attachment 204853 [details] did not pass mac-ews (mac): > Output: http://webkit-queues.appspot.com/results/857435 > > New failing tests: > fast/css/transform-inline-style.html > transitions/color-transition-rounding.html > fast/dom/CSSStyleDeclaration/transition-property-names.html > fast/css/transform-inline-style-remove.html > media/video-zoom.html > transitions/transitions-parsing.html > fast/repaint/table-cell-collapsed-border-scroll.html > fast/css/shorthand-mismatched-list-crash.html Expected failures due to 117481
Landed in Blink: https://chromium.googlesource.com/chromium/blink/+/4b7ca0e5f34d3004770dc12e27894df698738454
Comment on attachment 204853 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=204853&action=review > Source/WebCore/css/StylePropertyShorthand.cpp:626 > +typedef HashMap<CSSPropertyID, Vector<StylePropertyShorthand> > longhandsMap; Couldn't we just store pointers to StylePropertyShorthand here instead of the full objects? They are create-once-live-forever anyway, right? > Source/WebCore/css/StylePropertyShorthand.cpp:627 > +const Vector<StylePropertyShorthand> matchingShorthandsForLonghand(CSSPropertyID propertyID) This should return a const Vector& instead, so call sites don't have to make a copy.
Comment on attachment 204853 [details] Patch Attachment 204853 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/880413 New failing tests: fast/css/transform-inline-style.html transitions/color-transition-rounding.html fast/dom/CSSStyleDeclaration/transition-property-names.html fast/css/transform-inline-style-remove.html transitions/transitions-parsing.html fast/css/shorthand-mismatched-list-crash.html
Created attachment 205054 [details] Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-14 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.3
Created attachment 205110 [details] Patch
Created attachment 205220 [details] Patch
Comment on attachment 205220 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=205220&action=review r=me. I think this patch is an interesting step in the right direction. I'm sure we can refine it more over time. > Source/WebCore/ChangeLog:6 > + Today CSSProperty holds its metadata in the following way : I feel like this ChangeLog would be a lot more digestible if you simply made an "ASCII art" diagram of the new memory layout. > Source/WebCore/css/StylePropertyShorthand.cpp:627 > +const Vector<const StylePropertyShorthand*> matchingShorthandsForLonghand(CSSPropertyID propertyID) Can we make this return a reference instead of a Vector by value? > Source/WebCore/css/StylePropertyShorthand.h:60 > + CSSPropertyID id() const { return m_shorthandID; } I'd call this shorthandID() rather than id(). > Source/WebCore/css/makeprop.pl:35 > +die "We've reached more than 1024 CSS properties, please make sure to update CSSProperty/StylePropertyMetadata accordingly" if (scalar(@NAMES) > 1024); Great idea putting this here.
(In reply to comment #12) > (From update of attachment 205220 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=205220&action=review > > r=me. > > I think this patch is an interesting step in the right direction. I'm sure we can refine it more over time. > > > Source/WebCore/ChangeLog:6 > > + Today CSSProperty holds its metadata in the following way : > > I feel like this ChangeLog would be a lot more digestible if you simply made an "ASCII art" diagram of the new memory layout. Ahahaha I can try to do that. > > > Source/WebCore/css/StylePropertyShorthand.cpp:627 > > +const Vector<const StylePropertyShorthand*> matchingShorthandsForLonghand(CSSPropertyID propertyID) > > Can we make this return a reference instead of a Vector by value? Well if I do that then : return map.get(propertyID); return a ref to a temporary variable. typedef HashMap<CSSPropertyID, Vector<StylePropertyShorthand*> > longhandsMap; could be changed maybe but not sure if it would be cleaner. Thoughts? > > > Source/WebCore/css/StylePropertyShorthand.h:60 > > + CSSPropertyID id() const { return m_shorthandID; } > > I'd call this shorthandID() rather than id(). Will change. > > > Source/WebCore/css/makeprop.pl:35 > > +die "We've reached more than 1024 CSS properties, please make sure to update CSSProperty/StylePropertyMetadata accordingly" if (scalar(@NAMES) > 1024); > > Great idea putting this here. :D I saw it coming hitting the future me working there.
It seems like the blink patch got rolled out due to a regression?
*** Bug 118013 has been marked as a duplicate of this bug. ***
(In reply to comment #14) > It seems like the blink patch got rolled out due to a regression? The part that was rolled out was only the packing to uint16_t in StylePropertyMetada. The rest is there and is going to stay. In Blink I landed it in two parts. It's a regression on Windows only where MSVC doesn't generate good code to access to the new 16 bits StylePropertyMetada. Clang and GCC are unaffected. I was busy the past weeks with internal stuff but I'm planning to see what I can do about Windows.
Created attachment 207863 [details] Patch
Comment on attachment 207863 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=207863&action=review This patch needs a "Reviewed by" line. > Source/WebCore/ChangeLog:10 > + ------------------------------------------------------------------------------------------ > + | m_propertyID : 14 | m_shorthandID : 14 | m_important : 1| m_implicit : 1| m_inherited:1| > + ------------------------------------------------------------------------------------------ Seems like these boxes would look better if vertical. :) > Source/WebCore/ChangeLog:39 > + ------------------------------------------------------------------------------------------------------ > + | m_propertyID : 10 (up to 1024 properties)| m_isSetFromShorthand : 1 | m_indexInShorthandsVector : 2|... > + ------------------------------------------------------------------------------------------------------ ^
Created attachment 207911 [details] Patch for landing
Comment on attachment 207911 [details] Patch for landing Clearing flags on attachment: 207911 Committed r153581: <http://trac.webkit.org/changeset/153581>
All reviewed patches have been landed. Closing bug.