Bug 128058 – Speed up DatasetDOMStringMap::item() when the element has multiple attributes

• Component: New Bugs
• Status: RESOLVED FIXED

[Benjamin Poulain]

Speed up DatasetDOMStringMap::item() when the element has multiple attributes

[Benjamin Poulain]

Created attachment 222900 [details]
Patch

[Darin Adler]

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?

[Benjamin Poulain]

(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.

[Benjamin Poulain]

Committed r163847: <http://trac.webkit.org/changeset/163847>