Summary: | Optimize Element attribute storage for the common case (no Attr objects.) | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Andreas Kling <kling> | ||||||||||||||||
Component: | DOM | Assignee: | Andreas Kling <kling> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | abarth, cmarcelo, darin, dglazkov, fishd, jamesr, koivisto, rakuco, rniwa, tkent, tkent+wkapi, webkit.review.bot | ||||||||||||||||
Priority: | P2 | ||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||
Bug Depends on: | 84183 | ||||||||||||||||||
Bug Blocks: | 84413 | ||||||||||||||||||
Attachments: |
|
Description
Andreas Kling
2012-04-08 16:19:00 PDT
Created attachment 136160 [details]
POC/WIP for EWS
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 Created attachment 137152 [details]
WIP2
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI. 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.
Comment on attachment 137152 [details] WIP2 Attachment 137152 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12396831 Comment on attachment 137152 [details] WIP2 Attachment 137152 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12402173 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. Created attachment 137279 [details]
WIP3
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.
Comment on attachment 137279 [details] WIP3 Attachment 137279 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12407884 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 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
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. 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. How would changing to references help with that problem? (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. 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? Created attachment 138188 [details]
Patch (I want to believe.)
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.
r=me, looks great 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 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 Created attachment 138285 [details]
Same patch for landalizing.
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 Created attachment 138289 [details]
Patch for landing (less derp.)
Comment on attachment 138289 [details] Patch for landing (less derp.) Clearing flags on attachment: 138289 Committed r114870: <http://trac.webkit.org/changeset/114870> All reviewed patches have been landed. Closing bug. 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 *** Bug 75071 has been marked as a duplicate of this bug. *** |