Bug 94465 – ElementAttributeData: Use subclasses to manage varying object layouts.

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

[Andreas Kling]

There's a bunch of memory optimizations we can do for ElementAttributeData, but it's going to get pretty dirty.
Let's start out by switching to a scheme where we use subclasses and casting to manage the (currently 2) different object layouts.

[Andreas Kling]

Created attachment 159398 [details]
Patch

[Antti Koivisto]

Comment on attachment 159398 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=159398&action=review

> Source/WebCore/dom/Element.cpp:802
> -    m_attributeData = ElementAttributeData::createImmutable(filteredAttributes);
> +    m_attributeData = ElementAttributeData::createFixedSize(filteredAttributes);

Immutability offers possibility for sharing so it might make sense to keep that concept.

> Source/WebCore/dom/ElementAttributeData.cpp:229
> +    static_cast<VariableSizeElementAttributeData*>(this)->m_attributeVector.append(attribute);

You should add RenderTree style casting functions, asVariableSize()/asFixedSize() or similar.

[Build Bot]

Comment on attachment 159398 [details]
Patch

Attachment 159398 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13530938

[Andreas Kling]

(In reply to comment #2)
> (From update of attachment 159398 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=159398&action=review
> 
> > Source/WebCore/dom/Element.cpp:802
> > -    m_attributeData = ElementAttributeData::createImmutable(filteredAttributes);
> > +    m_attributeData = ElementAttributeData::createFixedSize(filteredAttributes);
> 
> Immutability offers possibility for sharing so it might make sense to keep that concept.

That's a very cool story actually, bro. Putting this on hold while I investigate attribute sharing..

[Andreas Kling]

Created attachment 163027 [details]
Patch II: Back from the Dead

Okay, now that ElementAttributeData sharing is implemented, let's do this to get rid of the heap allocation (and indirection) for mutable ElementAttributeData.

[Build Bot]

Comment on attachment 163027 [details]
Patch II: Back from the Dead

Attachment 163027 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13796711

[Antti Koivisto]

Comment on attachment 163027 [details]
Patch II: Back from the Dead

View in context: https://bugs.webkit.org/attachment.cgi?id=163027&action=review

Nice, makes the whole thing more comprehensible. r=me.

> Source/WebCore/dom/ElementAttributeData.cpp:87
> +PassRefPtr<ElementAttributeData> ElementAttributeData::makeMutable() const
> +{
> +    ASSERT(!isMutable());
> +    return adoptRef(new MutableElementAttributeData(asImmutableElementAttributeData()));
>  }

makeMutableCopy() perhaps?

> Source/WebCore/dom/ElementAttributeData.cpp:231
> -    m_mutableAttributeVector->append(attribute);
> +    asMutableElementAttributeData().m_attributeVector.append(attribute);

mutableAttributeVector()/immutableAttributeArray() helpers would reduce repetition of the casting function.

[Andreas Kling]

Committed r128109: <http://trac.webkit.org/changeset/128109>

[WebKit Review Bot]

Re-opened since this is blocked by 96326

[Andreas Kling]

Created attachment 163436 [details]
Patch III: Revenge of the reinterpret_cast

[WebKit Review Bot]

Comment on attachment 163436 [details]
Patch III: Revenge of the reinterpret_cast

Clearing flags on attachment: 163436

Committed r128239: <http://trac.webkit.org/changeset/128239>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Antti Koivisto]

Comment on attachment 163436 [details]
Patch III: Revenge of the reinterpret_cast

View in context: https://bugs.webkit.org/attachment.cgi?id=163436&action=review

> Source/WebCore/dom/ElementAttributeData.h:144
> +    void* m_attributeArray;

Why oh why?