Speed up DatasetDOMStringMap::item() when the element has multiple attributes
Created attachment 222900 [details] Patch
Comment on attachment 222900 [details] 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?
(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.
Committed r163847: <http://trac.webkit.org/changeset/163847>