Bug 90246 - Change argument types of Element::getAttribute*() from String to AtomicString
Summary: Change argument types of Element::getAttribute*() from String to AtomicString
Status: REOPENED
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: 90265 90273 90274 90276
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-28 21:21 PDT by Kentaro Hara
Modified: 2012-07-27 03:43 PDT (History)
8 users (show)

See Also:


Attachments
Patch (9.01 KB, patch)
2012-06-28 21:23 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-28 21:21:53 PDT
This is a follow-up patch for r121439.

r121439 changed an argument type of Element::getAttribute() from String to AtomicString, which optimized performance of Dromaeo/dom-attr.html. We can also change other argument types of Element::getAttribute*() from String to AtomicString. See http://trac.webkit.org/changeset/121439 for more details about why it optimizes performance.
Comment 1 Kentaro Hara 2012-06-28 21:23:56 PDT
Created attachment 150080 [details]
Patch
Comment 2 WebKit Review Bot 2012-06-28 23:20:51 PDT
Comment on attachment 150080 [details]
Patch

Clearing flags on attachment: 150080

Committed r121520: <http://trac.webkit.org/changeset/121520>
Comment 3 WebKit Review Bot 2012-06-28 23:20:56 PDT
All reviewed patches have been landed.  Closing bug.
Comment 4 Alexey Proskuryakov 2012-06-28 23:58:01 PDT
Comment on attachment 150080 [details]
Patch

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

> Source/WebCore/ChangeLog:13
> +        of Element::getAttribute*() from String to AtomicString. See the ChangeLog in
> +        http://trac.webkit.org/changeset/121439 for more details about why this change
> +        optimizes performance.

