| Summary: | Change argument types of Element::getAttribute*() from String to AtomicString | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Kentaro Hara <haraken> | ||||
| Component: | DOM | Assignee: | Kentaro Hara <haraken> | ||||
| Status: | RESOLVED CONFIGURATION CHANGED | ||||||
| Severity: | Normal | CC: | abarth, ahmad.saleem792, ap, bfulgham, cmarcelo, ggaren, kling, koivisto, rniwa, webkit-bug-importer, webkit.review.bot | ||||
| Priority: | P2 | Keywords: | InRadar | ||||
| Version: | 528+ (Nightly build) | ||||||
| Hardware: | Unspecified | ||||||
| OS: | Unspecified | ||||||
| Bug Depends on: | 90265, 90273, 90274, 90276 | ||||||
| Bug Blocks: | |||||||
| Attachments: |
|
||||||
|
Description
Kentaro Hara
2012-06-28 21:21:53 PDT
Created attachment 150080 [details]
Patch
Comment on attachment 150080 [details] Patch Clearing flags on attachment: 150080 Committed r121520: <http://trac.webkit.org/changeset/121520> All reviewed patches have been landed. Closing bug. 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 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 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. Reverted r121520 for reason: the performance optimization needs more investigation Committed r121539: <http://trac.webkit.org/changeset/121539> > >> 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.
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. (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. (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. (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. (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? 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 > rniwa@webkit.org - is it something RESOLVED LATER or RESOLVED CONFIGURATION CHANGED? Thanks! We've done this already. |