Bug 129851 - Add fast path for id/name/style/class in getAttribute/setAttribute bindings.
Summary: Add fast path for id/name/style/class in getAttribute/setAttribute bindings.
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andreas Kling
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-03-06 17:28 PST by Andreas Kling
Modified: 2014-05-12 10:08 PDT (History)
9 users (show)

See Also:

Attachments
Patch (4.80 KB, patch)
2014-03-06 17:30 PST, Andreas Kling 		rniwa: review+
Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Andreas Kling 2014-03-06 17:28:21 PST 
Add fast path for id/name/style/class in getAttribute/setAttribute bindings.
Comment 1 Andreas Kling 2014-03-06 17:30:38 PST 
Created attachment 226068 [details]
Patch
Comment 2 Ryosuke Niwa 2014-03-06 17:44:42 PST 
Can we make sure we haven't regressed other attar accessed significantly with this?
Comment 3 Dongseong Hwang 2014-03-10 12:42:18 PDT 
Hi, I'm cherry-picking this to Blink, https://codereview.chromium.org/189483004/
In the bug, I measured performance. Both were run on my Ivy Bridge laptop. 

PerformanceTests/Bindings/get-attribute.html -> 16% improvement 
Before : Avg get-attribute: 273.565932runs/s 
After : Avg get-attribute: 317.818405runs/s 

PerformanceTests/Bindings/set-attribute.html -> 77% improvement 
Before : Avg set-attribute: 476.101349runs/s 
After : Avg set-attribute: 847.042662runs/s
Comment 4 Dongseong Hwang 2014-03-10 12:43:15 PDT 
Comment on attachment 226068 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=226068&action=review

> Source/WebCore/dom/Element.cpp:954
> +void Element::bindingsSetAttribute(const String& localName, const String& newValue, ExceptionCode& ec)

Could I ask why you used String& instead of AtomicString&?
Comment 5 Dongseong Hwang 2014-03-12 13:49:32 PDT 
Blink gave up cherry-picking this patch because it causes regression of other attributes significantly (about 50%).

getAttribute("id") -> 11% improvement 
Before : Avg get-attribute: 329.751776runs/s 
After : Avg get-attribute: 367.252965runs/s 

getAttribute("group") -> 51% regression 
Before : Avg get-attribute-href: 329.339992runs/s 
After : Avg get-attribute-href: 159.540438runs/s 

setAttribute("id")  -> 19% improvement 
Before : Avg set-attribute: 809.784217runs/s 
After : Avg set-attribute: 965.404519runs/s 

setAttribute("group") -> 45% regression 
Avg set-attribute-href: 756.714409runs/s 
Avg set-attribute-href: 415.186785runs/s
Comment 6 Chris Dumez 2014-03-12 14:05:22 PDT 
(In reply to comment #5)
> Blink gave up cherry-picking this patch because it causes regression of other attributes significantly (about 50%).
> 
> getAttribute("id") -> 11% improvement 
> Before : Avg get-attribute: 329.751776runs/s 
> After : Avg get-attribute: 367.252965runs/s 
> 
> getAttribute("group") -> 51% regression 
> Before : Avg get-attribute-href: 329.339992runs/s 
> After : Avg get-attribute-href: 159.540438runs/s 
> 
> setAttribute("id")  -> 19% improvement 
> Before : Avg set-attribute: 809.784217runs/s 
> After : Avg set-attribute: 965.404519runs/s 
> 
> setAttribute("group") -> 45% regression 
> Avg set-attribute-href: 756.714409runs/s 
> Avg set-attribute-href: 415.186785runs/s

Dongseong, are those the results with Blink or the results with WebKit? 
Results may be different.
Comment 7 Ryosuke Niwa 2014-03-12 14:11:36 PDT 
Note that we didn't land this patch.
Comment 8 Andreas Kling 2014-05-12 10:08:07 PDT 
Oh this. We don't need this now that we construct perfect strings in JSC.