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+

Andreas Kling
Reported 2014-03-06 17:28:21 PST
Add fast path for id/name/style/class in getAttribute/setAttribute bindings.
Attachments
Patch (4.80 KB, patch)
2014-03-06 17:30 PST, Andreas Kling
rniwa: review+
Andreas Kling
Comment 1 2014-03-06 17:30:38 PST
Ryosuke Niwa
Comment 2 2014-03-06 17:44:42 PST
Can we make sure we haven't regressed other attar accessed significantly with this?
Dongseong Hwang
Comment 3 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
Dongseong Hwang
Comment 4 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&?
Dongseong Hwang
Comment 5 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
Chris Dumez
Comment 6 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.
Ryosuke Niwa
Comment 7 2014-03-12 14:11:36 PDT
Note that we didn't land this patch.
Andreas Kling
Comment 8 2014-05-12 10:08:07 PDT
Oh this. We don't need this now that we construct perfect strings in JSC.
Note You need to log in before you can comment on or make changes to this bug.