Bug 129851

Summary: Add fast path for id/name/style/class in getAttribute/setAttribute bindings.
Product: WebKit Reporter: Andreas Kling <kling>
Component: WebCore JavaScriptAssignee: Andreas Kling <kling>
Status: RESOLVED INVALID    
Severity: Normal CC: cdumez, cmarcelo, commit-queue, dongseong.hwang, esprehn+autocc, kangil.han, kling, kondapallykalyan, rniwa
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch rniwa: review+

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.