Bug 208741

Summary: Generate a trie parser for tag names
Product: WebKit Reporter: Tadeu Zagallo <tzagallo>
Component: WebCore Misc.Assignee: Tadeu Zagallo <tzagallo>
Status: NEW ---    
Severity: Normal CC: cdumez, esprehn+autocc, ews-watchlist, gyuyoung.kim, kangil.han, saam, sam, ysuzuki
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
patch
none
WIP none

Description Tadeu Zagallo 2020-03-06 15:41:48 PST
Instead of adding the chars to a vector than using it to create an AtomString we can generate a trie parser for builtin tags that will give us the AtomString directly (and the element constructor). This skips hashing the string, the atomic string table look and element table lookup.
Comment 1 Tadeu Zagallo 2020-03-06 15:44:15 PST
Created attachment 392792 [details]
Patch
Comment 2 Sam Weinig 2020-03-07 11:33:31 PST
Comment on attachment 392792 [details]
Patch

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

> Source/WebCore/ChangeLog:12
> +        Instead of adding the chars to a vector than using it to create an AtomString we can generate a trie
> +        parser for builtin tags that will give us the AtomString directly (and the element constructor). This
> +        skips hashing the string, the atomic string table look and element table lookup.
> +
> +        No new tests since no functionality changed.

Does this end up being a win on any benchmarks? What is the underlying goal of making this change?
Comment 3 Yusuke Suzuki 2020-03-09 00:39:43 PDT
Comment on attachment 392792 [details]
Patch

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

> Source/WebCore/html/parser/AtomicHTMLToken.h:253
> +            if (!(++counter % 1000))
> +                dataLogLn("Counter: ", counter, " (", makeString(m_name), ")");

Let's remove this debugging purpose code.
Comment 4 Tadeu Zagallo 2020-03-09 11:49:50 PDT
(In reply to Sam Weinig from comment #2)
> Does this end up being a win on any benchmarks? What is the underlying goal
> of making this change?

Sorry, didn't mean to r? yet. I'm still running the benchmarks, but the idea is that it may help with Speedometer.
Comment 5 Saam Barati 2021-08-02 17:03:24 PDT
Created attachment 434805 [details]
patch

rebased, but it has a hang. Gonna figure out why. Wanna test perf of this against ToT.
Comment 6 Saam Barati 2021-08-05 15:20:11 PDT
Created attachment 435029 [details]
WIP