Bug 224609 - HTML parser should yield more aggressively
Summary: HTML parser should yield more aggressively
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on: 231138
Blocks:
  Show dependency tree / graph
 
Reported: 2021-04-15 08:12 PDT by Antti Koivisto
Modified: 2021-10-04 08:26 PDT (History)
10 users (show)

See Also:

Attachments
patch (1.27 KB, patch)
2021-04-15 08:16 PDT, Antti Koivisto 		no flags Details | Formatted Diff | Diff
patch (1.38 KB, patch)
2021-04-15 08:20 PDT, Antti Koivisto 		darin: review+
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
wip (12.42 KB, patch)
2021-04-27 02:08 PDT, Antti Koivisto 		no flags Details | Formatted Diff | Diff
wip (26.87 KB, patch)
2021-05-18 08:01 PDT, Antti Koivisto 		ews-feeder: commit-queue-
Details | Formatted Diff | Diff
wip (26.37 KB, patch)
2021-05-19 04:45 PDT, Antti Koivisto 		ews-feeder: commit-queue-
Details | Formatted Diff | Diff
patch (10.48 KB, patch)
2021-05-20 02:27 PDT, Antti Koivisto 		no flags Details | Formatted Diff | Diff
patch (10.45 KB, patch)
2021-05-20 02:53 PDT, Antti Koivisto 		darin: review+
Details | Formatted Diff | Diff
patch (10.72 KB, patch)
2021-05-20 12:00 PDT, Antti Koivisto 		no flags Details | Formatted Diff | Diff
Show Obsolete (7) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Antti Koivisto 2021-04-15 08:12:21 PDT 
This allows more rendering updates during page loading.
Comment 1 Antti Koivisto 2021-04-15 08:13:11 PDT 
rdar://73458064
Comment 2 Antti Koivisto 2021-04-15 08:16:42 PDT 
Created attachment 426107 [details]
patch
Comment 3 Antti Koivisto 2021-04-15 08:20:02 PDT 
Created attachment 426108 [details]
patch
Comment 4 Darin Adler 2021-04-15 08:25:11 PDT 
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.
Comment 5 Antti Koivisto 2021-04-15 10:48:39 PDT 
> 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?
Comment 6 Alexey Proskuryakov 2021-04-15 15:08:57 PDT 
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.
Comment 7 Antti Koivisto 2021-04-27 02:08:51 PDT 
Created attachment 427129 [details]
wip
Comment 8 Antti Koivisto 2021-05-18 08:01:32 PDT 
Created attachment 428939 [details]
wip
Comment 9 Antti Koivisto 2021-05-19 04:45:24 PDT 
Created attachment 429040 [details]
wip
Comment 10 Antti Koivisto 2021-05-20 02:27:18 PDT 
Created attachment 429149 [details]
patch
Comment 11 Antti Koivisto 2021-05-20 02:53:52 PDT 
Created attachment 429150 [details]
patch
Comment 12 Darin Adler 2021-05-20 09:02:58 PDT 
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.
Comment 13 Antti Koivisto 2021-05-20 12:00:41 PDT 
Created attachment 429201 [details]
patch
Comment 14 EWS 2021-05-20 12:40:12 PDT 
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].
Comment 15 Chris Dumez 2021-10-04 08:26:57 PDT 
This appears to have caused Bug 231138.