WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
22379
Make CSSOM use less memory
https://bugs.webkit.org/show_bug.cgi?id=22379
Summary
Make CSSOM use less memory
Antti Koivisto
Reported
2008-11-20 02:20:35 PST
CSSOM data structures have quite a bit of air at the moment.
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
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Antti Koivisto
Comment 1
2008-11-20 02:29:23 PST
Created
attachment 25308
[details]
some simple wins
Maciej Stachowiak
Comment 2
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.
Antti Koivisto
Comment 3
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.
Antti Koivisto
Comment 4
2008-11-20 02:51:04 PST
Comment on
attachment 25308
[details]
some simple wins Clearing the review flag, futher improvements are possible
Antti Koivisto
Comment 5
2008-12-03 00:37:45 PST
Created
attachment 25703
[details]
reduce size of the CSSSelector
Antti Koivisto
Comment 6
2008-12-03 01:08:42 PST
Created
attachment 25704
[details]
implemented bdash's comments
mitz
Comment 7
2008-12-03 01:15:25 PST
Comment on
attachment 25704
[details]
implemented bdash's comments r=me!
Antti Koivisto
Comment 8
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.
Antti Koivisto
Comment 9
2008-12-03 01:45:08 PST
Comment on
attachment 25704
[details]
implemented bdash's comments Clearing the review flag for more patches.
Antti Koivisto
Comment 10
2008-12-03 13:24:08 PST
Created
attachment 25721
[details]
Store CSSSelectors to an array instead of a linked list
Darin Adler
Comment 11
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
Antti Koivisto
Comment 12
2008-12-03 14:26:34 PST
This was landed in
r38964
(with the license headers)
Antti Koivisto
Comment 13
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.
Antti Koivisto
Comment 14
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.
Darin Adler
Comment 15
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
Antti Koivisto
Comment 16
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.
Antti Koivisto
Comment 17
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.
Antti Koivisto
Comment 18
2008-12-06 21:10:07 PST
This one is getting crowded, continuing in
Bug 22717
: Make CSS values use less memory
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug