HTML parser should yield more aggressively

• Component: DOM
• Status: RESOLVED FIXED

[Antti Koivisto]

This allows more rendering updates during page loading.

[Antti Koivisto]

rdar://73458064

[Antti Koivisto]

Created attachment 426107 [details]
patch

[Antti Koivisto]

Created attachment 426108 [details]
patch

[Darin Adler]

Comment on attachment 426108 [details]
patch

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

> Source/WebCore/html/parser/HTMLParserScheduler.cpp:37
> +// FIXME: Yielding should probably be driven by display refresh.

It’s possible that while staying brief, we could say this even more directly and mention *why* that would be good.

[Antti Koivisto]

> It’s possible that while staying brief, we could say this even more directly
> and mention *why* that would be good.

And take away the space for imagination and creativity?

[Alexey Proskuryakov]

Nice collection of tests that need to be improved uncovered!

EWS only find up to 30 failures, so there are likely more. We probably don't want to just mark them as flaky, as it's quite a bit of test coverage.

[Antti Koivisto]

Created attachment 427129 [details]
wip

[Antti Koivisto]

Created attachment 428939 [details]
wip

[Antti Koivisto]

Created attachment 429040 [details]
wip

[Antti Koivisto]

Created attachment 429149 [details]
patch

[Antti Koivisto]

Created attachment 429150 [details]
patch

[Darin Adler]

Comment on attachment 429150 [details]
patch

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

> Source/WebCore/ChangeLog:26
> +        Consider yielding before script execution after 16ms has elapsed and at least 256 tokens have been parsed.

This is very straightforward to read in the code.

> Source/WebCore/ChangeLog:28
> +        Only yield for synchronous scripts.
> +        Don't yield on very short inline scripts (this is an imperfect way to try to guess the execution cost).

This is harder to understand, so some comment would be good with the constant that defines a short inline script and the code that’s checks a script is synchronous. I really like the "this is an imperfect way to try to guess the execution cost" remark, the core of a classically helpful "why" comment, and similarly, a comment that says why we don't need to yield for async scripts would be a plus.

> Source/WebCore/html/parser/HTMLParserScheduler.cpp:123
> +    static const auto elapsedTimeLimit = 16_ms;
> +    static const auto tokenLimit = 256;
> +    static const auto inlineScriptLengthLimit = 1024;

Should use constexpr.

[Antti Koivisto]

Created attachment 429201 [details]
patch

[EWS]

Committed r277818 (237965@main): <https://commits.webkit.org/237965@main>

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

[Chris Dumez]

This appears to have caused Bug 231138.