Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

LibWeb: Make character reference tokenization more spec-compliant and efficient #3011

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

LibWeb: Make character reference tokenization more spec-compliant and efficient #3011

wants to merge 4 commits into from
+5,208 −4,540

Conversation

squeek502
Copy link
Contributor

@squeek502 squeek502 commented Dec 22, 2024 •
edited
Loading

This PR fixes a few bugs in numeric character reference tokenization, but the main focus is on named character references (both in terms of correctness and performance).

Tons of details in the commit messages, so see those for the full explanation. This implementation is also based on this implementation written in Zig, and there are more nitty-gritty details available in its README if you're interested.

The tl;dr is that a DAFSA is generated at compile-time and used for efficient codepoint-by-codepoint named character reference matching.

Note: I have not touched the Swift implementation of the tokenizer, and in fact will have introduced a compile error in the Swift version with my changes. Advice on how to handle the Swift side of things is appreciated.

In a benchmark using an arbitrary set of test files found online, the tokenizer is about 1.23x faster (2070ms to 1682ms).

General benchmark code 
BENCHMARK_CASE(benchfiles)
{
    MUST(Core::Directory::for_each_entry("files"sv, Core::DirIterator::SkipParentAndBaseDir, [&](Core::DirectoryEntry const& entry, Core::Directory const& parent) {
        auto file = MUST(parent.open(entry.name, Core::File::OpenMode::Read));
        auto file_size = MUST(file->size());
        auto content = MUST(ByteBuffer::create_uninitialized(file_size));
        MUST(file->read_until_filled(content.bytes()));
        ByteString file_contents { content.bytes() };
        auto tokens = run_tokenizer(file_contents);
        return IterationDecision::Continue;
    }));
}

In a benchmark hyper-focused on named character references (see named-char-test.html here for the test file used), the tokenizer is ~8x faster (758ms to 93ms).

Named character reference benchmark code 
BENCHMARK_CASE(named_character_references)
{
    StringView path = "named-char-test.html"sv;

    auto file = MUST(Core::File::open(path, Core::File::OpenMode::Read));
    auto file_size = MUST(file->size());
    auto content = MUST(ByteBuffer::create_uninitialized(file_size));
    MUST(file->read_until_filled(content.bytes()));
    ByteString file_contents { content.bytes() };
    auto tokens = run_tokenizer(file_contents);
    dbgln("{} tokens", tokens.size());
}

The amount of data stored for the named character reference matching has also been reduced by around 95KiB.

With all the changes in this PR together, many WPT subtests now pass, and AFAICT all character reference-related subtests are now passing. Here are the results from running the WPT tests in /html/syntax/parsing (comparing master to this branch):

