Tweak HTML parser yielding #9394

pvollan · 2023-01-31T19:17:45Z

`6ce95a8`

Tweak HTML parser yielding
https://bugs.webkit.org/show_bug.cgi?id=228780
rdar://81517847

Reviewed by Ryosuke Niwa.

Tweak html parser yielding to attempt to improve page load performance with respect to certain page load metrics.
This patch makes the parser yield if the page has not painted before and meaningful content has been loaded. So
far, this looks like a 1-2% improvement in page load times.

* Source/WebCore/html/parser/HTMLParserScheduler.cpp:
(WebCore::HTMLParserScheduler::shouldYieldBeforeExecutingScript):

Canonical link: https://commits.webkit.org/260011@main

2e8d963

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🛠 gtk
✅ 🧪 webkitperl	~~🧪 ios-wk2~~	✅ 🧪 api-mac	~~🧪 gtk-wk2~~
	✅ 🧪 api-ios	~~🧪 mac-wk1~~	~~🧪 api-gtk~~
	✅ 🛠 tv	~~🧪 mac-wk2~~
	✅ 🛠 tv-sim	~~🧪 mac-AS-debug-wk2~~
	✅ 🛠 watch	✅ 🧪 mac-wk2-stress
✅ 🛠 🧪 merge	✅ 🛠 watch-sim

webkit-early-warning-system · 2023-01-31T19:19:00Z

EWS run on previous version of this PR (hash df59f34)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🛠 gtk
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 mac-wk1	✅ 🧪 api-gtk
	✅ 🛠 tv	✅ 🧪 mac-wk2
	✅ 🛠 tv-sim	✅ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch	✅ 🧪 mac-wk2-stress
	✅ 🛠 watch-sim

geoffreygaren · 2023-01-31T19:30:13Z

Seems like a neat idea to me; but I'd like to get Alan or Antti's opinion too.

rniwa · 2023-01-31T19:33:57Z

Source/WebCore/html/parser/HTMLParserScheduler.cpp

@@ -120,6 +120,9 @@ bool HTMLParserScheduler::shouldYieldBeforeExecutingScript(const ScriptElement*
    if (UNLIKELY(m_documentHasActiveParserYieldTokens))
        return true;

+    if (document->view() && !document->view()->hasEverPainted() && document->isLayoutPending())


It's funny there is a comment above saying:

If we've never painted before and a layout is pending, yield prior to running
scripts to give the page a chance to paint earlier.

Yes :) I should probably move that comment down to the change.

Thanks for reviewing!

geoffreygaren · 2023-01-31T19:34:38Z

Source/WebCore/html/parser/HTMLParserScheduler.cpp

+    if (document->view() && !document->view()->hasEverPainted() && document->isLayoutPending())
+        return true;
+


I wonder if we should also A/B test moving the "if (scriptElement)" block above this line. Forcing layout just because we parse an async or defer script tag seems wrong. Forcing layout before we execute a short inline script seems like an experimental question.

That is a good point, I will schedule that A/B test.

Thanks for reviewing!

alanbaradlay · 2023-01-31T19:36:44Z

Does this "page load metrics" include time for first display (when we decide to show content to the user)? Curious if this has any impact on it (since now we may delay that pending layout potentially producing paints at a later time).

alanbaradlay · 2023-01-31T19:38:16Z

(though also one could argue that processing more content first could lead to a non-empty layout sooner)

pvollan · 2023-01-31T23:08:22Z

Does this "page load metrics" include time for first display (when we decide to show content to the user)? Curious if this has any impact on it (since now we may delay that pending layout potentially producing paints at a later time).

Yes, first meaningful paint is one of these page load metrics. So far, it looks like this change will improve this metric on some of the pages, but I am looking to run more tests.

Thanks for reviewing!

webkit-early-warning-system · 2023-02-08T15:34:55Z

EWS run on current version of this PR (hash 2e8d963)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🛠 gtk
✅ 🧪 webkitperl	~~🧪 ios-wk2~~	✅ 🧪 api-mac	~~🧪 gtk-wk2~~
	✅ 🧪 api-ios	~~🧪 mac-wk1~~	~~🧪 api-gtk~~
	✅ 🛠 tv	~~🧪 mac-wk2~~
	✅ 🛠 tv-sim	~~🧪 mac-AS-debug-wk2~~
	✅ 🛠 watch	✅ 🧪 mac-wk2-stress
