Bug 22379 - Make CSSOM use less memory
Summary: Make CSSOM use less memory
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-11-20 02:20 PST by Antti Koivisto
Modified: 2008-12-06 21:10 PST (History)
1 user (show)

See Also:


Attachments
some simple wins (7.12 KB, patch)
2008-11-20 02:29 PST, Antti Koivisto
no flags Details | Formatted Diff | Diff
reduce size of the CSSSelector (33.87 KB, patch)
2008-12-03 00:37 PST, Antti Koivisto
no flags Details | Formatted Diff | Diff
implemented bdash's comments (33.86 KB, patch)
2008-12-03 01:08 PST, Antti Koivisto
no flags Details | Formatted Diff | Diff
Store CSSSelectors to an array instead of a linked list (28.21 KB, patch)
2008-12-03 13:24 PST, Antti Koivisto
no flags Details | Formatted Diff | Diff
switch CSSMutableStyleDeclaration out from the DeprecatedValueList. (42.93 KB, patch)
2008-12-05 02:36 PST, Antti Koivisto
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antti Koivisto 2008-11-20 02:20:35 PST
CSSOM data structures have quite a bit of air at the moment.
Comment 1 Antti Koivisto 2008-11-20 02:29:23 PST
Created attachment 25308 [details]
some simple wins
Comment 2 Maciej Stachowiak 2008-11-20 02:34:27 PST
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.
Comment 3 Antti Koivisto 2008-11-20 02:50:15 PST
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.
Comment 4 Antti Koivisto 2008-11-20 02:51:04 PST
Comment on attachment 25308 [details]
some simple wins

Clearing the review flag, futher improvements are possible
Comment 5 Antti Koivisto 2008-12-03 00:37:45 PST
Created attachment 25703 [details]
reduce size of the CSSSelector
Comment 6 Antti Koivisto 2008-12-03 01:08:42 PST
Created attachment 25704 [details]
implemented bdash's comments
Comment 7 mitz 2008-12-03 01:15:25 PST
Comment on attachment 25704 [details]
implemented bdash's comments

r=me!
Comment 8 Antti Koivisto 2008-12-03 01:43:50 PST
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.
Comment 9 Antti Koivisto 2008-12-03 01:45:08 PST
Comment on attachment 25704 [details]
implemented bdash's comments

Clearing the review flag for more patches.
Comment 10 Antti Koivisto 2008-12-03 13:24:08 PST
Created attachment 25721 [details]
Store CSSSelectors to an array instead of a linked list
Comment 11 Darin Adler 2008-12-03 13:30:47 PST
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
Comment 12 Antti Koivisto 2008-12-03 14:26:34 PST
This was landed in r38964 (with the license headers)
Comment 13 Antti Koivisto 2008-12-03 14:26:54 PST
Comment on attachment 25721 [details]
Store CSSSelectors to an array instead of a linked list

Was landed, clearing the review flag.
Comment 14 Antti Koivisto 2008-12-05 02:36:16 PST
Created attachment 25768 [details]
switch CSSMutableStyleDeclaration out from the DeprecatedValueList.

Also did some simple optimizations to avoid moving memory around when not needed.
Comment 15 Darin Adler 2008-12-05 06:03:56 PST
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
Comment 16 Antti Koivisto 2008-12-06 17:46:14 PST
(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.
Comment 17 Antti Koivisto 2008-12-06 17:46:23 PST
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.

Comment 18 Antti Koivisto 2008-12-06 21:10:07 PST
This one is getting crowded, continuing in Bug 22717: Make CSS values use less memory