Bug 117715 – Reduce CSSProperty's StylePropertyMetadata memory footprint by half when used inside a ImmutableStylePropertySet.

• Component: CSS
• Status: RESOLVED FIXED

[Alexis Menard (darktears)]

Reduce CSSProperty's StylePropertyMetadata memory footprint by half when used inside a ImmutableStylePropertySet.

[Alexis Menard (darktears)]

Created attachment 204853 [details]
Patch

[Alexis Menard (darktears)]

(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?

[Build Bot]

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

[Build Bot]

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

[Alexis Menard (darktears)]

(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

[Ryosuke Niwa]

Landed in Blink:
https://chromium.googlesource.com/chromium/blink/+/4b7ca0e5f34d3004770dc12e27894df698738454

[Andreas Kling]

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.

[Build Bot]

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

[Build Bot]

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

[Alexis Menard (darktears)]

Created attachment 205110 [details]
Patch

[Alexis Menard (darktears)]

Created attachment 205220 [details]
Patch

[Andreas Kling]

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.

[Alexis Menard (darktears)]

(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.

[Ryosuke Niwa]

It seems like the blink patch got rolled out due to a regression?

[Alexis Menard (darktears)]

*** Bug 118013 has been marked as a duplicate of this bug. ***

[Alexis Menard (darktears)]

(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.

[Alexis Menard (darktears)]

Created attachment 207863 [details]
Patch

[Andreas Kling]

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|...
> +        ------------------------------------------------------------------------------------------------------

^

[Alexis Menard (darktears)]

Created attachment 207911 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 207911 [details]
Patch for landing

Clearing flags on attachment: 207911

Committed r153581: <http://trac.webkit.org/changeset/153581>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.