WPT results for /html/syntax/parsing 
  ▶ Unexpected subtest result in /html/syntax/parsing/html5lib_entities01.html?run_type=write:
  └ PASS [expected FAIL] html5lib_entities01.html 6b336a43e394d3ab7ceb2ab54c63409e8a27aded

  ▶ Unexpected subtest result in /html/syntax/parsing/html5lib_entities01.html?run_type=write:
  └ PASS [expected FAIL] html5lib_entities01.html d21511e2df56c306c78e1449c960c66e565e016e

  ▶ Unexpected subtest result in /html/syntax/parsing/html5lib_entities01.html?run_type=write:
  └ PASS [expected FAIL] html5lib_entities01.html 39107d16f24d4c7bcd40ad1239b5f4f677877ee8

  ▶ Unexpected subtest result in /html/syntax/parsing/html5lib_entities01.html?run_type=write:
  └ PASS [expected FAIL] html5lib_entities01.html bffe7b00046407080251ab6bf58cb97ce2a34893

  ▶ Unexpected subtest result in /html/syntax/parsing/html5lib_entities01.html?run_type=write:
  └ PASS [expected FAIL] html5lib_entities01.html 5aef37f1f2b9ac45adfade044c882eb09a297569

  ▶ Unexpected subtest result in /html/syntax/parsing/html5lib_entities01.html?run_type=write:
  └ PASS [expected FAIL] html5lib_entities01.html 6e2d817539fb3b2023c7bcb88ad220c136f70cf0

  ▶ Unexpected subtest result in /html/syntax/parsing/html5lib_entities01.html?run_type=write_single:
  └ PASS [expected FAIL] html5lib_entities01.html 16c694bcf0b3ff3723fa070eea7e1e82ef12a337

  ▶ Unexpected subtest result in /html/syntax/parsing/html5lib_entities01.html?run_type=write_single:
  └ PASS [expected FAIL] html5lib_entities01.html 05e04b39ef06e2367a33326f5dd566913aa6628f

  ▶ Unexpected subtest result in /html/syntax/parsing/html5lib_entities01.html?run_type=write_single:
  └ PASS [expected FAIL] html5lib_entities01.html fbf7d9fec595585869c5c595d5588b34fd175278

  ▶ Unexpected subtest result in /html/syntax/parsing/html5lib_entities01.html?run_type=write_single:
  └ PASS [expected FAIL] html5lib_entities01.html e59b0a76d7bcfb429b27e00e469f35e08a9bdd1a

  ▶ Unexpected subtest result in /html/syntax/parsing/html5lib_entities01.html?run_type=write_single:
  └ PASS [expected FAIL] html5lib_entities01.html 5ea854d6ecd4d6dd459cb36d4faf3ed36e11c073

  ▶ Unexpected subtest result in /html/syntax/parsing/html5lib_entities01.html?run_type=write_single:
  └ PASS [expected FAIL] html5lib_entities01.html 119cd15b852615cd0fce759769b4a3788595e3bb

  ▶ Unexpected subtest result in /html/syntax/parsing/html5lib_entities01.html?run_type=write_single:
  └ PASS [expected FAIL] html5lib_entities01.html bf6c90305b2856c2d9c9a146dfff867fe7a5e0f3

  ▶ Unexpected subtest result in /html/syntax/parsing/html5lib_entities01.html?run_type=write_single:
  └ PASS [expected FAIL] html5lib_entities01.html 6b336a43e394d3ab7ceb2ab54c63409e8a27aded

  ▶ Unexpected subtest result in /html/syntax/parsing/html5lib_entities01.html?run_type=write_single:
  └ PASS [expected FAIL] html5lib_entities01.html d21511e2df56c306c78e1449c960c66e565e016e

  ▶ Unexpected subtest result in /html/syntsingle:
  └ PASS [expected FAIL] html5lib_entities01.html 39107d16f24d4c7bcd40ad1239b5f4f677877ee8

  ▶ Unexpected subtest result in /html/syntax/parsing/html5lib_entities01.html?run_type=write_single:
  └ PASS [expected FAIL] html5lib_entities01.html bffe7b00046407080251ab6bf58cb97ce2a34893

  ▶ Unexpected subtest result in /html/syntax/parsing/html5lib_entities01.html?run_type=write_single:
  └ PASS [expected FAIL] html5lib_entities01.html 5aef37f1f2b9ac45adfade044c882eb09a297569

  ▶ Unexpected subtest result in /html/syntax/parsing/html5lib_entities01.html?run_type=write_single:
  └ PASS [expected FAIL] html5lib_entities01.html 6e2d817539fb3b2023c7bcb88ad220c136f70cf0

  ▶ Unexpected subtest result in /html/syntax/parsing/html5lib_entities02.html?run_type=write_single:
  └ PASS [expected FAIL] html5lib_entities02.html ea66863900b0b42deee5a77c58a432c2215c32ac

  ▶ Unexpected subtest result in /html/syntax/parsing/html5lib_entities02.html?run_type=write_single:
  └ PASS [expected FAIL] html5lib_entities02.html fe22904d5f3936bedc1fa110e6bde48895b399a0

  ▶ Unexpected subtest result in /html/syntax/parsing/html5lib_entities02.html?run_type=write_single:
  └ PASS [expected FAIL] html5lib_entities02.html 6553483a30141fcff05787287c2c212df9f468e8

  ▶ Unexpected subtest result in /html/syntax/parsing/html5lib_entities02.html?run_type=write_single:
  └ PASS [expected FAIL] html5lib_entities02.html 88d7c74afcb27bbee3e3255d9116dce9c3dc6d73

  ▶ Unexpected subtest result in /html/syntax/parsing/html5lib_entities02.html?run_type=write_single:
  └ PASS [expected FAIL] html5lib_entities02.html db5d22d3350e0a51d675dc17c641c73251a4739d

  ▶ Unexpected subtest result in /html/syntax/parsing/html5lib_entities02.html?run_type=write_single:
  └ PASS [expected FAIL] html5lib_entities02.html ea08276faa7ba526e612fc1e80047d705cd29885

  ▶ Unexpected subtest result in /html/syntax/parsing/html5lib_entities02.html?run_type=write_single:
  └ PASS [expected FAIL] html5lib_entities02.html f9d3950620f8adcbe5f9a0542c7967de4be65963

  ▶ Unexpected subtest result in /html/syntax/parsing/html5lib_entities02.html?run_type=write_single:
  └ PASS [expected FAIL] html5lib_entities02.html 8e35dacd7c296f054e58f1ce83719401c8aff8a0

  ▶ Unexpected subtest result in /html/syntax/parsing/html5lib_entities02.html?run_type=write_single:
  └ PASS [expected FAIL] html5lib_entities02.html 565c5f6744a27602bb466d6df77803a80f064752

  ▶ Unexpected subtest result in /html/syntax/parsing/html5lib_entities02.html?run_type=write_single:
  └ PASS [expected FAIL] html5lib_entities02.html f908b529ac9ca5366e1160856db2c3d17e3898c9

  ▶ Unexpected subtest result in /html/syntax/parsing/html5lib_entities02.html?run_type=write_single:
  └ PASS [expected FAIL] html5lib_entities02.html 1294ffc6bee2ee41f65a60ac48ba445b99504286

  ▶ Unexpected subtest result in /html/syntax/parsing/html5lib_entities02.html?run_type=write_single:
  └ PASS [expected FAIL] html5lib_entities02.html ba7d8cdd4b40020f7af6bdde75a3574b5771fac9

  ▶ Unexpected subtest result in /html/syntax/parsing/html5lib_entities02.html?run_type=write_single:
  └ PASS [expected FAIL] html5lib_entities02.html 6bbeec30b849cebd1366ebb2e6d2a6c1790e8c68

  ▶ OK [expected TIMEOUT] /html/syntax/parsing/html5lib_entities01.html?run_type=uri

  ▶ Unexpected subtest result in /html/syntax/parsing/html5lib_entities01.html?run_type=uri:
  └ PASS [expected FAIL] html5lib_entities01.html 6b336a43e394d3ab7ceb2ab54c63409e8a27aded

  ▶ Unexpected subtest result in /html/syntax/parsing/html5lib_entities01.html?run_type=uri:
  └ PASS [expected FAIL] html5lib_entities01.html d21511e2df56c306c78e1449c960c66e565e016e

  ▶ Unexpected subtest result in /html/syntax/parsing/html5lib_entities01.html?run_type=uri:
  └ PASS [expected FAIL] html5lib_entities01.html 39107d16f24d4c7bcd40ad1239b5f4f677877ee8

  ▶ Unexpected subtest result in /html/syntax/parsing/html5lib_entities01.html?run_type=uri:
  └ PASS [expected TIMEOUT] html5lib_entities01.html ceba8404405dd3b3b423c45411bde15bf72a846d

  ▶ Unexpected subtest result in /html/syntax/parsing/html5lib_entities01.html?run_type=uri:
  └ PASS [expected FAIL] html5lib_entities01.html bffe7b00046407080251ab6bf58cb97ce2a34893

  ▶ Unexpected subtest result in /html/syntax/parsing/html5lib_entities01.html?run_type=uri:
  └ PASS [expected FAIL] html5lib_entities01.html 5aef37f1f2b9ac45adfade044c882eb09a297569

  ▶ Unexpected subtest result in /html/syntax/parsing/html5lib_entities01.html?run_type=uri:
  └ PASS [expected FAIL] html5lib_entities01.html 6e2d817539fb3b2023c7bcb88ad220c136f70cf0

  ▶ Unexpected subtest result in /html/syntax/parsing/html5lib_entities01.html?run_type=uri:
  └ PASS [expected TIMEOUT] html5lib_entities01.html d2584faaa4dda5283955b2dc22812a018d04a72d

  ▶ Unexpected subtest result in /html/syntax/parsing/html5lib_html5test-com.html?run_type=write_single:
  └ PASS [expected FAIL] html5lib_html5test-com.html be72b058e5be0f6aef2c442d83c92c0d251fcb7f

  ▶ Unexpected subtest result in /html/syntax/parsing/html5lib_html5test-com.html?run_type=write_single:
  └ PASS [expected FAIL] html5lib_html5test-com.html ab6e31cf52c8d57d6dfdcaf7165f1abf7bd5e73d

  ▶ Unexpected subtest result in /html/syntax/parsing/html5lib_html5test-com.html?run_type=write_single:
  └ PASS [expected FAIL] html5lib_html5test-com.html 11240d9b03b14eb515d6a1d1595c5a409830ea38

  ▶ Unexpected subtest result in /html/syntax/parsing/html5lib_html5test-com.html?run_type=write_single:
  └ PASS [expected FAIL] html5lib_html5test-com.html 809c1bebcded8f43981af902442ff8a2db5d2578

  ▶ Unexpected subtest result in /html/syntax/parsing/html5lib_html5test-com.html?run_type=write_single:
  └ PASS [expected FAIL] html5lib_html5test-com.html bcbeb84f40e56a642b794d514e97e3ec303d4a79

  ▶ Unexpected subtest result in /html/syntax/parsing/html5lib_tests16.html?run_type=write_single:
  └ PASS [expected FAIL] html5lib_tests16.html 1553cebdf01dc953ed7983d39a18752a4fbb24d7

  ▶ Unexpected subtest result in /html/syntax/parsing/html5lib_tests16.html?run_type=write_single:
  └ PASS [expected FAIL] html5lib_tests16.html fea28aab54637701c5dfaef4f3fe64c72b272e1c

  ▶ Unexpected subtest result in /html/syntax/parsing/html5lib_tests2.html?run_type=write_single:
  └ PASS [expected FAIL] html5lib_tests2.html e0c43080cf61c0696031bdb097bea4f2a647cfc2

  ▶ Unexpected subtest result in /html/syntax/parsing/html5lib_tests2.html?run_type=write_single:
  └ PASS [expected FAIL] html5lib_tests2.html 8dc47e70b94f2bea514ceaa51153ec1beeeda7ef

  ▶ Unexpected subtest result in /html/syntax/parsing/html5lib_tests2.html?run_type=write:
  └ PASS [expected FAIL] html5lib_tests2.html e0c43080cf61c0696031bdb097bea4f2a647cfc2

  ▶ Unexpected subtest result in /html/syntax/parsing/html5lib_tests2.html?run_type=uri:
  └ PASS [expected FAIL] html5lib_tests2.html e0c43080cf61c0696031bdb097bea4f2a647cfc2

  ▶ Unexpected subtest result in /html/syntax/parsing/html5lib_tests24.html?run_type=write_single:
  └ PASS [expected FAIL] html5lib_tests24.html 692c2dbacf18cb758f26a3d9e7d9add4356f9067

  ▶ Unexpected subtest result in /html/syntax/parsing/html5lib_tests24.html?run_type=write_single:
  └ PASS [expected FAIL] html5lib_tests24.html 614cc9827d3a0c5f91863dde5281dd0d97f64e6d

  ▶ Unexpected subtest result in /html/syntax/parsing/html5lib_tests24.html?run_type=write_single:
  └ PASS [expected FAIL] html5lib_tests24.html c517924583ee71b8e684c9ca1f2eed5e88139a39

  ▶ Unexpected subtest result in /html/syntax/parsing/html5lib_tests24.html?run_type=write_single:
  └ PASS [expected FAIL] html5lib_tests24.html 140a82fab878c139b388d159c511eb999fe2d8c7

  ▶ Unexpected subtest result in /html/syntax/parsing/html5lib_tests24.html?run_type=write_single:
  └ PASS [expected FAIL] html5lib_tests24.html a2ddddcccb652a6529daafd4153a0e12b6d5ca8c

  ▶ Unexpected subtest result in /html/syntax/parsing/html5lib_tests24.html?run_type=write_single:
  └ PASS [expected FAIL] html5lib_tests24.html da2e30a0b6577b608bf48bbd11a16ff832bc7e46

  ▶ Unexpected subtest result in /html/syntax/parsing/html5lib_tests24.html?run_type=write_single:
  └ PASS [expected FAIL] html5lib_tests24.html 66a5777f5453bd4b5161f00df02883b6d71f7cea

  ▶ Unexpected subtest result in /html/syntax/parsing/html5lib_tests24.html?run_type=write_single:
  └ PASS [expected FAIL] html5lib_tests24.html c8d97f31b70f67005eeacc3c86ac29e577c3d0ed

  ▶ Unexpected subtest result in /html/syntax/parsing/html5lib_tests5.html?run_type=write_single:
  └ PASS [expected FAIL] html5lib_tests5.html bd7dfd1a0f74731c22b3e2d331f7c14ba7c9a4e8

  ▶ Unexpected subtest result in /html/syntax/parsing/html5lib_tests5.html?run_type=write_single:
  └ PASS [expected FAIL] html5lib_tests5.html 5f847a390a413a42fcef3d4510ddc56815c7d722

  ▶ Unexpected subtest result in /html/syntax/parsing/html5lib_tests6.html?run_type=write_single:
  └ PASS [expected FAIL] html5lib_tests6.html fa05c524ac7918197adf422a2c4be35d5eca9ddc

  ▶ Unexpected subtest result in /html/syntax/parsing/html5lib_tests6.html?run_type=write_single:
  └ PASS [expected FAIL] html5lib_tests6.html 652ae0a8ca3ab725b2d10e9866898b3419333f64

  ▶ Unexpected subtest result in /html/syntax/parsing/html5lib_webkit01.html?run_type=write_single:
  └ PASS [expected FAIL] html5lib_webkit01.html 3bea2bf663be5de2bbcdad57ac95c5933e266d42

  ▶ Unexpected subtest result in /html/syntax/parsing/ambiguous-ampersand.html:
  └ PASS [expected FAIL] Check div structure: document.write

