Bug 94465 - ElementAttributeData: Use subclasses to manage varying object layouts.
Summary: ElementAttributeData: Use subclasses to manage varying object layouts.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andreas Kling
URL:
Keywords:
Depends on: 96326
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-20 04:46 PDT by Andreas Kling
Modified: 2012-09-12 04:12 PDT (History)
5 users (show)

See Also:


Attachments
Patch (21.62 KB, patch)
2012-08-20 05:14 PDT, Andreas Kling
buildbot: commit-queue-
Details | Formatted Diff | Diff
Patch II: Back from the Dead (17.37 KB, patch)
2012-09-09 20:09 PDT, Andreas Kling
koivisto: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
Patch III: Revenge of the reinterpret_cast (17.37 KB, patch)
2012-09-11 14:23 PDT, Andreas Kling
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andreas Kling 2012-08-20 04:46:22 PDT
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.
Comment 1 Andreas Kling 2012-08-20 05:14:46 PDT
Created attachment 159398 [details]
Patch
Comment 2 Antti Koivisto 2012-08-20 05:33:50 PDT
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.
Comment 3 Build Bot 2012-08-20 05:55:15 PDT
Comment on attachment 159398 [details]
Patch

Attachment 159398 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13530938
Comment 4 Andreas Kling 2012-08-20 09:13:06 PDT
(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..
Comment 5 Andreas Kling 2012-09-09 20:09:08 PDT
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.
Comment 6 Build Bot 2012-09-09 20:42:23 PDT
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
Comment 7 Antti Koivisto 2012-09-10 01:43:45 PDT
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.
Comment 8 Andreas Kling 2012-09-10 14:43:57 PDT
Committed r128109: <http://trac.webkit.org/changeset/128109>
Comment 9 WebKit Review Bot 2012-09-10 15:00:10 PDT
Re-opened since this is blocked by 96326
Comment 10 Andreas Kling 2012-09-11 14:23:52 PDT
Created attachment 163436 [details]
Patch III: Revenge of the reinterpret_cast
Comment 11 WebKit Review Bot 2012-09-11 16:25:49 PDT
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>
Comment 12 WebKit Review Bot 2012-09-11 16:25:53 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Antti Koivisto 2012-09-12 04:12:08 PDT
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?