Summary: | ElementAttributeData: Use subclasses to manage varying object layouts. | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Andreas Kling <kling> | ||||||||
Component: | DOM | Assignee: | Andreas Kling <kling> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | cmarcelo, eric, jochen, koivisto, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | 96326 | ||||||||||
Bug Blocks: | |||||||||||
Attachments: |
|
Description
Andreas Kling
2012-08-20 04:46:22 PDT
Created attachment 159398 [details]
Patch
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 on attachment 159398 [details] Patch Attachment 159398 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13530938 (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.. 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 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 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. Committed r128109: <http://trac.webkit.org/changeset/128109> Re-opened since this is blocked by 96326 Created attachment 163436 [details]
Patch III: Revenge of the reinterpret_cast
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> All reviewed patches have been landed. Closing bug. 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? |