Bug 236570 - Cache an entire attribute QualifiedName when parsing HTML, not just its local name AtomString
Summary: Cache an entire attribute QualifiedName when parsing HTML, not just its local...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Cameron McCormack (:heycam)
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-02-13 13:31 PST by Cameron McCormack (:heycam)
Modified: 2022-02-16 19:22 PST (History)
9 users (show)

See Also:


Attachments
Patch (27.77 KB, patch)
2022-02-13 14:00 PST, Cameron McCormack (:heycam)
no flags Details | Formatted Diff | Diff
Patch (27.51 KB, patch)
2022-02-13 17:35 PST, Cameron McCormack (:heycam)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Cameron McCormack (:heycam) 2022-02-13 13:31:51 PST
Bug 229907 added HTMLAtomStringCache, which uses a fast to compute hash that works well to cache HTML tag names, attribute names, and attribute value.  When AtomHTMLToken initializes its list of Attributes, it uses HTMLAtomStringCache to look up or create an AtomString for the attribute's local name, and then creates a QualifiedName to wrap it.  QualifiedName construction involves looking up QualifiedNameCache, which is a thread-specific cache of QualifiedNameImpl objects.  If we make HTMLAtomStringCache responsible for caching an attribute's QualifiedName instead of just its local name AtomString, we can avoid the work of looking up the QualifiedNameCache.

Doing this results in a 0.2-0.3% improvement on Speedometer 2, and a 0.3-0.4% improvement on PLT5.
Comment 1 Radar WebKit Bug Importer 2022-02-13 13:32:03 PST
<rdar://problem/88876545>
Comment 2 Cameron McCormack (:heycam) 2022-02-13 14:00:49 PST
Created attachment 451841 [details]
Patch
Comment 3 Darin Adler 2022-02-13 15:29:28 PST
Comment on attachment 451841 [details]
Patch

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

> Source/WebCore/html/parser/HTMLNameCache.h:38
> +    ALWAYS_INLINE static AtomString makeTagName(const Vector<UChar, inlineCapacity>& string)

Can this take a Span<UChar> instead? Should be just as fast, although we’d need to test.

> Source/WebCore/html/parser/HTMLNameCache.h:44
> +    ALWAYS_INLINE static QualifiedName makeAttributeQualifiedName(const Vector<UChar, inlineCapacity>& string)

Ditto.

> Source/WebCore/html/parser/HTMLNameCache.h:50
> +    ALWAYS_INLINE static AtomString makeAttributeValue(const Vector<UChar, inlineCapacity>& string)

Ditto.

> Source/WebCore/html/parser/HTMLNameCache.h:67
> +    ALWAYS_INLINE static AtomString makeAtomString(const Vector<UChar, inlineCapacity>& string)

Ditto.

> Source/WebCore/html/parser/HTMLNameCache.h:89
> +    ALWAYS_INLINE static QualifiedName makeQualifiedName(const Vector<UChar, inlineCapacity>& string)

Ditto.

> Source/WebCore/html/parser/HTMLNameCache.h:134
> +    using QualifiedNameCache = std::array<RefPtr<QualifiedName::QualifiedNameImpl>, capacity>;

Why is this RefPtr<Impl> instead of just QualifiedName?
Comment 4 Cameron McCormack (:heycam) 2022-02-13 15:45:47 PST
(In reply to Darin Adler from comment #3)
> Can this take a Span<UChar> instead? Should be just as fast, although we’d
> need to test.

Yes, should be able to.  Then it could be untemplatized.

> > Source/WebCore/html/parser/HTMLNameCache.h:134
> > +    using QualifiedNameCache = std::array<RefPtr<QualifiedName::QualifiedNameImpl>, capacity>;
> 
> Why is this RefPtr<Impl> instead of just QualifiedName?

std::array<QualifiedName> would require QualifiedName have a default constructor, which it doesn't currently, since it doesn't support its m_impl being null.
Comment 5 Cameron McCormack (:heycam) 2022-02-13 17:35:06 PST
Created attachment 451849 [details]
Patch
Comment 6 EWS 2022-02-16 19:22:13 PST
Committed r289991 (247377@main): <https://commits.webkit.org/247377@main>

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