Bug 22379 – Make CSSOM use less memory

• Component: CSS
• Status: RESOLVED FIXED

[Antti Koivisto]

CSSOM data structures have quite a bit of air at the moment.

[Antti Koivisto]

Created attachment 25308 [details]
some simple wins

[Maciej Stachowiak]

Comment on attachment 25308 [details]
some simple wins

r=me

I think it would be a good idea to perf-test, though the only part I'd really worry about is the 15-bit and 31-bit bitfields.

Not required though.

[Antti Koivisto]

Sending        WebCore/ChangeLog
Sending        WebCore/css/CSSMutableStyleDeclaration.cpp
Sending        WebCore/css/CSSMutableStyleDeclaration.h
Sending        WebCore/css/CSSProperty.h
Sending        WebCore/css/CSSStyleSheet.cpp
Sending        WebCore/css/CSSStyleSheet.h
Sending        WebCore/css/StyleBase.h
Transmitting file data .......
Committed revision 38617.

[Antti Koivisto]

Comment on attachment 25308 [details]
some simple wins

Clearing the review flag, futher improvements are possible

[Antti Koivisto]

Created attachment 25703 [details]
reduce size of the CSSSelector

[Antti Koivisto]

Created attachment 25704 [details]
implemented bdash's comments

Comment on attachment 25704 [details]
implemented bdash's comments

r=me!

[Antti Koivisto]

Sending        WebCore/ChangeLog
Sending        WebCore/GNUmakefile.am
Sending        WebCore/WebCore.pro
Sending        WebCore/WebCore.scons
Sending        WebCore/WebCore.vcproj/WebCore.vcproj
Sending        WebCore/WebCore.xcodeproj/project.pbxproj
Sending        WebCore/css/CSSGrammar.y
Deleting       WebCore/css/CSSNthSelector.cpp
Deleting       WebCore/css/CSSNthSelector.h
Sending        WebCore/css/CSSParser.cpp
Sending        WebCore/css/CSSParser.h
Sending        WebCore/css/CSSSelector.cpp
Sending        WebCore/css/CSSSelector.h
Sending        WebCore/css/CSSStyleSelector.cpp
Transmitting file data ............
Committed revision 38934.

[Antti Koivisto]

Comment on attachment 25704 [details]
implemented bdash's comments

Clearing the review flag for more patches.

[Antti Koivisto]

Created attachment 25721 [details]
Store CSSSelectors to an array instead of a linked list

[Darin Adler]

Comment on attachment 25721 [details]
Store CSSSelectors to an array instead of a linked list

> Index: WebCore/css/CSSSelectorList.cpp
> ===================================================================
> --- WebCore/css/CSSSelectorList.cpp	(revision 0)
> +++ WebCore/css/CSSSelectorList.cpp	(revision 0)
> @@ -0,0 +1,55 @@
> +#include "config.h"
> +
> +#include "CSSSelectorList.h"

This needs a license header. Also, we don't usually put spaces between these two includes.

> Index: WebCore/css/CSSSelectorList.h
> ===================================================================
> --- WebCore/css/CSSSelectorList.h	(revision 0)
> +++ WebCore/css/CSSSelectorList.h	(revision 0)
> @@ -0,0 +1,31 @@
> +#ifndef CSSSelectorList_h
> +#define CSSSelectorList_h

This needs a license header.

It's a little annoying that this class is called "List" when that so often means linked list, but I think that's the best name.

r=me

[Antti Koivisto]

This was landed in r38964 (with the license headers)

[Antti Koivisto]

Comment on attachment 25721 [details]
Store CSSSelectors to an array instead of a linked list

Was landed, clearing the review flag.

[Antti Koivisto]

Created attachment 25768 [details]
switch CSSMutableStyleDeclaration out from the DeprecatedValueList.

Also did some simple optimizations to avoid moving memory around when not needed.

[Darin Adler]

Comment on attachment 25768 [details]
switch CSSMutableStyleDeclaration out from the DeprecatedValueList.

> +    Vector<CSSProperty>::const_iterator it = findPropertyWithId(propertyID);

Something I always tell people who are working with Vector is that iterators are just pointers and thus I think it's unnecessary to use the typedef or use end() to indicate "not found". So I would have done:

    const CSSProperty* property = findPropertyWithId(propertyID);

And I would have make it return 0 when not found rather than end().

> +    String value = returnText ? (*it).value()->cssText() : String();

I think it->value() reads better than (*it).value.

> +            (*it) = property;

No parentheses needed here.

