Bug 88240 – Make ElementAttributeData a variable-sized object to reduce memory use.

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Andreas Kling]

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.

[Andreas Kling]

Created attachment 145604 [details]
Pøtch

[WebKit Review Bot]

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 on attachment 145604 [details]
Pøtch

Attachment 145604 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12905190

[Antti Koivisto]

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]

Committed r119421: <http://trac.webkit.org/changeset/119421>

[Csaba Osztrogonác]

(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]

Re-opened since this is blocked by 88256

[Andreas Kling]

Created attachment 151261 [details]
EWSxperiment

[WebKit Review Bot]

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 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 on attachment 151261 [details]
EWSxperiment

Attachment 151261 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13157875

[Andreas Kling]

Created attachment 151269 [details]
EWSxperiment 2

[WebKit Review Bot]

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 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]

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]

Created attachment 151711 [details]
EWSxperiment 3

[Antti Koivisto]

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]

Committed r122450: <http://trac.webkit.org/changeset/122450>

[WebKit Review Bot]

Re-opened since this is blocked by 91263

[Andreas Kling]

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 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 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]

(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]

Created attachment 154165 [details]
Patch

[Andreas Kling]

Created attachment 154209 [details]
Patch

C'mon new EWS.

[Andreas Kling]

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 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 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]

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]

(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]

Created attachment 154353 [details]
Patch

[WebKit Review Bot]

Comment on attachment 154353 [details]
Patch

Clearing flags on attachment: 154353

Committed r123636: <http://trac.webkit.org/changeset/123636>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.