Bug 88240

Summary: Make ElementAttributeData a variable-sized object to reduce memory use.
Product: WebKit Reporter: Andreas Kling <kling>
Component: DOMAssignee: Andreas Kling <kling>
Status: RESOLVED FIXED    
Severity: Normal CC: cmarcelo, dglazkov, macpherson, menard, mifenton, ossy, rniwa, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 88256, 91263    
Bug Blocks:    
Attachments:
Description Flags
Pøtch
koivisto: review+, webkit-ews: commit-queue-
EWSxperiment
webkit-ews: commit-queue-
EWSxperiment 2
webkit.review.bot: commit-queue-
Archive of layout-test-results from gce-cr-linux-06
none
EWSxperiment 3
koivisto: review+
Patch
none
Patch
none
Patch
webkit.review.bot: commit-queue-
Archive of layout-test-results from gce-cr-linux-02
none
Patch none

Andreas Kling
Reported 2012-06-04 10:14:29 PDT
Most of the time, we know how many attributes an element needs to accomodate at the time we construct the ElementAttributeData. We should take advantage of this by allocating only as much space as we really need and avoiding the bloat from using Vector<Attribute>. We can then use the same scheme as StylePropertySet and move the data to a vector dynamically when the attribute map is mutated.
Attachments
Pøtch (43.21 KB, patch)
2012-06-04 10:32 PDT, Andreas Kling
koivisto: review+
webkit-ews: commit-queue-
EWSxperiment (47.49 KB, patch)
2012-07-09 09:44 PDT, Andreas Kling
webkit-ews: commit-queue-
EWSxperiment 2 (48.49 KB, patch)
2012-07-09 10:15 PDT, Andreas Kling
webkit.review.bot: commit-queue-
Archive of layout-test-results from gce-cr-linux-06 (590.72 KB, application/zip)
2012-07-09 11:26 PDT, WebKit Review Bot
no flags
EWSxperiment 3 (50.93 KB, patch)
2012-07-11 08:27 PDT, Andreas Kling
koivisto: review+
Patch (79.08 KB, patch)
2012-07-23 08:58 PDT, Andreas Kling
no flags
Patch (80.26 KB, patch)
2012-07-24 16:10 PDT, Andreas Kling
no flags
Patch (79.48 KB, patch)
2012-07-24 18:29 PDT, Andreas Kling
webkit.review.bot: commit-queue-
Archive of layout-test-results from gce-cr-linux-02 (1.02 MB, application/zip)
2012-07-24 23:04 PDT, WebKit Review Bot
no flags
Patch (77.21 KB, patch)
2012-07-25 07:43 PDT, Andreas Kling
no flags
Andreas Kling
Comment 1 2012-06-04 10:32:05 PDT
WebKit Review Bot
Comment 2 2012-06-04 10:34:36 PDT
Attachment 145604 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/dom/ElementAttributeData.h:85: The parameter name "name" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/dom/ElementAttributeData.h:86: The parameter name "name" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 2 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.
Early Warning System Bot
Comment 3 2012-06-04 11:27:40 PDT
Antti Koivisto
Comment 4 2012-06-04 11:45:20 PDT
Comment on attachment 145604 [details] Pøtch View in context: https://bugs.webkit.org/attachment.cgi?id=145604&action=review r=me, nice! > Source/WebCore/dom/Element.cpp:684 > inline void Element::setAttributeInternal(size_t index, const QualifiedName& name, const AtomicString& value, EInUpdateStyleAttribute inUpdateStyleAttribute) > { > + mutableAttributeData(); This looks lonely. It would perhaps be nicer to use the returned value instead of accessing attributes directly. > Source/WebCore/dom/Element.cpp:2123 > +void Element::createMutableAttributeData() > +{ > + if (!m_attributeData) > + m_attributeData = ElementAttributeData::create(); It would be nicer if this was called ElementAttributeData::createMutable() (same for StylePropertySet::create() too) > Source/WebCore/dom/ElementAttributeData.cpp:54 > +ElementAttributeData::ElementAttributeData(const Vector<Attribute>& attributes) > + : m_isMutable(false) > + , m_arraySize(attributes.size()) > +{ > + for (unsigned i = 0; i < attributes.size(); ++i) > + new (&array()[i]) Attribute(attributes[i]); > +} Anders tells that we should be able to generalize this pattern into a template. Would be nice to do that at some point as we can probably find other uses.
Andreas Kling
Comment 5 2012-06-04 13:01:24 PDT
Csaba Osztrogonác
Comment 6 2012-06-04 13:13:37 PDT
(In reply to comment #5) > Committed r119421: <http://trac.webkit.org/changeset/119421> Hey man, why did you commit it, when the Qt EWS bubble was red? Could you fix the build?
WebKit Review Bot
Comment 7 2012-06-04 14:23:59 PDT
Re-opened since this is blocked by 88256
Andreas Kling
Comment 8 2012-07-09 09:44:06 PDT
Created attachment 151261 [details] EWSxperiment
WebKit Review Bot
Comment 9 2012-07-09 09:45:55 PDT
Attachment 151261 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/dom/ElementAttributeData.h:83: The parameter name "name" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/dom/ElementAttributeData.h:85: The parameter name "name" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 2 in 9 files If any of these errors are false positives, please file a bug against check-webkit-style.
Early Warning System Bot
Comment 10 2012-07-09 10:04:18 PDT
Comment on attachment 151261 [details] EWSxperiment Attachment 151261 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/13165333
WebKit Review Bot
Comment 11 2012-07-09 10:11:48 PDT
Comment on attachment 151261 [details] EWSxperiment Attachment 151261 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13157875
Andreas Kling
Comment 12 2012-07-09 10:15:16 PDT
Created attachment 151269 [details] EWSxperiment 2
WebKit Review Bot
Comment 13 2012-07-09 10:18:13 PDT
Attachment 151269 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/dom/ElementAttributeData.h:83: The parameter name "name" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/dom/ElementAttributeData.h:85: The parameter name "name" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 2 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.
WebKit Review Bot
Comment 14 2012-07-09 11:26:14 PDT
Comment on attachment 151269 [details] EWSxperiment 2 Attachment 151269 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13174145 New failing tests: svg/css/svg-ellipse-render-crash.html
WebKit Review Bot
Comment 15 2012-07-09 11:26:25 PDT
Created attachment 151286 [details] Archive of layout-test-results from gce-cr-linux-06 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-06 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Andreas Kling
Comment 16 2012-07-11 08:27:03 PDT
Created attachment 151711 [details] EWSxperiment 3
Antti Koivisto
Comment 17 2012-07-12 04:02:47 PDT
Comment on attachment 151711 [details] EWSxperiment 3 View in context: https://bugs.webkit.org/attachment.cgi?id=151711&action=review r=me, glorious > Source/WebCore/dom/Element.cpp:244 > + // FIXME: This shouldn't have to make the ElementAttributeData mutable. > + const_cast<Element*>(this)->ensureUpdatedAttributeData(); Is there is really anything wrong with this? The FIXME might be an overkill (and point to over-optimization). Un-consting the function would remove that awkward const_cast. > Source/WebCore/dom/ElementAttributeData.cpp:69 > + , m_mutableAttributeVector(new Vector<Attribute, 4>) > +{ > + // This copy constructor should only be used by makeMutable() to go from immutable to mutable. > + ASSERT(!other.m_isMutable); > + for (unsigned i = 0; i < other.m_arraySize; ++i) > + m_mutableAttributeVector->append(other.array()[i]); > +} This could use reserverInitialCapacity() and uncheckedAppend(). Not sure how to calculate the optimal initial capacity. Maybe just 1 more than the current size?
Andreas Kling
Comment 18 2012-07-12 05:50:58 PDT
WebKit Review Bot
Comment 19 2012-07-13 10:22:22 PDT
Re-opened since this is blocked by 91263
Andreas Kling
Comment 20 2012-07-23 08:58:36 PDT
Created attachment 153807 [details] Patch Okay, let's give this another try. This time with liberal use of the "const" keyword to catch any incorrect ElementAttributeData usage at compile-time.
Andreas Kling
Comment 21 2012-07-23 09:00:47 PDT
Comment on attachment 153807 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=153807&action=review > Source/WebCore/ChangeLog:155 > + Updated code to handle both immutable and mutable source objects. This could ...or could it?
Ryosuke Niwa
Comment 22 2012-07-23 19:48:11 PDT
Comment on attachment 153807 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=153807&action=review > Source/WebCore/dom/Element.cpp:1398 > RefPtr<Attr> oldAttr = attrIfExists(attr->qualifiedName()); It appears that attrIfExists doesn't update the invalid attributes. > Source/WebCore/dom/ElementAttributeData.cpp:37 > + void* slot = WTF::fastMalloc(sizeof(ElementAttributeData) - sizeof(void*) + sizeof(Attribute) * attributes.size()); We don't have a fancy WTF macros to deal with alignments? > Source/WebCore/dom/ElementAttributeData.cpp:82 > + for (unsigned i = 0; i < m_arraySize; ++i) > + array()[i].~Attribute(); I think it's better to wrap this in the else clause. I would also assert that m_arraySize is 0 when isMutable(). > Source/WebCore/dom/ElementAttributeData.h:140 > + void* m_attributes; Why can't we use Attribute* here?
Andreas Kling
Comment 23 2012-07-24 16:04:42 PDT
(In reply to comment #22) > (From update of attachment 153807 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=153807&action=review > > > Source/WebCore/dom/Element.cpp:1398 > > RefPtr<Attr> oldAttr = attrIfExists(attr->qualifiedName()); > > It appears that attrIfExists doesn't update the invalid attributes. Good catch. Fixing that and adding a test so it's covered in the future. > > Source/WebCore/dom/ElementAttributeData.cpp:37 > > + void* slot = WTF::fastMalloc(sizeof(ElementAttributeData) - sizeof(void*) + sizeof(Attribute) * attributes.size()); > > We don't have a fancy WTF macros to deal with alignments? Not sure what you're talking about. Subtracting sizeof(void*) is to get the base size of the object excluding the union. > > Source/WebCore/dom/ElementAttributeData.cpp:82 > > + for (unsigned i = 0; i < m_arraySize; ++i) > > + array()[i].~Attribute(); > > I think it's better to wrap this in the else clause. I would also assert that m_arraySize is 0 when isMutable(). Will do. > > Source/WebCore/dom/ElementAttributeData.h:140 > > + void* m_attributes; > > Why can't we use Attribute* here? We could use any type, at the end of the day we have to reinterpret_cast<Attribute*>(&m_attributes) anyway. It would look nicer if the compiler would let us do "Attribute m_attributes[0]" or something like that, but that doesn't fly in a union. :/
Andreas Kling
Comment 24 2012-07-24 16:10:45 PDT
Andreas Kling
Comment 25 2012-07-24 18:29:24 PDT
Created attachment 154209 [details] Patch C'mon new EWS.
Andreas Kling
Comment 26 2012-07-24 19:34:16 PDT
Dumping IRC log since rniwa pinged out. rniwa: kling: one more question about the patch rniwa: kling: what happens if CSSMutableStyle had been created for inline style rniwa: kling: when the attributes were immutable (i.e. stored in ElementAttributeData) rniwa: kling: and then attributes got mutable via setAttribute on the element (not via CSSOM wrapper for the inline style) rniwa: kling: doesn't the m_propertySet in the wrapper get stable in that case? kling: rniwa: the StylePropertySet pointer doesn't change when making a new mutable ElementAttributeData kling: rniwa: it inherits the old one from the immutable EAD kling: rniwa: or what do you mean by stable? kling: rniwa: if you look at ElementAttributeData(const ElementAttributeData& other), you can see that all members are cloned kling: rniwa: the source object is discarded after this constructor runs (the immutable EAD has now been replaced by a mutable EAD) kling: rniwa: (so there aren't multiple EADs pointing to the same m_propertySet)
Ryosuke Niwa
Comment 27 2012-07-24 19:43:19 PDT
Comment on attachment 154209 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=154209&action=review > Source/WebCore/css/PropertySetCSSStyleDeclaration.cpp:410 > + if (mutablePropertySet != m_propertySet) { When does this happen? > Source/WebCore/dom/ElementAttributeData.cpp:235 > ASSERT(index < length()); Missing ASSERT(isMutable()); here. > Source/WebCore/dom/StyledElement.cpp:237 > + bool changes = mutableAttributeData()->inlineStyle()->removeProperty(propertyID); We can't call ensureInlineStyle() here?
WebKit Review Bot
Comment 28 2012-07-24 23:03:58 PDT
Comment on attachment 154209 [details] Patch Attachment 154209 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13341133 New failing tests: fast/loader/loadInProgress.html fast/inline/positionedLifetime.html fast/loader/unload-form-post-about-blank.html
WebKit Review Bot
Comment 29 2012-07-24 23:04:03 PDT
Created attachment 154263 [details] Archive of layout-test-results from gce-cr-linux-02 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-02 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Andreas Kling
Comment 30 2012-07-25 07:06:45 PDT
(In reply to comment #27) > (From update of attachment 154209 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=154209&action=review > > > Source/WebCore/css/PropertySetCSSStyleDeclaration.cpp:410 > > + if (mutablePropertySet != m_propertySet) { > > When does this happen? Never! D'oh! When InlineCSSStyleDeclaration is created, we always have a mutable StylePropertySet already. I'll replace this by an assertion that isMutable(). The code can be cleaned up further, I'll do that in a follow-up. > > Source/WebCore/dom/ElementAttributeData.cpp:235 > > ASSERT(index < length()); > > Missing ASSERT(isMutable()); here. Good idea. > > Source/WebCore/dom/StyledElement.cpp:237 > > + bool changes = mutableAttributeData()->inlineStyle()->removeProperty(propertyID); > > We can't call ensureInlineStyle() here? Sure we can!
Andreas Kling
Comment 31 2012-07-25 07:43:41 PDT
WebKit Review Bot
Comment 32 2012-07-25 11:14:36 PDT
Comment on attachment 154353 [details] Patch Clearing flags on attachment: 154353 Committed r123636: <http://trac.webkit.org/changeset/123636>
WebKit Review Bot
Comment 33 2012-07-25 11:14:43 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.