✅ 🛠 🧪 merge	✅ 🛠 watch-sim

https://bugs.webkit.org/show_bug.cgi?id=228780 rdar://81517847 Reviewed by Ryosuke Niwa. Tweak html parser yielding to attempt to improve page load performance with respect to certain page load metrics. This patch makes the parser yield if the page has not painted before and meaningful content has been loaded. So far, this looks like a 1-2% improvement in page load times. * Source/WebCore/html/parser/HTMLParserScheduler.cpp: (WebCore::HTMLParserScheduler::shouldYieldBeforeExecutingScript): Canonical link: https://commits.webkit.org/260011@main

webkit-commit-queue · 2023-02-08T16:50:23Z

Committed 260011@main (6ce95a8): https://commits.webkit.org/260011@main

Reviewed commits have been landed. Closing PR #9394 and removing active labels.

aj062 · 2023-02-08T21:00:46Z

@pvollan This seems to have caused 60+ test failures on mac tester queues. EWS (unfinished) build on this PR also had failures, see https://ews-build.webkit.org/#/builders/73/builds/27722

Failures on post-commit bots: https://build.webkit.org/#/builders/48

pvollan requested review from cdumez and rniwa as code owners January 31, 2023 19:17

pvollan self-assigned this Jan 31, 2023

pvollan added the WebKit Misc. For miscellaneous bugs in the WebKit framework (and not JavaScriptCore or WebCore). label Jan 31, 2023

pvollan requested review from sundiamonde, anttijk, bnham and geoffreygaren January 31, 2023 19:18

geoffreygaren requested a review from alanbaradlay January 31, 2023 19:29

rniwa reviewed Jan 31, 2023

View reviewed changes

rniwa approved these changes Jan 31, 2023

View reviewed changes

geoffreygaren reviewed Jan 31, 2023

View reviewed changes

pvollan force-pushed the eng/Tweak-HTML-parser-yielding branch from df59f34 to 2e8d963 Compare February 8, 2023 15:32

pvollan added the merge-queue Applied to send a pull request to merge-queue label Feb 8, 2023

webkit-early-warning-system force-pushed the eng/Tweak-HTML-parser-yielding branch from 2e8d963 to 6ce95a8 Compare February 8, 2023 16:50

webkit-early-warning-system merged commit 6ce95a8 into WebKit:main Feb 8, 2023

webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Feb 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tweak HTML parser yielding #9394

Tweak HTML parser yielding #9394

pvollan commented Jan 31, 2023 •

edited by webkit-early-warning-system

webkit-early-warning-system commented Jan 31, 2023 •

edited

geoffreygaren commented Jan 31, 2023

rniwa Jan 31, 2023

pvollan Jan 31, 2023

pvollan Jan 31, 2023

geoffreygaren Jan 31, 2023

pvollan Jan 31, 2023

alanbaradlay commented Jan 31, 2023

alanbaradlay commented Jan 31, 2023

pvollan commented Jan 31, 2023

webkit-early-warning-system commented Feb 8, 2023 •

edited

webkit-commit-queue commented Feb 8, 2023

aj062 commented Feb 8, 2023

		if (document->view() && !document->view()->hasEverPainted() && document->isLayoutPending())
		return true;

Tweak HTML parser yielding #9394

Tweak HTML parser yielding #9394

Conversation

pvollan commented Jan 31, 2023 • edited by webkit-early-warning-system

6ce95a8

webkit-early-warning-system commented Jan 31, 2023 • edited

geoffreygaren commented Jan 31, 2023

rniwa Jan 31, 2023

Choose a reason for hiding this comment

pvollan Jan 31, 2023

Choose a reason for hiding this comment

pvollan Jan 31, 2023

Choose a reason for hiding this comment

geoffreygaren Jan 31, 2023

Choose a reason for hiding this comment

pvollan Jan 31, 2023

Choose a reason for hiding this comment

alanbaradlay commented Jan 31, 2023

alanbaradlay commented Jan 31, 2023

pvollan commented Jan 31, 2023

webkit-early-warning-system commented Feb 8, 2023 • edited

webkit-commit-queue commented Feb 8, 2023

aj062 commented Feb 8, 2023

pvollan commented Jan 31, 2023 •

edited by webkit-early-warning-system

`6ce95a8`

webkit-early-warning-system commented Jan 31, 2023 •

edited

webkit-early-warning-system commented Feb 8, 2023 •

edited