Bug 23084 – Avoid redundant AtomicString conversions

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[David Smith]

Followup to #22699 for this review comment:

> +    pair<NodeListsNodeData::TagCacheMap::iterator, bool> result = data->nodeLists()->m_tagNodeListCaches.add(QualifiedName(nullAtom, name, namespaceURI), 0);
> +    if (result.second)
> +        result.first->second = new DynamicNodeList::Caches;
> +    
> +    return TagNodeList::create(this, namespaceURI.isEmpty() ? nullAtom : AtomicString(namespaceURI), name, result.first->second);

It's inefficient to convert namespaceURI to an AtomicString twice here. It
should be made into an AtomicString only once and reused. The best way to do
this is to change the argument into an AtomicString in the first place in
Node.h. This should be done for isDefaultNamespace, lookupPrefix,
lookupNamespacePrefix, and getElementsByTagNameNS, since all those functions
just end up making an AtomicString a moment later. Maybe in a separate patch,
though.

[David Smith]

Created attachment 26427 [details]
Does what it says on the tin

Much to my surprise this actually made a noticeable difference; My getElementsByTagName microbenchmark sped up by 20% or so, although the variance was so high I'm not sure how much that means.

[David Smith]

Landed in r39596

[David Smith]

Found a few more to remove

[David Smith]

Created attachment 26452 [details]
Cuts the time spent in AtomicString::add() by slightly more than half.

[Darin Adler]

One other thought. Converting to AtomicString early helps another way. If the argument for the DOM function itself is an AtomicString then the JavaScript UString can be converted to an AtomicString without first making a String, and if that value happens to already be in the AtomicString map, then this means there's no String allocation needed.