Summary: | Make CSSOM use less memory | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Antti Koivisto <koivisto> | ||||||||||||
Component: | CSS | Assignee: | Nobody <webkit-unassigned> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | ddkilzer | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | Mac | ||||||||||||||
OS: | OS X 10.5 | ||||||||||||||
Attachments: |
|
Description
Antti Koivisto
2008-11-20 02:20:35 PST
Created attachment 25308 [details]
some simple wins
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.
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 on attachment 25308 [details]
some simple wins
Clearing the review flag, futher improvements are possible
Created attachment 25703 [details]
reduce size of the CSSSelector
Created attachment 25704 [details]
implemented bdash's comments
Comment on attachment 25704 [details]
implemented bdash's comments
r=me!
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 on attachment 25704 [details]
implemented bdash's comments
Clearing the review flag for more patches.
Created attachment 25721 [details]
Store CSSSelectors to an array instead of a linked list
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 This was landed in r38964 (with the license headers) Comment on attachment 25721 [details]
Store CSSSelectors to an array instead of a linked list
Was landed, clearing the review flag.
Created attachment 25768 [details]
switch CSSMutableStyleDeclaration out from the DeprecatedValueList.
Also did some simple optimizations to avoid moving memory around when not needed.
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 (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. 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. This one is getting crowded, continuing in Bug 22717: Make CSS values use less memory |