Bug 239314 – Use [AtomString] where appropriate in IDL files for performance

• Status: RESOLVED
• Resolution: FIXED
• Component: Bindings

[Chris Dumez]

Use [AtomString] where appropriate in IDL files for performance. I added [AtomString] on the IDL side whenever our C++ implementation uses AtomString.
Without this, the generated bindings code will generate a String, which will then get atomized once passed to our implementation. This means we're doing unnecessary String allocations in cases where the AtomString is already in the AtomStringTable.

[Chris Dumez]

Created attachment 457575 [details]
Patch

[Chris Dumez]

Note that in some cases, we may be able to use [RequiresExistingAtomString] for even better performance. However, I didn't do that in this patch because this is less mechanical and requires closer inspection. I'll follow up as needed.

[Alexey Shvayka]

Comment on attachment 457575 [details]
Patch

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

Nice!

> Source/WebCore/dom/Document.idl:61
> +    [NewObject, ImplementedAs=createElementForBindings] Element createElement([AtomString] DOMString localName); // FIXME: missing options parameter.

unrelated nit: we might want to remove this FIXME as `options` parameter is part of customised built-ins API which we won't implement.

> Source/WebCore/dom/Document.idl:62
> +    [NewObject] Element createElementNS([AtomString] DOMString? namespaceURI, [AtomString] DOMString qualifiedName); // FIXME: missing options parameter.

ditto

[Chris Dumez]

Created attachment 457576 [details]
Patch

[Chris Dumez]

Created attachment 457577 [details]
Patch

[Darin Adler]

Looking forward to the [RequiresExistingAtomString] follow-up too!

[Darin Adler]

Comment on attachment 457577 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        Use [AtomString] where appropriate in IDL files for performance. I added [AtomString] on the
> +        IDL side whenever our C++ implementation uses AtomString.

I remember a while back that we thought that some day we might be able to use template cleverness to get the bindings generation to do this automatically based on the type of the function and make the keyword a no-op (and then remove it). But in the mean time, and in case we never manage to figure out how to do that, it’s great to use it the way it was intended.

> Source/WebCore/html/HTMLInputElement.idl:59
> +    [CEReactions=NotNeeded] attribute [AtomString] DOMString type; // readonly dropped as part of DOM level 2

Wow that comment is *old*, and sure doesn't seem to add much value. But you’re so close to committing, please don’t worry about it.

[Chris Dumez]

Created attachment 457584 [details]
Patch

[EWS]

Committed r292854 (249625@main): <https://commits.webkit.org/249625@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 457584 [details].

[Radar WebKit Bug Importer]

<rdar://problem/91733055>

[Chris Dumez]

Confirmed 0.6% progression on Speedometer on Apple Silicon (neutral on Intel) via A/B bots.