Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
189483004:
WontFix: Add fast path for id/name/style/class in getAttribute/setAttribute bindings. (Closed)
|
Created:
6 years, 9 months ago by dshwang Modified:
6 years, 9 months ago CC:
blink-reviews, arv+blink, sof, eae+blinkwatch, dglazkov+blink, adamk+blink_chromium.org, watchdog-blink-watchlist_google.com, rwlbuis Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionWontFix: Add fast path for id/name/class/style in getAttribute/setAttribute bindings.
Cherry-picked the patch by akling@apple.com from <https://webkit.org/b/129851>;
Since these attributes are so commonly read and modified via the DOM bindings,
add a little fast path to avoid doing AtomicString lookups for those names when
they are used in friendly lowercase.
fastGetAttribute() in WebKit can handles style attribute, but fastGetAttribute()
in Blink does not support that, so there is relevant changes.
Test: PerformanceTests/Bindings/get-attribute.html,
PerformanceTests/Bindings/get-attribute-rare.html,
PerformanceTests/Bindings/set-attribute.html,
PerformanceTests/Bindings/set-attribute-rare.html
Both were run on my Ivy Bridge laptop.
PerformanceTests/Bindings/get-attribute.html -> 11% improvement
Before : Avg get-attribute: 329.751776runs/s
After : Avg get-attribute: 367.252965runs/s
PerformanceTests/Bindings/get-attribute-rare.html -> 51% regression
Before : Avg get-attribute-href: 329.339992runs/s
After : Avg get-attribute-href: 159.540438runs/s
PerformanceTests/Bindings/set-attribute.html -> 19% improvement
Before : Avg set-attribute: 809.784217runs/s
After : Avg set-attribute: 965.404519runs/s
PerformanceTests/Bindings/set-attribute-rare.html -> 45% regression
Avg set-attribute-href: 756.714409runs/s
Avg set-attribute-href: 415.186785runs/s
BUG=350380
Patch Set 1 #
Total comments: 1
Patch Set 2 : Exclude style tag in fast path because blink don't support #
Total comments: 10
Patch Set 3 : deal with svg #
Total comments: 5
Patch Set 4 : Use String for localName, and remove duplicated code #
Total comments: 3
Patch Set 5 : use enum instead of bool #Patch Set 6 : Add performance tests about rare attribute. #
Messages
Total messages: 30 (0 generated)
Would be good to make sure we don't regress other attributes. Is there an noticeable improvement on any benchmark / perf test? https://codereview.chromium.org/189483004/diff/1/Source/core/dom/Element.cpp File Source/core/dom/Element.cpp (right): https://codereview.chromium.org/189483004/diff/1/Source/core/dom/Element.cpp#... Source/core/dom/Element.cpp:938: void Element::bindingsSetAttribute(const String& localName, const String& newValue, ExceptionState& exceptionState) Looks like newValue should be an AtomicString.
Could you provide the numbers for the following performance test for e.g.? PerformanceTests/Bindings/get-attribute.html
On 2014/03/07 13:30:21, Chris Dumez wrote: > Could you provide the numbers for the following performance test for e.g.? > PerformanceTests/Bindings/get-attribute.html That's good questions! I measures perf improvement in ivy bridge laptop. The results is quite nice. PerformanceTests/Bindings/get-attribute.html -> 16% improvement *RESULT get-attribute: get-attribute= [282.645199,281.853984,279.357275,249.997187,273.976015] runs/s Avg get-attribute: 273.565932runs/s Sd get-attribute: 13.604397runs/s *RESULT get-attribute: get-attribute= [312.613567,293.105493,319.894648,325.643553,337.834765] runs/s Avg get-attribute: 317.818405runs/s Sd get-attribute: 16.613374runs/s PerformanceTests/Bindings/set-attribute.html -> 77% improvement *RESULT set-attribute: set-attribute= [474.362674,470.221892,478.268032,474.399555,483.254592] runs/s Avg set-attribute: 476.101349runs/s Sd set-attribute: 4.907915runs/s *RESULT set-attribute: set-attribute= [829.885204,827.331945,869.225086,859.750768,849.020310] runs/s Avg set-attribute: 847.042662runs/s Sd set-attribute: 18.305448runs/s
Let me make more readable. 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
On 2014/03/07 13:44:41, dshwang wrote: > Let me make more readable. > > 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 Looks convincing to me. Please update as per review comment and include those numbers in your CL description.
On 2014/03/07 14:02:44, Chris Dumez wrote: > Looks convincing to me. Please update as per review comment and include those > numbers in your CL description. Yes, done.
On 2014/03/07 14:21:32, dshwang wrote: > On 2014/03/07 14:02:44, Chris Dumez wrote: > > Looks convincing to me. Please update as per review comment and include those > > numbers in your CL description. > > Yes, done. Could you check the failures on the try bots. Looks like they might be real.
On 2014/03/07 15:24:38, Chris Dumez wrote: > Could you check the failures on the try bots. Looks like they might be real. yes, I think so. I'll fix it soon.
I fix the layout test failure. https://codereview.chromium.org/189483004/diff/20001/Source/core/dom/Element.cpp File Source/core/dom/Element.cpp (right): https://codereview.chromium.org/189483004/diff/20001/Source/core/dom/Element.... Source/core/dom/Element.cpp:2970: return false; Blink does not support styleAttr fast path.
https://codereview.chromium.org/189483004/diff/20001/Source/core/dom/Element.cpp File Source/core/dom/Element.cpp (right): https://codereview.chromium.org/189483004/diff/20001/Source/core/dom/Element.... Source/core/dom/Element.cpp:936: void Element::bindingsSetAttribute(const String& localName, const String& newValue, ExceptionState& exceptionState) I think I made this comment before but newValue should really be an const AtomicString& here, not a const String& as we always use it as an AtomicString. It makes little sense for the bindings to construct a String if it will anyway be converted into an AtomicString internally.
On 2014/03/10 18:46:44, Chris Dumez wrote: > https://codereview.chromium.org/189483004/diff/20001/Source/core/dom/Element.cpp > File Source/core/dom/Element.cpp (right): > > https://codereview.chromium.org/189483004/diff/20001/Source/core/dom/Element.... > Source/core/dom/Element.cpp:936: void Element::bindingsSetAttribute(const > String& localName, const String& newValue, ExceptionState& exceptionState) > I think I made this comment before but newValue should really be an const > AtomicString& here, not a const String& as we always use it as an AtomicString. > It makes little sense for the bindings to construct a String if it will anyway > be converted into an AtomicString internally. I was also curious about that. but I just followed webkit commit: https://bugs.webkit.org/attachment.cgi?id=226068&action=review I'll try to use AtomicString here and tet me check performance difference.
https://codereview.chromium.org/189483004/diff/20001/Source/core/dom/Element.cpp File Source/core/dom/Element.cpp (right): https://codereview.chromium.org/189483004/diff/20001/Source/core/dom/Element.... Source/core/dom/Element.cpp:923: return HTMLNames::classAttr; Technically, I believe we cannot do a fast lookup for the "class" attribute for SVG elements either, because it is animated: Source/core/svg/SVGElement.idl: [MeasureAs=SVGClassName] readonly attribute SVGAnimatedString className; I believe you are likely breaking getAttribute("class") on SVG elements.
https://codereview.chromium.org/189483004/diff/20001/Source/core/dom/Element.cpp File Source/core/dom/Element.cpp (right): https://codereview.chromium.org/189483004/diff/20001/Source/core/dom/Element.... Source/core/dom/Element.cpp:2973: return !toSVGElement(this)->isAnimatableAttribute(name); Thanks Chris. here is svg consideration. How about this code? Can this address your concern? const QualifiedName& fastName = fastAttributeNameFromString(localName); if (fastName != nullQName() && fastAttributeLookupAllowed(fastName )) return fastGetAttribute(fastName)
See one proposal below. Feel free to propose something better. https://codereview.chromium.org/189483004/diff/20001/Source/core/dom/Element.cpp File Source/core/dom/Element.cpp (right): https://codereview.chromium.org/189483004/diff/20001/Source/core/dom/Element.... Source/core/dom/Element.cpp:911: static inline const QualifiedName& fastAttributeNameFromString(const String& localName) add "const Element& element" argument. https://codereview.chromium.org/189483004/diff/20001/Source/core/dom/Element.... Source/core/dom/Element.cpp:922: if (length == 5 && name[0] == 'c' && name[1] == 'l' && name[2] == 'a' && name[3] == 's' && name[4] == 's') add "!element.isSVGElement() && " condition. https://codereview.chromium.org/189483004/diff/20001/Source/core/dom/Element.... Source/core/dom/Element.cpp:930: const QualifiedName& fastName = fastAttributeNameFromString(localName); fastAttributeNameFromString(*this, localName); https://codereview.chromium.org/189483004/diff/20001/Source/core/dom/Element.... Source/core/dom/Element.cpp:938: const QualifiedName& fastName = fastAttributeNameFromString(localName); fastAttributeNameFromString(*this, localName); https://codereview.chromium.org/189483004/diff/20001/Source/core/dom/Element.... Source/core/dom/Element.cpp:2973: return !toSVGElement(this)->isAnimatableAttribute(name); On 2014/03/10 19:48:59, dshwang wrote: > Thanks Chris. here is svg consideration. > > How about this code? Can this address your concern? > const QualifiedName& fastName = fastAttributeNameFromString(localName); > if (fastName != nullQName() && fastAttributeLookupAllowed(fastName )) > return fastGetAttribute(fastName) No, fastAttributeLookupAllowed() is only available in debug builds and would be too expensible to call in your fast path.
https://codereview.chromium.org/189483004/diff/20001/Source/core/dom/Element.cpp File Source/core/dom/Element.cpp (right): https://codereview.chromium.org/189483004/diff/20001/Source/core/dom/Element.... Source/core/dom/Element.cpp:936: void Element::bindingsSetAttribute(const String& localName, const String& newValue, ExceptionState& exceptionState) On 2014/03/10 18:46:45, Chris Dumez wrote: > I think I made this comment before but newValue should really be an const > AtomicString& here, not a const String& as we always use it as an AtomicString. > It makes little sense for the bindings to construct a String if it will anyway > be converted into an AtomicString internally. To be clear, I only want newValue to be an AtomicString. Keeping localName as a String is fine. Unlike WebKit, our AtomicString(const String&) constructor is explicit so it is more obvious when we are converting a String to an AtomicString.
On 2014/03/10 20:04:14, Chris Dumez wrote: > https://codereview.chromium.org/189483004/diff/20001/Source/core/dom/Element.cpp > File Source/core/dom/Element.cpp (right): > > https://codereview.chromium.org/189483004/diff/20001/Source/core/dom/Element.... > Source/core/dom/Element.cpp:936: void Element::bindingsSetAttribute(const > String& localName, const String& newValue, ExceptionState& exceptionState) > On 2014/03/10 18:46:45, Chris Dumez wrote: > > I think I made this comment before but newValue should really be an const > > AtomicString& here, not a const String& as we always use it as an > AtomicString. > > It makes little sense for the bindings to construct a String if it will anyway > > be converted into an AtomicString internally. > > To be clear, I only want newValue to be an AtomicString. Keeping localName as a > String is fine. Unlike WebKit, our AtomicString(const String&) constructor is > explicit so it is more obvious when we are converting a String to an > AtomicString. FYI, our V8 bindings are smart enough to construct directly an AtomicString from the v8 String if the method takes an AtomicString in argument so we don't need to construct a String and then atomize it. It also simplifies your patch a bit because it avoid calling the AtomicString(const String&) constructor explicitly.
On 2014/03/10 20:10:01, Chris Dumez wrote: > On 2014/03/10 20:04:14, Chris Dumez wrote: > > > https://codereview.chromium.org/189483004/diff/20001/Source/core/dom/Element.cpp > > File Source/core/dom/Element.cpp (right): > > > > > https://codereview.chromium.org/189483004/diff/20001/Source/core/dom/Element.... > > Source/core/dom/Element.cpp:936: void Element::bindingsSetAttribute(const > > String& localName, const String& newValue, ExceptionState& exceptionState) > > On 2014/03/10 18:46:45, Chris Dumez wrote: > > > I think I made this comment before but newValue should really be an const > > > AtomicString& here, not a const String& as we always use it as an > > AtomicString. > > > It makes little sense for the bindings to construct a String if it will > anyway > > > be converted into an AtomicString internally. > > > > To be clear, I only want newValue to be an AtomicString. Keeping localName as > a > > String is fine. Unlike WebKit, our AtomicString(const String&) constructor is > > explicit so it is more obvious when we are converting a String to an > > AtomicString. > > FYI, our V8 bindings are smart enough to construct directly an AtomicString from > the v8 String if the method takes an AtomicString in argument so we don't need > to construct a String and then atomize it. It also simplifies your patch a bit > because it avoid calling the AtomicString(const String&) constructor explicitly. wow, thank you very much. Let me try.
I update this CL as Chris's suggestion. setAttribute accelerates id/name/class/style attributes getAttribute accelerates id/name/class attributes. there is additional svg condition for class attribute. Could you review again?
I measure performance test between using String and using AtomicString. The results for both are almost same, so I use AtomicString.
https://codereview.chromium.org/189483004/diff/40001/Source/core/dom/Element.cpp File Source/core/dom/Element.cpp (right): https://codereview.chromium.org/189483004/diff/40001/Source/core/dom/Element.... Source/core/dom/Element.cpp:918: if (length == 2 && name[0] == 'i' && name[1] == 'd') if localName if an AtomicString, checking its characters no longer makes sense as you could simply compare AtomicStrings (pointer comparison). https://codereview.chromium.org/189483004/diff/40001/Source/core/dom/Element.... Source/core/dom/Element.cpp:928: const AtomicString& Element::bindingsGetAttribute(const AtomicString& localName) const Based on the Changelog on WebKit side, part of the optimization was to avoid AtomicString lookup for the commonly used attribute names. However, now that you are using AtomicString type for localName, this optimization does not make sense anymore (you already did the AtomicString lookup). https://codereview.chromium.org/189483004/diff/40001/Source/core/dom/Element.... Source/core/dom/Element.cpp:936: static inline const QualifiedName& principalAttributeNameFromString(const AtomicString& localName) Sorry, I don't like the fact that we now have 2 functions doing almost the same thing and thus a bit of code duplication. I understand you are trying to handle the attribute setting case more efficiently since we don't have a fastSetAttribute() on Blink-side. However, we need to find a better way to achieve this. https://codereview.chromium.org/189483004/diff/40001/Source/core/dom/Element.... Source/core/dom/Element.cpp:955: void Element::bindingsSetAttribute(const AtomicString& localName, const AtomicString& newValue, ExceptionState& exceptionState) localName should be a String. I only asked for newValue to be an AtomicString and I explicitly said that localName should remain a String. https://codereview.chromium.org/189483004/diff/40001/Source/core/dom/Element.... Source/core/dom/Element.cpp:959: setAttribute(principalAttributeName, AtomicString(newValue)); Useless AtomicString(). Note that WebKit has a fastSetAttribute() as well. We may consider porting this patch as well if we see an impact on perf tests.
On 2014/03/11 13:48:32, Chris Dumez wrote: Thank you for great review. > https://codereview.chromium.org/189483004/diff/40001/Source/core/dom/Element.cpp > File Source/core/dom/Element.cpp (right): > > https://codereview.chromium.org/189483004/diff/40001/Source/core/dom/Element.... > Source/core/dom/Element.cpp:918: if (length == 2 && name[0] == 'i' && name[1] == > 'd') > if localName if an AtomicString, checking its characters no longer makes sense > as you could simply compare AtomicStrings (pointer comparison). > > https://codereview.chromium.org/189483004/diff/40001/Source/core/dom/Element.... > Source/core/dom/Element.cpp:928: const AtomicString& > Element::bindingsGetAttribute(const AtomicString& localName) const > Based on the Changelog on WebKit side, part of the optimization was to avoid > AtomicString lookup for the commonly used attribute names. However, now that you > are using AtomicString type for localName, this optimization does not make sense > anymore (you already did the AtomicString lookup). I use String for localName again. > https://codereview.chromium.org/189483004/diff/40001/Source/core/dom/Element.... > Source/core/dom/Element.cpp:936: static inline const QualifiedName& > principalAttributeNameFromString(const AtomicString& localName) > Sorry, I don't like the fact that we now have 2 functions doing almost the same > thing and thus a bit of code duplication. I understand you are trying to handle > the attribute setting case more efficiently since we don't have a > fastSetAttribute() on Blink-side. However, we need to find a better way to > achieve this. Yes, I remove duplicated code using template. I use template instead of parameter in order to save one cpu ragister for parameter and to reduce cpu pipleline stall due to branching fail. > https://codereview.chromium.org/189483004/diff/40001/Source/core/dom/Element.... > Source/core/dom/Element.cpp:959: setAttribute(principalAttributeName, > AtomicString(newValue)); > Useless AtomicString(). > > Note that WebKit has a fastSetAttribute() as well. We may consider porting this > patch as well if we see an impact on perf tests. Yes, that's good to know. let me see.
Much better but I still have a couple of comments. https://codereview.chromium.org/189483004/diff/60001/Source/core/dom/Element.cpp File Source/core/dom/Element.cpp (right): https://codereview.chromium.org/189483004/diff/60001/Source/core/dom/Element.... Source/core/dom/Element.cpp:911: template<bool isSet> static inline const QualifiedName& fastAttributeNameFromString(const Element& element, const String& localName) "isSet" is not very clear. Maybe "forSetter"? Also, we prefer using enumerations than boolean because it makes the callers more readable. https://codereview.chromium.org/189483004/diff/60001/Source/core/dom/Element.... Source/core/dom/Element.cpp:932: const QualifiedName& fastName = fastAttributeNameFromString<false>(*this, localName); For e.g. fastAttributeNameFromString<false>() is much less readable than fastAttributeNameFromString<ForGetter>() https://codereview.chromium.org/189483004/diff/60001/Source/core/dom/Element.... Source/core/dom/Element.cpp:940: const QualifiedName& principalAttributeName = fastAttributeNameFromString<true>(*this, localName); principalAttributeName is not a great name. Please use fastName as for the other method.
@dshwang, can you please make sure the results in your CL description are up-to-date? Results may have changed after the patches iterations. @eseidel, I think this is getting close to ready in my books (especially if the impact on the perf tests is still as good as stated in the CL's description). Are you OK with this as well?
On 2014/03/11 15:46:27, Chris Dumez wrote: > @dshwang, can you please make sure the results in your CL description are > up-to-date? Results may have changed after the patches iterations. Thank you for review. I addressed your review and update test results. Test results is changed FROM 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 TO PerformanceTests/Bindings/get-attribute.html -> 11% improvement Before : Avg get-attribute: 329.751776runs/s After : Avg get-attribute: 367.252965runs/s PerformanceTests/Bindings/set-attribute.html -> 19% improvement Before : Avg set-attribute: 809.784217runs/s After : Avg set-attribute: 965.404519runs/s I can not reproduce previous test results. It seems to depend on machine condition...
There is a typo in the description: "WebKie" -> "WebKit". The patch looks good now. One thing that hasn't been verified yet is how it impacts other attributes (e.g. "href"). I'd like us to make sure it does not regress these significantly. Would you mind adding new get-attribute-href.html / set-attribute-href.html perf tests (adapted from the get-attribute.html / set-attribute-href.html ones) and check the impact of your patch on them? (you can then include the new tests in your CL for better coverage in the future).
On 2014/03/11 17:19:04, Chris Dumez wrote: > There is a typo in the description: "WebKie" -> "WebKit". > > The patch looks good now. > > One thing that hasn't been verified yet is how it impacts other attributes (e.g. > "href"). I'd like us to make sure it does not regress these significantly. > Would you mind adding new get-attribute-href.html / set-attribute-href.html perf > tests (adapted from the get-attribute.html / set-attribute-href.html ones) and > check the impact of your patch on them? (you can then include the new tests in > your CL for better coverage in the future). That's good suggestion! I did it and the results show regression. I think this CL should close as WontFix. wdyt?
On 2014/03/11 18:16:55, dshwang wrote: > On 2014/03/11 17:19:04, Chris Dumez wrote: > > There is a typo in the description: "WebKie" -> "WebKit". > > > > The patch looks good now. > > > > One thing that hasn't been verified yet is how it impacts other attributes > (e.g. > > "href"). I'd like us to make sure it does not regress these significantly. > > Would you mind adding new get-attribute-href.html / set-attribute-href.html > perf > > tests (adapted from the get-attribute.html / set-attribute-href.html ones) and > > check the impact of your patch on them? (you can then include the new tests in > > your CL for better coverage in the future). > > That's good suggestion! I did it and the results show regression. > I think this CL should close as WontFix. wdyt? Ouch... 45-50% regression on slow-path attributes is bad indeed (much bigger than the improvement on the fast path). It is a good thing you checked, thanks for taking the time to verify! It is unfortunate since you spent time on this but I also agree that this optimization does not seem to be worth it.
On 2014/03/11 18:23:50, Chris Dumez wrote: > Ouch... 45-50% regression on slow-path attributes is bad indeed (much bigger > than the improvement on the fast path). It is a good thing you checked, thanks > for taking the time to verify! It is unfortunate since you spent time on this > but I also agree that this optimization does not seem to be worth it. Yes, sorry for taking your amount of time for invalid optimization. I need to notify WebKit dev also.
Message was sent while issue was closed.
On 2014/03/11 18:34:22, dshwang wrote: > On 2014/03/11 18:23:50, Chris Dumez wrote: > > Ouch... 45-50% regression on slow-path attributes is bad indeed (much bigger > > than the improvement on the fast path). It is a good thing you checked, thanks > > for taking the time to verify! It is unfortunate since you spent time on this > > but I also agree that this optimization does not seem to be worth it. > > Yes, sorry for taking your amount of time for invalid optimization. I need to > notify WebKit dev also. I'm always happy to discuss dom optimizations. You can try your new perf tests on WebKit side, it may yield different results there. The code bases and also the patches are slightly different. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
