128058 2014-02-01 19:35:28 -0800 Speed up DatasetDOMStringMap::item() when the element has multiple attributes 2014-02-10 18:07:04 -0800 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 benjamin benjamin commit-queue darin esprehn+autocc kangil.han oldest_to_newest 974692 0 benjamin 2014-02-01 19:35:28 -0800 Speed up DatasetDOMStringMap::item() when the element has multiple attributes 974693 1 222900 benjamin 2014-02-01 19:45:32 -0800 Created attachment 222900 Patch 974806 2 222900 darin 2014-02-02 12:21:21 -0800 Comment on attachment 222900 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=222900&action=review > Source/WebCore/dom/DatasetDOMStringMap.cpp:111 > +static inline AtomicString convertPropertyNameToAttributeName(const StringImpl* name) Should take StringImpl& rather than StringImpl* since you already check for null in the caller. No need for const since a StringImpl is an immutable class. > Source/WebCore/dom/DatasetDOMStringMap.cpp:113 > + ASSERT(name); This assertion is usually the clue that you should be passing a reference rather than a pointer. > Source/WebCore/dom/DatasetDOMStringMap.cpp:122 > + for (unsigned i = 0, length = name->length(); i < length; ++i) { Since name->length() is also used a couple lines earlier I suggest declaring this local variable outside the loop, and using it up there. > Source/WebCore/dom/DatasetDOMStringMap.cpp:123 > + CharacterType character = name->getCharacters<CharacterType>()[i]; I think we should call name->getCharacters outside the loop and put the result into a local variable. > Source/WebCore/dom/DatasetDOMStringMap.cpp:138 > + StringImpl* nameImpl = name.impl(); This should be a StringImpl&. > Source/WebCore/dom/DatasetDOMStringMap.cpp:180 > + AtomicString attributeName = convertPropertyNameToAttributeName(propertyName); I’m not sure that using the AtomicString here really pays off. What happens if you use a String for the result of convertPropertyNameToAttributeName and do a string equality check each time instead of an AtomicString pointer check? 974817 3 benjamin 2014-02-02 12:56:22 -0800 (In reply to comment #2) > > Source/WebCore/dom/DatasetDOMStringMap.cpp:180 > > + AtomicString attributeName = convertPropertyNameToAttributeName(propertyName); > > I’m not sure that using the AtomicString here really pays off. What happens if you use a String for the result of convertPropertyNameToAttributeName and do a string equality check each time instead of an AtomicString pointer check? The AtomicString here is an important part of this patch. Since attributes are already AtomicString, we do not actually allocate anything in the common case, we get the original StringImpl from the Atomic String Table. Add to this the fast comparison and that makes the bulk of the speed up. I just tried a quick ad-hoc testing replacing the AtomicString with String: -enumeration: 165ms -> 201ms -access: 137ms -> 151ms. (still a lot faster than without the change though :) ). Also note that convertPropertyNameToAttributeName() is used by 2 other functions expecting an AtomicString. 979175 4 benjamin 2014-02-10 18:07:04 -0800 Committed r163847: <http://trac.webkit.org/changeset/163847> 222900 2014-02-01 19:45:32 -0800 2014-02-02 12:21:21 -0800 Patch bug-128058-20140201194531.patch text/plain 5699 benjamin SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDE2MzI1NikKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDMyIEBACisyMDE0LTAyLTAxICBCZW5qYW1p biBQb3VsYWluICA8YmVuamFtaW5Ad2Via2l0Lm9yZz4KKworICAgICAgICBTcGVlZCB1cCBEYXRh c2V0RE9NU3RyaW5nTWFwOjppdGVtKCkgd2hlbiB0aGUgZWxlbWVudCBoYXMgbXVsdGlwbGUgYXR0 cmlidXRlcworICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9 MTI4MDU4CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAg QWNjZXNzaW5nIGRhdGEgYXR0cmlidXRlcyBieSBuYW1lIHRocm91Z2ggRGF0YXNldERPTVN0cmlu Z01hcCBpbnZvbGVzIHRoZSBjb252ZXJzaW9uCisgICAgICAgIGZyb20gSmF2YVNjcmlwdCBwcm9w ZXJ0eSBuYW1lIHRvIGF0dHJpYnV0ZSBuYW1lIChkb25lIHdpdGggcHJvcGVydHlOYW1lTWF0Y2hl c0F0dHJpYnV0ZU5hbWUoKSkuCisKKyAgICAgICAgV2hlbiB0aGVyZSBpcyBhIHNpbmdsZSBkYXRh IGF0dHJpYnV0ZSwgdGhhdCBtZXRob2QgaXMgZWZmaWNpZW50LiBXaGVuIHRoZXJlIGFyZSBzZXZl cmFsIGF0dHJpYnV0ZXMsCisgICAgICAgIGNvbXBhcmluZyBuYW1lcyBjaGFyYWN0ZXIgYnkgY2hh cmFjdGVyIGJlY29tZXMgYSBib3R0bGVuZWNrLgorCisgICAgICAgIFRoaXMgcGF0Y2ggYWRkIGFu IGVmZmljZW50IHBhdGggZm9yIHRoaXMgY2FzZTogaW5zdGVhZCBvZiBjb252ZXJ0aW5nIHRoZSBh dHRyaWJ1dGUgbmFtZSBvbiB0aGUgZmx5LAorICAgICAgICB0aGUgSmF2YVNjcmlwdCBwcm9wZXJ0 eSBuYW1lIGlzIGNvbnZlcnRlZCB0byBhbiBhdHRyaWJ1dGUgbmFtZSBzbyB0aGF0IGl0IGNhbiBj b21wYXJlZCBieSBpdHMKKyAgICAgICAgQXRvbWljU3RyaW5nSW1wbCBwb2ludGVyLgorCisgICAg ICAgIFRoaXMgbWV0aG9kIHB1dHMgYSBsb3QgbW9yZSBwcmVzc3VyZSBvbiBjb252ZXJ0UHJvcGVy dHlOYW1lVG9BdHRyaWJ1dGVOYW1lKCkncyBzcGVlZC4gVGhlIG1ldGhvZCB3YXMKKyAgICAgICAg aW1wcm92ZWQgYWNjb3JkaW5nbHkgdG8gY29tcGVuc2F0ZSBmb3IgaXRzIG5ldyBjYWxsZXIuCisK KyAgICAgICAgV2hlbiBlbnVtZXJhdGluZyBtdWx0aXBsZSBhdHRyaWJ1dGVzIGJ5IG5hbWUsIHRo aXMgcGF0Y2ggcHJvdmlkZXMgYWJvdXQgODAlIHNwZWVkdXAuCisgICAgICAgIEkgY291bGQgbm90 IG1lYXN1cmUgYW55IHNsb3cgZG93biBvbiB0aGUgc2ltcGxlIGNhc2VzLgorCisgICAgICAgICog ZG9tL0RhdGFzZXRET01TdHJpbmdNYXAuY3BwOgorICAgICAgICAoV2ViQ29yZTo6Y29udmVydFBy b3BlcnR5TmFtZVRvQXR0cmlidXRlTmFtZSk6CisgICAgICAgIChXZWJDb3JlOjpEYXRhc2V0RE9N U3RyaW5nTWFwOjppdGVtKToKKyAgICAgICAgKiBkb20vRWxlbWVudERhdGEuaDoKKyAgICAgICAg KFdlYkNvcmU6OkF0dHJpYnV0ZUl0ZXJhdG9yQWNjZXNzb3I6OmF0dHJpYnV0ZUNvdW50KToKKwog MjAxNC0wMi0wMSAgRW5yaWNhIENhc3VjY2kgIDxlbnJpY2FAYXBwbGUuY29tPgogCiAgICAgICAg IEFkZCBzdXBwb3J0IGZvciBBY3Rpb25TaGVldHMgaW4gV0syIGZvciBpT1MuCkluZGV4OiBTb3Vy Y2UvV2ViQ29yZS9kb20vRGF0YXNldERPTVN0cmluZ01hcC5jcHAKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL1dlYkNvcmUvZG9tL0RhdGFzZXRET01TdHJpbmdNYXAuY3BwCShyZXZpc2lvbiAxNjMyNTEp CisrKyBTb3VyY2UvV2ViQ29yZS9kb20vRGF0YXNldERPTVN0cmluZ01hcC5jcHAJKHdvcmtpbmcg Y29weSkKQEAgLTEwNywyMiArMTA3LDM4IEBAIHN0YXRpYyBib29sIGlzVmFsaWRQcm9wZXJ0eU5h bWUoY29uc3QgU3QKICAgICByZXR1cm4gdHJ1ZTsKIH0KIAotc3RhdGljIFN0cmluZyBjb252ZXJ0 UHJvcGVydHlOYW1lVG9BdHRyaWJ1dGVOYW1lKGNvbnN0IFN0cmluZyYgbmFtZSkKK3RlbXBsYXRl PHR5cGVuYW1lIENoYXJhY3RlclR5cGU+CitzdGF0aWMgaW5saW5lIEF0b21pY1N0cmluZyBjb252 ZXJ0UHJvcGVydHlOYW1lVG9BdHRyaWJ1dGVOYW1lKGNvbnN0IFN0cmluZ0ltcGwqIG5hbWUpCiB7 Ci0gICAgU3RyaW5nQnVpbGRlciBidWlsZGVyOwotICAgIGJ1aWxkZXIuYXBwZW5kKCJkYXRhLSIp OworICAgIEFTU0VSVChuYW1lKTsKIAotICAgIHVuc2lnbmVkIGxlbmd0aCA9IG5hbWUubGVuZ3Ro KCk7Ci0gICAgZm9yICh1bnNpZ25lZCBpID0gMDsgaSA8IGxlbmd0aDsgKytpKSB7Ci0gICAgICAg IFVDaGFyIGNoYXJhY3RlciA9IG5hbWVbaV07CisgICAgY29uc3QgQ2hhcmFjdGVyVHlwZSBkYXRh UHJlZml4W10gPSB7ICdkJywgJ2EnLCAndCcsICdhJywgJy0nIH07CisKKyAgICBWZWN0b3I8Q2hh cmFjdGVyVHlwZSwgMzI+IGJ1ZmZlcjsKKyAgICBidWZmZXIucmVzZXJ2ZUluaXRpYWxDYXBhY2l0 eShXVEZfQVJSQVlfTEVOR1RIKGRhdGFQcmVmaXgpICsgbmFtZS0+bGVuZ3RoKCkpOworCisgICAg YnVmZmVyLmFwcGVuZChkYXRhUHJlZml4LCBXVEZfQVJSQVlfTEVOR1RIKGRhdGFQcmVmaXgpKTsK KworICAgIGZvciAodW5zaWduZWQgaSA9IDAsIGxlbmd0aCA9IG5hbWUtPmxlbmd0aCgpOyBpIDwg bGVuZ3RoOyArK2kpIHsKKyAgICAgICAgQ2hhcmFjdGVyVHlwZSBjaGFyYWN0ZXIgPSBuYW1lLT5n ZXRDaGFyYWN0ZXJzPENoYXJhY3RlclR5cGU+KClbaV07CiAgICAgICAgIGlmIChpc0FTQ0lJVXBw ZXIoY2hhcmFjdGVyKSkgewotICAgICAgICAgICAgYnVpbGRlci5hcHBlbmQoJy0nKTsKLSAgICAg ICAgICAgIGJ1aWxkZXIuYXBwZW5kKHRvQVNDSUlMb3dlcihjaGFyYWN0ZXIpKTsKKyAgICAgICAg ICAgIGJ1ZmZlci5hcHBlbmQoJy0nKTsKKyAgICAgICAgICAgIGJ1ZmZlci5hcHBlbmQodG9BU0NJ SUxvd2VyKGNoYXJhY3RlcikpOwogICAgICAgICB9IGVsc2UKLSAgICAgICAgICAgIGJ1aWxkZXIu YXBwZW5kKGNoYXJhY3Rlcik7CisgICAgICAgICAgICBidWZmZXIuYXBwZW5kKGNoYXJhY3Rlcik7 CiAgICAgfQorICAgIHJldHVybiBBdG9taWNTdHJpbmcoYnVmZmVyLmRhdGEoKSwgYnVmZmVyLnNp emUoKSk7Cit9CiAKLSAgICByZXR1cm4gYnVpbGRlci50b1N0cmluZygpOworc3RhdGljIEF0b21p Y1N0cmluZyBjb252ZXJ0UHJvcGVydHlOYW1lVG9BdHRyaWJ1dGVOYW1lKGNvbnN0IFN0cmluZyYg bmFtZSkKK3sKKyAgICBpZiAobmFtZS5pc051bGwoKSkKKyAgICAgICAgcmV0dXJuIG51bGxBdG9t OworCisgICAgU3RyaW5nSW1wbCogbmFtZUltcGwgPSBuYW1lLmltcGwoKTsKKyAgICBpZiAobmFt ZUltcGwtPmlzOEJpdCgpKQorICAgICAgICByZXR1cm4gY29udmVydFByb3BlcnR5TmFtZVRvQXR0 cmlidXRlTmFtZTxMQ2hhcj4obmFtZUltcGwpOworICAgIHJldHVybiBjb252ZXJ0UHJvcGVydHlO YW1lVG9BdHRyaWJ1dGVOYW1lPFVDaGFyPihuYW1lSW1wbCk7CiB9CiAKIHZvaWQgRGF0YXNldERP TVN0cmluZ01hcDo6cmVmKCkKQEAgLTE0NiwxNSArMTYyLDI4IEBAIHZvaWQgRGF0YXNldERPTVN0 cmluZ01hcDo6Z2V0TmFtZXMoVmVjdG8KICAgICB9CiB9CiAKLWNvbnN0IEF0b21pY1N0cmluZyYg RGF0YXNldERPTVN0cmluZ01hcDo6aXRlbShjb25zdCBTdHJpbmcmIG5hbWUsIGJvb2wmIGlzVmFs aWQpCitjb25zdCBBdG9taWNTdHJpbmcmIERhdGFzZXRET01TdHJpbmdNYXA6Oml0ZW0oY29uc3Qg U3RyaW5nJiBwcm9wZXJ0eU5hbWUsIGJvb2wmIGlzVmFsaWQpCiB7CiAgICAgaXNWYWxpZCA9IGZh bHNlOwogICAgIGlmIChtX2VsZW1lbnQuaGFzQXR0cmlidXRlcygpKSB7Ci0gICAgICAgIGZvciAo Y29uc3QgQXR0cmlidXRlJiBhdHRyaWJ1dGUgOiBtX2VsZW1lbnQuYXR0cmlidXRlc0l0ZXJhdG9y KCkpIHsKLSAgICAgICAgICAgIGlmIChwcm9wZXJ0eU5hbWVNYXRjaGVzQXR0cmlidXRlTmFtZShu YW1lLCBhdHRyaWJ1dGUubG9jYWxOYW1lKCkpKSB7CisgICAgICAgIEF0dHJpYnV0ZUl0ZXJhdG9y QWNjZXNzb3IgYXR0cmlidXRlSXRlcmF0b3JBY2Nlc3NvciA9IG1fZWxlbWVudC5hdHRyaWJ1dGVz SXRlcmF0b3IoKTsKKworICAgICAgICBpZiAoYXR0cmlidXRlSXRlcmF0b3JBY2Nlc3Nvci5hdHRy aWJ1dGVDb3VudCgpID09IDEpIHsKKyAgICAgICAgICAgIC8vIElmIHRoZSBub2RlIGhhcyBhIHNp bmdsZSBhdHRyaWJ1dGUsIGl0IGlzIHRoZSBkYXRhc2V0IG1lbWJlciBhY2Nlc3NlZCBpbiBtb3N0 IGNhc2VzLgorICAgICAgICAgICAgLy8gQnVpbGRpbmcgYSBuZXcgQXRvbWljU3RyaW5nIGluIHRo YXQgY2FzZSBpcyBvdmVya2lsbCBzbyB3ZSBkbyBhIGRpcmVjdCBjaGFyYWN0ZXIgY29tcGFyaXNv bi4KKyAgICAgICAgICAgIGNvbnN0IEF0dHJpYnV0ZSYgYXR0cmlidXRlID0gKmF0dHJpYnV0ZUl0 ZXJhdG9yQWNjZXNzb3IuYmVnaW4oKTsKKyAgICAgICAgICAgIGlmIChwcm9wZXJ0eU5hbWVNYXRj aGVzQXR0cmlidXRlTmFtZShwcm9wZXJ0eU5hbWUsIGF0dHJpYnV0ZS5sb2NhbE5hbWUoKSkpIHsK ICAgICAgICAgICAgICAgICBpc1ZhbGlkID0gdHJ1ZTsKICAgICAgICAgICAgICAgICByZXR1cm4g YXR0cmlidXRlLnZhbHVlKCk7CiAgICAgICAgICAgICB9CisgICAgICAgIH0gZWxzZSB7CisgICAg ICAgICAgICBBdG9taWNTdHJpbmcgYXR0cmlidXRlTmFtZSA9IGNvbnZlcnRQcm9wZXJ0eU5hbWVU b0F0dHJpYnV0ZU5hbWUocHJvcGVydHlOYW1lKTsKKyAgICAgICAgICAgIGZvciAoY29uc3QgQXR0 cmlidXRlJiBhdHRyaWJ1dGUgOiBhdHRyaWJ1dGVJdGVyYXRvckFjY2Vzc29yKSB7CisgICAgICAg ICAgICAgICAgaWYgKGF0dHJpYnV0ZS5sb2NhbE5hbWUoKSA9PSBhdHRyaWJ1dGVOYW1lKSB7Cisg ICAgICAgICAgICAgICAgICAgIGlzVmFsaWQgPSB0cnVlOworICAgICAgICAgICAgICAgICAgICBy ZXR1cm4gYXR0cmlidXRlLnZhbHVlKCk7CisgICAgICAgICAgICAgICAgfQorICAgICAgICAgICAg fQogICAgICAgICB9CiAgICAgfQogCkluZGV4OiBTb3VyY2UvV2ViQ29yZS9kb20vRWxlbWVudERh dGEuaAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2ViQ29yZS9kb20vRWxlbWVudERhdGEuaAkocmV2 aXNpb24gMTYzMjUwKQorKysgU291cmNlL1dlYkNvcmUvZG9tL0VsZW1lbnREYXRhLmgJKHdvcmtp bmcgY29weSkKQEAgLTY3LDYgKzY3LDkgQEAgcHVibGljOgogCiAgICAgQXR0cmlidXRlQ29uc3RJ dGVyYXRvciBiZWdpbigpIGNvbnN0IHsgcmV0dXJuIEF0dHJpYnV0ZUNvbnN0SXRlcmF0b3IobV9h cnJheSwgMCk7IH0KICAgICBBdHRyaWJ1dGVDb25zdEl0ZXJhdG9yIGVuZCgpIGNvbnN0IHsgcmV0 dXJuIEF0dHJpYnV0ZUNvbnN0SXRlcmF0b3IobV9hcnJheSwgbV9zaXplKTsgfQorCisgICAgdW5z aWduZWQgYXR0cmlidXRlQ291bnQoKSBjb25zdCB7IHJldHVybiBtX3NpemU7IH0KKwogcHJpdmF0 ZToKICAgICBjb25zdCBBdHRyaWJ1dGUqIG1fYXJyYXk7CiAgICAgdW5zaWduZWQgbV9zaXplOwo=