Bug 90246 – Change argument types of Element::getAttribute*() from String to AtomicString

• Status: RESOLVED
• Resolution: CONFIGURATION CHANGED
• Component: DOM

[Kentaro Hara]

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.

[Kentaro Hara]

Created attachment 150080 [details]
Patch

[WebKit Review Bot]

Comment on attachment 150080 [details]
Patch

Clearing flags on attachment: 150080

Committed r121520: <http://trac.webkit.org/changeset/121520>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

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.

[Kentaro Hara]

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.)

[Kentaro Hara]

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.

[Kentaro Hara]

Reverted r121520 for reason:

the performance optimization needs more investigation

Committed r121539: <http://trac.webkit.org/changeset/121539>

[Alexey Proskuryakov]

> >> 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.

[Alexey Proskuryakov]

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.

[Kentaro Hara]

(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.

[Kentaro Hara]

(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.

[Ryosuke Niwa]

(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.

[Kentaro Hara]

reviewers: ping? (bug 90265, bug 90273, bug 90274, bug 90276)

[Kentaro Hara]

(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?

[Ahmad Saleem]

All dependent bugs are landed and I think Comment 03 mention this landing but then it got backed out here:

https://github.com/WebKit/WebKit/commit/4d9a2888319ed8f6e9a349aab7808fc05497517e

I think these changes in one way or another have landed:

https://github.com/WebKit/WebKit/blob/99cdf40835792e421d8e35798c2fed5d8dda4860/Source/WebCore/dom/Element.cpp

https://github.com/WebKit/WebKit/blob/99cdf40835792e421d8e35798c2fed5d8dda4860/Source/WebCore/dom/Element.h

Because I can see few having AtomicString..

Even ElementDataAttribute is changed to different name:

https://github.com/WebKit/WebKit/commit/ffb27b6e3103e265a99e7d9245b7150bf2f07437

> [e-mail] - is it something RESOLVED LATER or RESOLVED CONFIGURATION CHANGED? Thanks!

[Ryosuke Niwa]

We've done this already.

[Radar WebKit Bug Importer]

<rdar://problem/97613764>