Optimize Element attribute storage for the common case (no Attr objects.)

• Component: DOM
• Status: RESOLVED FIXED

[Andreas Kling]

The Element attribute storage in WebCore currently has ref-counted Attribute objects because they can have multiple owners (Element or Attr.)
We should refactor our way out of this so that Attribute doesn't need the ref count. Each attribute would shrink by 8 bytes, and we could store them in e.g Vector<Attribute> rather than Vector<RefPtr<Attribute>>, removing some indirection and possible/probable fragmentation.

My first idea is to rewrite Attr to wrap either an Element or qname/value pair.

[Andreas Kling]

Created attachment 136160 [details]
POC/WIP for EWS

[WebKit Review Bot]

Comment on attachment 136160 [details]
POC/WIP for EWS

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

[Andreas Kling]

Created attachment 137152 [details]
WIP2

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[WebKit Review Bot]

Attachment 137152 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/W..." exit_code: 1
Source/WebCore/dom/ElementAttributeData.cpp:30:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 137152 [details]
WIP2

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

[WebKit Review Bot]

Comment on attachment 137152 [details]
WIP2

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

[Antti Koivisto]

Comment on attachment 137152 [details]
WIP2

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

Looks great!

> Source/WebCore/dom/Attr.cpp:52
> +Attr::Attr(Document* document, const QualifiedName& name, const AtomicString& value)
> +    : ContainerNode(document)
> +    , m_element(0)
> +    , m_name(name)
> +    , m_value(value)

As I understand m_value is only used in special case of standalone Attr node that is not backed by an element. It should have a less generic name to make this clear.

> Source/WebCore/dom/Element.cpp:203
> +PassRefPtr<Attr> Element::takeAttribute(size_t index)
> +{
> +    if (!attributeData())
> +        return 0;
> +
> +    Attribute* attribute = attributeData()->attributeItem(index);
> +
> +    RefPtr<Attr> attr = getAttr(attribute->name());
> +    if (attr)
> +        attr->detachFromElementWithValue(attribute->value());
> +    else
> +        attr = Attr::create(document(), attribute->name(), attribute->value());
> +
> +    attributeData()->removeAttribute(index, this);
> +    return attr.release();
> +}

This seems bit strange. Might it be better to separate detaching of Attr (which seems to have other uses too) and the removal of the attribute?

> Source/WebCore/dom/Element.cpp:1438
> +    if (oldAttr)
> +        oldAttr->detachFromElementWithValue(oldAttribute->value());
> +    else
> +        oldAttr = Attr::create(document(), oldAttribute->name(), oldAttribute->value());

Like here.

> Source/WebCore/dom/Element.cpp:1507
> -        return;
> +        return; 
>  

Extra space.

> Source/WebCore/dom/Element.cpp:2107
> +PassRefPtr<Attr> Element::getAttr(const QualifiedName& name)

existingAttr()/attrIfExists()?

> Source/WebCore/dom/Element.cpp:2114
> +PassRefPtr<Attr> Element::getOrCreateAttr(const QualifiedName& name)

ensureAttr()?

> Source/WebCore/dom/Element.h:168
>      Attribute* attributeItem(unsigned index) const;
>      Attribute* getAttributeItem(const QualifiedName&) const;
> +    size_t getAttributeItemIndex(const QualifiedName& name) const { return attributeData()->getAttributeItemIndex(name); }
> +    size_t getAttributeItemIndex(const String& name, bool shouldIgnoreAttributeCase) const { return attributeData()->getAttributeItemIndex(name, shouldIgnoreAttributeCase); }

Propagating bad get*Item* naming here.

