Based on the observation described in this ChangeLog (http://trac.webkit.org/changeset/121439), the performance of Element::removeAttribute() can be optimized by replacing String with AtomicString.
Created attachment 150124 [details] Performance test
Created attachment 150140 [details] Updated performance test
Created attachment 150141 [details] Patch
Comment on attachment 150141 [details] Patch #webkit tells me that AP has an opinion about this patch.
Based on further discussion in #webkit, my current understanding is that ap is ok with these patches. I'm going to r+ them, but please give ap a chance to respond before landing.
Comment on attachment 150141 [details] Patch This is faster in some cases. But a case I’d expect this makes slower is when the passed-in attribute name is uppercase. This adds a lookup in the atomic string table of the not-yet-lowercased string.
FWIW, my comments are in bug 90246, and are not as specific as Darin's one above. I have no general objection to these patches as long as they show measurable improvement on benchmarks such as Dromaeo (not just on micro-benchmarks created just for one code path).
Comment on attachment 150141 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=150141&action=review > Source/WebCore/dom/Element.cpp:1476 > -void Element::removeAttributeNS(const String& namespaceURI, const String& localName) > +void Element::removeAttributeNS(const AtomicString& namespaceURI, const AtomicString& localName) I see. That doesn't seem to be a problem with this part of the change, however.
@Kentaro: One way to make progress here is to land the NS parts of these changes since they are a strict improvement. For the non-NS versions, is there a measurable change on non-micro benchmarks?
(In reply to comment #9) > @Kentaro: One way to make progress here is to land the NS parts of these changes since they are a strict improvement. > > For the non-NS versions, is there a measurable change on non-micro benchmarks? OK, let me land the NS parts. For non-NS parts I'll investigate them on Monday.
(In reply to comment #7) >> as long as they show measurable improvement on benchmarks such as Dromaeo (not just on micro-benchmarks created just for one code path). >> >(In reply to comment #9) > For the non-NS versions, is there a measurable change on non-micro benchmarks? ap, abarth: Unfortunately, I couldn't measure it because there are no tests in Dromaeo and WebKit performance tests for getAttributeNode(), hasAttribute() and removeAttribute(). So the rationale for the patches would be (1) performance improvement in micro benchmarks and (2) code consistency (i.e. mixing String and AtomicString is confusing).
(In reply to comment #11) > ap, abarth: Unfortunately, I couldn't measure it because there are no tests in Dromaeo and WebKit performance tests for getAttributeNode(), hasAttribute() and removeAttribute(). > > So the rationale for the patches would be (1) performance improvement in micro benchmarks and (2) code consistency (i.e. mixing String and AtomicString is confusing). ap: May I land them? (For performance improvement in micro benchmarks and code consistency.)
Comment on attachment 150141 [details] Patch Looks like no strong objection, let me commit it.
Comment on attachment 150141 [details] Patch Rejecting attachment 150141 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: g Hunk #1 succeeded at 1 with fuzz 3. patching file Source/WebCore/dom/Element.cpp Hunk #1 FAILED at 1459. Hunk #2 succeeded at 1511 with fuzz 1 (offset 38 lines). 1 out of 2 hunks FAILED -- saving rejects to file Source/WebCore/dom/Element.cpp.rej patching file Source/WebCore/dom/Element.h Hunk #1 succeeded at 201 (offset 4 lines). Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Adam Barth']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue/ Full output: http://queues.webkit.org/results/13456289
Created attachment 157088 [details] patch for landing
Comment on attachment 157088 [details] patch for landing Clearing flags on attachment: 157088 Committed r125008: <http://trac.webkit.org/changeset/125008>