Bug 23084 - Avoid redundant AtomicString conversions
Summary: Avoid redundant AtomicString conversions
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.5
: P2 Normal
Assignee: David Smith
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-01-02 16:38 PST by David Smith
Modified: 2009-01-06 06:35 PST (History)
0 users

See Also:


Attachments
Does what it says on the tin (4.40 KB, patch)
2009-01-05 00:30 PST, David Smith
oliver: review+
Details | Formatted Diff | Diff
Cuts the time spent in AtomicString::add() by slightly more than half. (3.04 KB, patch)
2009-01-05 23:07 PST, David Smith
oliver: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Smith 2009-01-02 16:38:16 PST
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.
Comment 1 David Smith 2009-01-05 00:30:32 PST
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.
Comment 2 David Smith 2009-01-05 00:41:01 PST
Landed in r39596
Comment 3 David Smith 2009-01-05 23:06:52 PST
Found a few more to remove
Comment 4 David Smith 2009-01-05 23:07:37 PST
Created attachment 26452 [details]
Cuts the time spent in AtomicString::add() by slightly more than half.
Comment 5 Darin Adler 2009-01-06 06:35:59 PST
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.