Bug 90276 - Optimize Element::getAttributeNS() by replacing String with AtomicString
Summary: Optimize Element::getAttributeNS() by replacing String with AtomicString
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kentaro Hara
URL:
Keywords:
Depends on:
Blocks: 90246
  Show dependency treegraph
 
Reported: 2012-06-29 05:37 PDT by Kentaro Hara
Modified: 2012-07-27 18:21 PDT (History)
4 users (show)

See Also:


Attachments
Performance test (1.35 KB, text/html)
2012-06-29 05:38 PDT, Kentaro Hara
no flags Details
Updated performance test (1.36 KB, text/html)
2012-06-29 05:43 PDT, Kentaro Hara
no flags Details
Patch (4.11 KB, patch)
2012-06-29 05:44 PDT, Kentaro Hara
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kentaro Hara 2012-06-29 05:37:11 PDT
In r121439, we changed the argument type of Element::getAttribute() from String to AtomicString for optimization. We can also change the argument type of Element::getAttributeNS() from String to AtomicString.

Actually this change affects V8 performance only.

[JSC without the patch]
The binding code converts a given JavaScript string to a String -> the String is passed to getAttributeNS() -> the String is converted to an AtomicString when it is passed to QualifiedName() constructor.

[JSC with the patch]
The binding code converts a given JavaScript string to a String -> the String is converted to an AtomicString when it is passed to getAttributeNS() -> the AtomicString is passed to QualifiedName() constructor.

[V8 without the patch]
The binding code retrieves a cached external String for a given JavaScript string -> the String is passed to getAttributeNS() -> the String is converted to AtomicString when it is passed to QualifiedName() constructor.

[V8 with the patch]
The binding code retrieves a cached external AtomicString for a given JavaScript string -> the AtomicString is passed to getAttributeNS() -> the AtomicString is passed to QualifiedName() constructor.

In this way, in JSC, the change affects where a String is converted to an AtomicString. On the other hand, in V8, (since the AtomicString is already cached,) we can skip the conversion step.
Comment 1 Kentaro Hara 2012-06-29 05:38:20 PDT
Created attachment 150156 [details]
Performance test
Comment 2 Kentaro Hara 2012-06-29 05:43:32 PDT
Created attachment 150158 [details]
Updated performance test
Comment 3 Kentaro Hara 2012-06-29 05:44:36 PDT
Created attachment 150159 [details]
Patch
Comment 4 Adam Barth 2012-07-27 01:05:18 PDT
Comment on attachment 150159 [details]
Patch

Based on further discussion in #webkit, my current understanding is that ap is ok with these patches.  I'm going to r+ them, but please give ap a chance to respond before landing.
Comment 5 WebKit Review Bot 2012-07-27 18:21:31 PDT
Comment on attachment 150159 [details]
Patch

Clearing flags on attachment: 150159

Committed r123944: <http://trac.webkit.org/changeset/123944>
Comment 6 WebKit Review Bot 2012-07-27 18:21:35 PDT
All reviewed patches have been landed.  Closing bug.