Ran 209 tests finished in 111.6 seconds.
  • 195 ran as expected. 0 tests skipped.
  • 1 tests unexpectedly okay
  • 14 tests had unexpected subtest results

There are probably other newly passing subtests elsewhere (one I'm aware of is html/webappapis/dynamic-markup-insertion/document-write/041.html)

@squeek502
LibWeb: Don't skip named-character-references test
10d4af8 
This already passes, so there's no reason to skip it anymore
@squeek502
LibWeb: Fix numeric character reference at EOF leaking its last digit
be6f45e 
Previously, if the NumericCharacterReferenceEnd state was reached when
current_input_character was None, then the
DONT_CONSUME_NEXT_INPUT_CHARACTER macro would restore back before the
EOF, and allow the next state (after the SWITCH_TO_RETURN_STATE) to
proceed with the last digit of the numeric character reference.

For example, with something like `&LadybirdBrowser#1111`, before this commit the
output would incorrectly be `<code point with the value 1111>1` instead
of just `<code point with the value 1111>`.

Instead of putting the `if (current_input_character.has_value())` check
inside NumericCharacterReferenceEnd directly, it was instead added to
DONT_CONSUME_NEXT_INPUT_CHARACTER, because all usages of the macro
benefit from this check, even if the other existing usage sites don't
exhibit any bugs without it:

- In MarkupDeclarationOpen, if the current_input_character is EOF, then
  the previous character is always `!`, so restoring and then checking
  forward for strings like `--`, `DOCTYPE`, etc won't match and the
  BogusComment state will run one extra time (once for `!` and once
  for EOF) with no practical consequences. With the `has_value()` check,
  BogusComment will only run once with EOF.

- In AfterDOCTYPEName, ConsumeNextResult::RanOutOfCharacters can only
  occur when stopping at the insertion point, and because of how
  the code is structured, it is guaranteed that current_input_character
  is either `P` or `S`, so the `has_value()` check is irrelevant.
@squeek502
LibWeb: Fix hex character references accepting all alphabetic ASCII
4da871c 
Instead of just A-F/a-f, any char A-Z/a-z was being accepted as a valid
hexadecimal digit.
@ladybird-bot
Copy link
Collaborator

Hello!

One or more of the commit messages in this PR do not match the Ladybird code submission policy, please check the lint_commits CI job for more details on which commits were flagged and why.
Please do not close this PR and open another, instead modify your commit message(s) with git commit --amend and force push those changes to update this PR.

@squeek502 squeek502 force-pushed the named-character-references branch from 136c05a to d8be1ca Compare December 22, 2024 21:10
@squeek502 squeek502 changed the title LibWeb: Make character reference matching more spec-compliant and efficient LibWeb: Make character reference tokenization more spec-compliant and efficient Dec 22, 2024
@squeek502
LibWeb: Make named character references more spec-compliant & efficient
50ea5b4 
There are two changes happening here: a correctness fix, and an
optimization. In theory they are unrelated, but the optimization
actually paves the way for the correctness fix.

Before this commit, the HTML tokenizer would attempt to look for named
character references by checking from after the `&` until the end of
m_decoded_input, which meant that it was unable to recognize things like
named character references that are inserted via `document.write` one
byte at a time. For example, if `&notin;` was written one-byte-at-a-time
with `document.write`, then the tokenizer would only check against `n`
since that's all that would exist at the time of the check and therefore
erroneously conclude that it was an invalid named character reference.

This commit modifies the approach taken for named character reference
matching by using a trie-like structure (specifically, a deterministic
acyclic finite state automaton or DAFSA), which allows for efficiently
matching one-character-at-a-time and therefore it is able to pick up
matching where it left off after each code point is consumed.

Note: Because it's possible for a partial match to not actually develop
into a full match (e.g. `&notindo` which could lead to `&notindot;`),
some backtracking is performed after-the-fact in order to only consume
the code points within the longest match found (e.g. `&notindo` would
backtrack back to `&not`).

With this new approach, `document.write` being called one-byte-at-a-time
is handled correctly, which allows for passing more WPT tests, with the
most directly relevant tests being
`/html/syntax/parsing/html5lib_entities01.html`
and
`/html/syntax/parsing/html5lib_entities02.html`
when run with `?run_type=write_single`. Additionally, the implementation
now better conforms to the language of the spec (and resolves a FIXME)
because exactly the matched characters are consumed and nothing more, so
SWITCH_TO is able to be used as the spec says instead of RECONSUME_IN.

The new approach is also an optimization:

- Instead of a linear search using `starts_with`, the usage of a DAFSA
  means that it is always aware of which characters can lead to a match
  at any given point, and will bail out whenever a match is no longer
  possible.
- The DAFSA is able to take advantage of the note in the section
  `13.5 Named character references` that says "This list is static and
  will not be expanded or changed in the future." and tailor its Node
  struct accordingly to tightly pack each node's data into 32-bits.
  Together with the inherent DAFSA property of redundant node
  deduplication, the amount of data stored for named character reference
  matching is minimized.

In my testing:

- A benchmark tokenizing an arbitrary set of HTML test files was about
  1.23x faster (2070ms to 1682ms).
- A benchmark tokenizing a file with tens of thousands of named
  character references mixed in with truncated named character
  references and arbitrary ASCII characters/ampersands runs about 8x
  faster (758ms to 93ms).
- The size of `liblagom-web.so` was reduced by 94.96KiB.

Some technical details:

A DAFSA (deterministic acyclic finite state automaton) is essentially a
trie flattened into an array, but it also uses techniques to minimize
redundant nodes. This provides fast lookups while minimizing the
required data size, but normally does not allow for associating data
related to each word. However, by adding a count of the number of
possible words from each node, it becomes possible to also use it to
achieve minimal perfect hashing for the set of words (which allows going
from word -> unique index as well as unique index -> word). This allows
us to store a second array of data so that the DAFSA can be used as a
lookup for e.g. the associated code points.
@squeek502 squeek502 force-pushed the named-character-references branch from d8be1ca to 50ea5b4 Compare December 22, 2024 21:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@squeek502 @ladybird-bot