Bug 90265 – Optimize Element::removeAttribute() by replacing String with AtomicString

• Component: DOM
• Status: RESOLVED FIXED

[Kentaro Hara]

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.

[Kentaro Hara]

Created attachment 150124 [details]
Performance test

[Kentaro Hara]

Created attachment 150140 [details]
Updated performance test

[Kentaro Hara]

Created attachment 150141 [details]
Patch

[Adam Barth]

Comment on attachment 150141 [details]
Patch

#webkit tells me that AP has an opinion about this patch.

[Adam Barth]

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.

[Darin Adler]

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.

[Alexey Proskuryakov]

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

[Adam Barth]

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.

[Adam Barth]

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

[Kentaro Hara]

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

[Kentaro Hara]

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

[Kentaro Hara]

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

[Kentaro Hara]

Comment on attachment 150141 [details]
Patch

Looks like no strong objection, let me commit it.

[WebKit Review Bot]

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

[Kentaro Hara]

Created attachment 157088 [details]
patch for landing

[WebKit Review Bot]

Comment on attachment 157088 [details]
patch for landing

Clearing flags on attachment: 157088

Committed r125008: <http://trac.webkit.org/changeset/125008>