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

Tweak HTML parser yielding #9925

Merged
merged 1 commit into from Mar 15, 2023
Merged

Tweak HTML parser yielding #9925

merged 1 commit into from Mar 15, 2023

Conversation

pvollan
Copy link
Contributor

@pvollan pvollan commented Feb 10, 2023 •
edited by webkit-early-warning-system

f17bb5a

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

Reviewed by Antti Koivisto.

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. Also
make sure that the parser only yields before executing scripts if there has been parsing progress since the last
yield. Measurements show that this is a 1-2% improvement in page load times. This change also revealed a race
condition in a test, where a call to notifyDone() in an inline script was racing with a call to waitForDone() in
a script with SRC attribute. This race was resolved by starting the test in the onload handler.

* LayoutTests/fast/forms/focus-option-control-on-page.html:
* Source/WebCore/html/parser/HTMLParserScheduler.cpp:
(WebCore::HTMLParserScheduler::shouldYieldBeforeExecutingScript):
* Source/WebCore/html/parser/HTMLParserScheduler.h:

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

0c0f933

Misc iOS, tvOS & watchOS macOS Linux Windows
⏳ 🧪 style ⏳ 🛠 ios ⏳ 🛠 mac ⏳ 🛠 wpe ⏳ 🛠 wincairo
⏳ 🧪 bindings ⏳ 🛠 ios-sim ⏳ 🛠 mac-AS-debug ⏳ 🧪 wpe-wk2
⏳ 🧪 webkitperl ⏳ 🧪 ios-wk2 ⏳ 🧪 api-mac ⏳ 🛠 gtk
⏳ 🧪 webkitpy ⏳ 🧪 api-ios ⏳ 🧪 mac-wk1 ⏳ 🧪 gtk-wk2
⏳ 🛠 🧪 jsc ⏳ 🛠 tv ⏳ 🧪 mac-wk2 ⏳ 🧪 api-gtk
⏳ 🛠 🧪 jsc-arm64 ⏳ 🛠 tv-sim ⏳ 🧪 mac-AS-debug-wk2 ⏳ 🛠 jsc-armv7
⏳ 🧪 services ⏳ 🛠 watch ⏳ 🧪 mac-wk2-stress ⏳ 🧪 jsc-armv7-tests
❌ 🛠 🧪 merge ⏳ 🛠 watch-sim ⏳ 🛠 jsc-mips
⏳ 🧪 jsc-mips-tests

@pvollan pvollan self-assigned this Feb 10, 2023
@pvollan pvollan added the WebKit Misc. For miscellaneous bugs in the WebKit framework (and not JavaScriptCore or WebCore). label Feb 10, 2023
@webkit-early-warning-system
Copy link
Collaborator

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

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
geoffreygaren reviewed Feb 10, 2023
View reviewed changes
Source/WebCore/html/parser/HTMLDocumentParser.cpp Outdated
Comment on lines 263 to 267
// Only yield if there has been progress since last yield.
if (session.processedTokens > session.processedTokensOnLastYield) {
session.processedTokensOnLastYield = session.processedTokens;
return true;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you think of any case where this check would be false, and we would want to yield anyway? If not, then I think this check belongs inside shouldYieldBeforeExecutingScript. Because our observation is that you should not yield if you haven't made any progress yet.

Copy link
Contributor

@anttijk anttijk Feb 12, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also session.processedTokensOnLastYield is not a great name since this is only updated on a before-script yield yet there is another yield path too (shouldYieldBeforeToken()).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you think of any case where this check would be false, and we would want to yield anyway? If not, then I think this check belongs inside shouldYieldBeforeExecutingScript. Because our observation is that you should not yield if you haven't made any progress yet.

That is a good point. I cannot think of such a case, and moved the check inside shouldYieldBeforeExecutingScript.

Thanks for reviewing!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also session.processedTokensOnLastYield is not a great name since this is only updated on a before-script yield yet there is another yield path too (shouldYieldBeforeToken()).

Ah, yes, that is a good point. In the latest patch I have updated the name, which is hopefully more accurate.

Thanks for reviewing!

@pvollan pvollan force-pushed the eng/Tweak-HTML-parser-yielding branch from b6c01b2 to f8f930e Compare February 14, 2023 15:30
@webkit-early-warning-system
Copy link
Collaborator

EWS run on current version of this PR (hash f8f930e)

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

@pvollan pvollan force-pushed the eng/Tweak-HTML-parser-yielding branch from f8f930e to 0c0f933 Compare March 7, 2023 18:01
anttijk
anttijk approved these changes Mar 14, 2023
View reviewed changes
Source/WebCore/html/parser/HTMLParserScheduler.cpp
Comment on lines +104 to +108
static bool parsingProgressedSinceLastYield(PumpSession& session)
{
// Only yield if there has been progress since last yield.
if (session.processedTokens > session.processedTokensOnLastYieldBeforeScript) {
session.processedTokensOnLastYieldBeforeScript = session.processedTokens;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name of the variable session.processedTokensOnLastYieldBeforeScript is not super accurate since we only update it if we get here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yes, that is a good point. Since the name is already quite long, I wonder if we should look into calling this function in the other places where we yield before executing a script, since we should not yield if there hasn't been any progress on parsing. I will look into that in a follow-up patch.

Thanks for reviewing!

@pvollan pvollan added the merge-queue Applied to send a pull request to merge-queue label Mar 15, 2023
@pvollan
Tweak HTML parser yielding
f17bb5a 
https://bugs.webkit.org/show_bug.cgi?id=228780
rdar://81517847

Reviewed by Antti Koivisto.

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. Also
make sure that the parser only yields before executing scripts if there has been parsing progress since the last
yield. Measurements show that this is a 1-2% improvement in page load times. This change also revealed a race
condition in a test, where a call to notifyDone() in an inline script was racing with a call to waitForDone() in
a script with SRC attribute. This race was resolved by starting the test in the onload handler.

* LayoutTests/fast/forms/focus-option-control-on-page.html:
* Source/WebCore/html/parser/HTMLParserScheduler.cpp:
(WebCore::HTMLParserScheduler::shouldYieldBeforeExecutingScript):
* Source/WebCore/html/parser/HTMLParserScheduler.h:

Canonical link: https://commits.webkit.org/261705@main
@webkit-commit-queue
Copy link
Collaborator

Committed 261705@main (f17bb5a): https://commits.webkit.org/261705@main

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

@webkit-commit-queue webkit-commit-queue merged commit f17bb5a into WebKit:main Mar 15, 2023
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Mar 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anttijk anttijk anttijk approved these changes

@cdumez cdumez Awaiting requested review from cdumez cdumez is a code owner

@rniwa rniwa Awaiting requested review from rniwa rniwa is a code owner

@alanbaradlay alanbaradlay Awaiting requested review from alanbaradlay

@sundiamonde sundiamonde Awaiting requested review from sundiamonde

@bnham bnham Awaiting requested review from bnham

@geoffreygaren geoffreygaren Awaiting requested review from geoffreygaren

Assignees

@pvollan pvollan

Labels
WebKit Misc. For miscellaneous bugs in the WebKit framework (and not JavaScriptCore or WebCore).
Projects
None yet
Milestone
No milestone
5 participants
@pvollan @webkit-early-warning-system @webkit-commit-queue @geoffreygaren @anttijk