> Source/WebCore/dom/ElementAttributeData.h:40
> -class AttributeVector : public Vector<RefPtr<Attribute>, 4> {
> -    friend class ElementAttributeData;
> -
> +class AttributeVector : public Vector<Attribute, 4> {
>  public:

This should be cleaned up at some point.

> Source/WebCore/dom/ElementAttributeData.h:175
>  inline Attribute* ElementAttributeData::getAttributeItem(const String& name, bool shouldIgnoreAttributeCase) const
>  {
>      size_t index = getAttributeItemIndex(name, shouldIgnoreAttributeCase);
>      if (index != notFound)
> -        return m_attributes[index].get();
> +        return &const_cast<ElementAttributeData*>(this)->m_attributes[index];

Might want to consider a separate const and non-const versions to avoid ugliness here.

> Source/WebCore/html/parser/HTMLConstructionSite.cpp:197
> +    element->parserSetAttributes(token.attributes(), m_fragmentScriptingPermission);

We should take care that attributes vector is shrunken to minimal size. This matters more now since it contains object bigger than pointers.

[Andreas Kling]

Created attachment 137279 [details]
WIP3

[WebKit Review Bot]

Attachment 137279 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/W..." exit_code: 1
Source/WebCore/dom/ElementAttributeData.cpp:30:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 137279 [details]
WIP3

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

[WebKit Review Bot]

Comment on attachment 137279 [details]
WIP3

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

New failing tests:
fast/forms/select-size.html

[WebKit Review Bot]

Created attachment 137286 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Kent Tamura]

Comment on attachment 137279 [details]
WIP3

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

> Source/WebKit/chromium/public/WebAttribute.h:-69
>      WEBKIT_EXPORT WebString value() const;
> -
> -#if WEBKIT_IMPLEMENTATION
> -    WebAttribute(const WTF::PassRefPtr<WebCore::Attribute>&);
> -#endif
> -
> -private:
> -    WebPrivatePtr<WebCore::Attribute> m_private;

We need to keep workable WebAttribute. Maybe we should make WebAttribute a wrapper of WebCore::Attr instead of WebCore::Atribute.

[Antti Koivisto]

Comment on attachment 137279 [details]
WIP3

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

> Source/WebCore/dom/ElementAttributeData.h:43
> -    Attribute* attributeItem(unsigned index) const { return at(index).get(); }
> +    Attribute* attributeItem(unsigned index) const { return &const_cast<AttributeVector*>(this)->at(index); }

In the future should replace the remaining use of Attribute pointers with references. It is not obvious that the pointers will not be correct after mutating the attribute vector.

[James Robinson]

How would changing to references help with that problem?

[Antti Koivisto]

(In reply to comment #16)
> How would changing to references help with that problem?

We don't usually pass around pointers to members of vector. It helps by convention and by the fact that you explicitly need to get the address of such object. The same reason Vector<>::operator[] returns references essentially.

[Antti Koivisto]

Comment on attachment 137279 [details]
WIP3

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

> Source/WebCore/dom/Element.cpp:799
> +    AttributeVector attributes = m_attributeData->attributeVector();
> +    for (unsigned i = 0; i < attributes.size(); ++i)
> +        attributeChanged(&attributes[i]);

This could be called 'clonedAttributes' or similar.

> Source/WebCore/dom/Element.cpp:1760
> +    AttributeVector attributes = attributeData->attributeVector();
> +    for (size_t i = 0; i < attributes.size(); ++i) {
> +        if (RefPtr<Attr> attr = attrIfExists(attributes[i].name()))
>              attr->normalize();
>      }

Can this be const AttributeVector& and avoid copy?

> Source/WebCore/dom/ElementAttributeData.h:143
> +    AttributeVector attributeVector() const { return m_attributes; }

This should probably be

const AttributeVector& attributeVector() const { return m_attributes; }
AttributeVector attributeVector() { return m_attributes; }

> Source/WebCore/html/parser/HTMLConstructionSite.cpp:438
> +    AttributeVector attributes;
> +    if (ElementAttributeData* attributeData = element->updatedAttributeData())
> +        attributes = attributeData->attributeVector();
> +
> +    AtomicHTMLToken fakeToken(HTMLTokenTypes::StartTag, element->localName(), attributes);

Maybe call this clonedAttributes?

[Andreas Kling]

Created attachment 138188 [details]
Patch (I want to believe.)

[WebKit Review Bot]

Attachment 138188 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/W..." exit_code: 1
Source/WebCore/dom/ElementAttributeData.cpp:30:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Antti Koivisto]

r=me, looks great

[Build Bot]

Comment on attachment 138188 [details]
Patch (I want to believe.)

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

[WebKit Review Bot]

Comment on attachment 138188 [details]
Patch (I want to believe.)

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

[Andreas Kling]

Created attachment 138285 [details]
Same patch for landalizing.

[WebKit Review Bot]

Comment on attachment 138285 [details]
Same patch for landalizing.

Rejecting attachment 138285 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
Debug/obj.target/webkit/Source/WebKit/chromium/src/WebDevToolsFrontendImpl.o
  CXX(target) out/Debug/obj.target/webkit/Source/WebKit/chromium/src/WebDocument.o
  CXX(target) out/Debug/obj.target/webkit/Source/WebKit/chromium/src/WebElement.o
Source/WebKit/chromium/src/WebElement.cpp:42: fatal error: WebNamedNodeMap.h: No such file or directory
compilation terminated.
make: *** [out/Debug/obj.target/webkit/Source/WebKit/chromium/src/WebElement.o] Error 1
make: *** Waiting for unfinished jobs....

Full output: http://queues.webkit.org/results/12469731

[Andreas Kling]

Created attachment 138289 [details]
Patch for landing (less derp.)

[WebKit Review Bot]

Comment on attachment 138289 [details]
Patch for landing (less derp.)

Clearing flags on attachment: 138289

Committed r114870: <http://trac.webkit.org/changeset/114870>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

It seems like there's 10-15% improvements on DOM/GetElement :D

http://webkit-perf.appspot.com/graph.html#tests=[[19048,2001,173262],[19048,2001,963028],[19048,2001,957069]]&sel=1334628815979,1335233615979&displayrange=7&datatype=running

[Antti Koivisto]

*** Bug 75071 has been marked as a duplicate of this bug. ***