Is there a measurable performance improvement on a test that reflects real life DOM use? If not, why is it OK to make this change (yes, I read the other bug's ChangeLog, but an explanation cannot trump actual measurement)?

Thrashing AtomicString table has been known to cause practical performance issues in the past.

Also, with a global replacement like this, how can you know which individual changes are progressions, and which are regressions?

> Source/WebCore/dom/DatasetDOMStringMap.cpp:113
> -static String convertPropertyNameToAttributeName(const String& name)
> +static const String convertPropertyNameToAttributeName(const String& name)

This makes no sense. Was this patch carefully reviewed, or rubber-stamped?

If you wrote per-function ChangeLog comments as always recommended, you would have noticed this apparently accidental change.
Comment 5 Kentaro Hara 2012-06-29 01:05:53 PDT
Comment on attachment 150080 [details]
Patch

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

>> Source/WebCore/ChangeLog:13
>> +        optimizes performance.
> 
> Is there a measurable performance improvement on a test that reflects real life DOM use? If not, why is it OK to make this change (yes, I read the other bug's ChangeLog, but an explanation cannot trump actual measurement)?
> 
> Thrashing AtomicString table has been known to cause practical performance issues in the past.
> 
> Also, with a global replacement like this, how can you know which individual changes are progressions, and which are regressions?

Let me investigate the performance. Though I would guess that it would be difficult to find a test case that can cause a regression caused by the thrashing.

>> Source/WebCore/dom/DatasetDOMStringMap.cpp:113
>> +static const String convertPropertyNameToAttributeName(const String& name)
> 
> This makes no sense. Was this patch carefully reviewed, or rubber-stamped?
> 
> If you wrote per-function ChangeLog comments as always recommended, you would have noticed this apparently accidental change.

This is needed. Without this, convertPropertyNameToAttributeName() in DatasetDOMStringMap.cpp leads to compile error, since Element::removeAttribute() expects 'const AtomicString&' after this patch. (I should have explained it in the ChangeLog.)
Comment 6 Kentaro Hara 2012-06-29 02:37:51 PDT
Comment on attachment 150080 [details]
Patch

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

>>> Source/WebCore/ChangeLog:13
>>> +        optimizes performance.
>> 
>> Is there a measurable performance improvement on a test that reflects real life DOM use? If not, why is it OK to make this change (yes, I read the other bug's ChangeLog, but an explanation cannot trump actual measurement)?
>> 
>> Thrashing AtomicString table has been known to cause practical performance issues in the past.
>> 
>> Also, with a global replacement like this, how can you know which individual changes are progressions, and which are regressions?
> 
> Let me investigate the performance. Though I would guess that it would be difficult to find a test case that can cause a regression caused by the thrashing.

ap: You're right. The optimization I landed was not sufficient. The patch missed replacing s/Strong/AtomicString/ in Element::removeAttribute() and Element::hasAttribute(), without which I found we cannot get a perf win. With the replacement, there is a big perf win. I'll revert the original patch and will upload patches for each optimization with a performance test.
Comment 7 Kentaro Hara 2012-06-29 02:44:27 PDT
Reverted r121520 for reason:

the performance optimization needs more investigation

Committed r121539: <http://trac.webkit.org/changeset/121539>
Comment 8 Alexey Proskuryakov 2012-06-29 07:59:26 PDT
> >> Source/WebCore/dom/DatasetDOMStringMap.cpp:113
> >> +static const String convertPropertyNameToAttributeName(const String& name)
>>
> This is needed. Without this, convertPropertyNameToAttributeName() in DatasetDOMStringMap.cpp leads to compile error, since Element::removeAttribute() expects 'const AtomicString&' after this patch. (I should have explained it in the ChangeLog.)

I still don't understand why this is needed. Perhaps the bug causing a compilation failure is elsewhere.
Comment 9 Alexey Proskuryakov 2012-06-29 08:03:02 PDT
Thank you for splitting the patch into parts, and adding micro-benchmarks. This is an improvement, although it doesn't fully alleviate my concerns about this potentially negatively impacting overall performance on real life workloads. I'm not quite sure how to prove that this is an improvement, perhaps others who work on performance aspects more closely will have a better feeling of what to expect from such a  change.
Comment 10 Kentaro Hara 2012-06-29 09:21:19 PDT
(In reply to comment #8)
> > >> Source/WebCore/dom/DatasetDOMStringMap.cpp:113
> > >> +static const String convertPropertyNameToAttributeName(const String& name)
> >>
> > This is needed. Without this, convertPropertyNameToAttributeName() in DatasetDOMStringMap.cpp leads to compile error, since Element::removeAttribute() expects 'const AtomicString&' after this patch. (I should have explained it in the ChangeLog.)
> 
> I still don't understand why this is needed. Perhaps the bug causing a compilation failure is elsewhere.

Sorry, you're right... It was not needed. I removed it from the latest patch in bug 90265.
Comment 11 Kentaro Hara 2012-06-29 09:30:13 PDT
(In reply to comment #9)
> This is an improvement, although it doesn't fully alleviate my concerns about this potentially negatively impacting overall performance on real life workloads.

FWIW, performance improvement is larger in V8 than JSC. In particular, for xxxxNS() methods, no improvement/regression in JSC but a big improvement in V8. Please look at my comment in bug 90276 for more details.

Also I would like to note that if you are concerning about the number of AtomicStrings generated, the patch we should concern is not these patches but the patch of bug 90174 (already landed). The patch of bug 90174 increased the number of AtomicStrings. On the other hand, these patches (i.e. bug 90265, bug 90273, bug 90274, bug 90276) are just changing the timing when a String is converted to an AtomicString.
Comment 12 Ryosuke Niwa 2012-06-29 09:52:31 PDT
(In reply to comment #9)
> Thank you for splitting the patch into parts, and adding micro-benchmarks. This is an improvement, although it doesn't fully alleviate my concerns about this potentially negatively impacting overall performance on real life workloads.

My expectation is that most websites don't call getAttribute, hasAttribute, removeAttribute, etc... with arbitrary strings. Given that the number of AtomicString is still bound by the number of distinct content attribute names, I don't think it'll negatively affect the performance.

> I'm not quite sure how to prove that this is an improvement, perhaps others who work on performance aspects more closely will have a better feeling of what to expect from such a  change.

Given that some benchmark has shown an improvement, I'd like to see some concrete and realistic evidence to suggest that these changes may indeed case perf. regressions if we're to re-consider these changes.
Comment 13 Kentaro Hara 2012-07-11 00:24:27 PDT
reviewers: ping? (bug 90265, bug 90273, bug 90274, bug 90276)
Comment 14 Kentaro Hara 2012-07-27 03:43:55 PDT
(In reply to comment #13)
> reviewers: ping? (bug 90265, bug 90273, bug 90274, bug 90276)

ap: Now those patches are r+ed and I'd like to land them. I think the patches are beneficial in the following points:

- They improve performance of V8 as described in each ChangeLog.

- They do not change performance of JSC for now, but will improve the performance once bug 92486 is fixed.

- I couldn't observe perf regression caused by the increased number of AtomicStrings.

Do you have opinions?