>  void CSSMutableStyleDeclaration::removePropertiesInSet(const int* set, unsigned length, bool notifyChanged)
>  {
> -    bool changed = false;
> -    for (unsigned i = 0; i < length; i++) {
> -        RefPtr<CSSValue> value = getPropertyCSSValue(set[i]);
> -        if (value) {
> -            m_values.remove(CSSProperty(set[i], value.release(), false));
> -            changed = true;
> +    ASSERT(!m_iteratorCount);
> +    
> +    if (m_properties.isEmpty())
> +        return;
> +    
> +    // FIXME: This is often used with static sets and in that case constructing the hash repeatedly is pretty pointless.
> +    HashSet<unsigned> toRemove;
> +    for (unsigned i = 0; i < length; i++)
> +        toRemove.add(set[i]);

I think we should change this function to take a HashSet and make a helper function to use with it. I think the function is *always* used with static sets.

I don't understand why you chose HashSet<unsigned> rather than HashSet<int> or HashSet<CSSPropertyID>.

> Index: WebCore/css/CSSProperty.h
> ===================================================================
> --- WebCore/css/CSSProperty.h	(revision 39020)
> +++ WebCore/css/CSSProperty.h	(working copy)
> @@ -45,6 +45,7 @@ public:
>          m_id = other.m_id;
>          m_shorthandID = other.m_shorthandID;
>          m_important = other.m_important;
> +        m_implicit = other.m_implicit;
>          m_value = other.m_value;
>          return *this;
>      }


Maybe you could land this change sepaarately?

> +namespace WTF {
> +    template<> struct VectorTraits<WebCore::CSSProperty> : SimpleClassVectorTraits { };
> +}

I think this needs a comment to explain what it means and why we're doing it.

> -    Vector<int> properties;
> -    DeprecatedValueListConstIterator<CSSProperty> end;
> -    for (DeprecatedValueListConstIterator<CSSProperty> it(style->valuesIterator()); it != end; ++it) {
> -        const CSSProperty& property = *it;
> -        RefPtr<CSSValue> value = getPropertyCSSValue(property.id());
> -        if (value && (value->cssText() == property.value()->cssText()))
> -            properties.append(property.id());
> +    Vector<int> propertiesToRemove;
> +    {
> +        CSSMutableStyleDeclaration::const_iterator end = style->end();
> +        for (CSSMutableStyleDeclaration::const_iterator it = style->begin(); it != end; ++it) {
> +            const CSSProperty& property = *it;
> +            RefPtr<CSSValue> value = getPropertyCSSValue(property.id());
> +            if (value && (value->cssText() == property.value()->cssText()))
> +                propertiesToRemove.append(property.id());
> +        }
>      }
>  
> -    for (unsigned i = 0; i < properties.size(); i++)
> -        style->removeProperty(properties[i]);
> +    // FIXME: This should use mass removal.
> +    for (unsigned i = 0; i < propertiesToRemove.size(); i++)
> +        style->removeProperty(propertiesToRemove[i]);
>  }

Why not change this to use removePropertiesFromSet now? If you're going to add a comment, please mention the name of the function specifically.

How did you test the impact on speed?

r=me

[Antti Koivisto]

(In reply to comment #15)
> (From update of attachment 25768 [details] [review])
> > +    Vector<CSSProperty>::const_iterator it = findPropertyWithId(propertyID);
> 
> Something I always tell people who are working with Vector is that iterators
> are just pointers and thus I think it's unnecessary to use the typedef or use
> end() to indicate "not found". So I would have done:

I was planning to do an empty slot optimization but then decided that it is not really needed. Use of iterators was a leftover from that.
 
>     const CSSProperty* property = findPropertyWithId(propertyID);

Done.

> I think we should change this function to take a HashSet and make a helper
> function to use with it. I think the function is *always* used with static
> sets.

Left as a future exercise.

> > +        m_implicit = other.m_implicit;
> >          m_value = other.m_value;

> Maybe you could land this change sepaarately?

Ok.

> Why not change this to use removePropertiesFromSet now? If you're going to add
> a comment, please mention the name of the function specifically.

It not fully equivalent though perhaps it should be. I didn't want to introduce many behavioral changes.

Left as a future exercise.
 
> How did you test the impact on speed?

PLT seemed to show a slight progression.

[Antti Koivisto]

Sending        LayoutTests/ChangeLog
Sending        LayoutTests/editing/pasteboard/5780697-2-expected.txt
Sending        WebCore/ChangeLog
Sending        WebCore/WebCore.xcodeproj/project.pbxproj
Sending        WebCore/css/CSSMutableStyleDeclaration.cpp
Sending        WebCore/css/CSSMutableStyleDeclaration.h
Sending        WebCore/css/CSSProperty.h
Sending        WebCore/css/CSSStyleDeclaration.cpp
Sending        WebCore/css/CSSStyleSelector.cpp
Sending        WebCore/dom/EventTarget.h
Sending        WebCore/editing/ApplyStyleCommand.cpp
Sending        WebCore/editing/Editor.cpp
Sending        WebCore/editing/ReplaceSelectionCommand.cpp
Sending        WebCore/editing/markup.cpp
Transmitting file data ..............
Committed revision 39075.

[Antti Koivisto]

This one is getting crowded, continuing in Bug 22717: Make CSS values use less memory