Tweak html parser yielding to attempt to improve page load performance with respect to certain page load metrics.
<rdar://problem/81517847>
Created attachment 434910 [details] Patch
Comment on attachment 434910 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=434910&action=review > Source/WebCore/html/parser/HTMLParserScheduler.cpp:-126 > - if (elapsedTime < elapsedTimeLimit) Is there any reason to not retain the 16 ms upper bound on yielding? I think with this change we will quickly yield if we have a layout pending after first meaningful paint, but if that's not true we might no longer yield at 16 ms. I guess I'm suggesting we retain the current time limit, but check for it after first checking for the pending layout.
Comment on attachment 434910 [details] Patch I think Antti needs to review this, since he recently established this behavior based on extensive testing, and there were lots of tradeoffs involved. Can you share which page load metric(s) improve when we make this change? I see the logic of not yielding when there is no pending layout (the main goal of yielding is to show something new to the user, and if there is no pending layout, there is nothing new to show); but why also test isVisuallyNonEmpty()? If we have not painted yet, and we have nothing to paint now, what is the point of yielding the parser? Wouldn't it be better to keep parsing, in the hopes of finding something new to show to the user? Side note: I believe the document->view()->isVisuallyNonEmpty() check tests for first paint, not first *meaningful* paint. I'm not sure if this influences the code at all, but assuming my understanding is correct, let's fix up the ChangeLog.
Also: Please see my comment in Radar.
isVisuallyNonEmpty() does not really have much to do with _painting_ (it's a legacy term, we should probably change it). It only tells you whether we believe we've got enough content to present to the user. >document->view()->isVisuallyNonEmpty() && !document->isLayoutTimerActive() Is this about letting us paint sooner after we established that there's enough content to show (i.e. we are in between (a)figured we've got enough content to show (b)paint)? -If so, I am a bit confused by the layout timer check.
>isVisuallyNonEmpty() does not really have much to do with _painting_ Let's make it a bit more accurate. The subsequent paint is supposed to be visually non-empty.
(In reply to Brent Fulgham from comment #3) > Comment on attachment 434910 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=434910&action=review > > > Source/WebCore/html/parser/HTMLParserScheduler.cpp:-126 > > - if (elapsedTime < elapsedTimeLimit) > > Is there any reason to not retain the 16 ms upper bound on yielding? I think > with this change we will quickly yield if we have a layout pending after > first meaningful paint, but if that's not true we might no longer yield at > 16 ms. > > I guess I'm suggesting we retain the current time limit, but check for it > after first checking for the pending layout. That is a good point. I replaced the absolute timeout since I think it can lead to difference in behavior between devices. I will try measuring performance with your suggested change. Thanks for reviewing!
(In reply to Geoffrey Garen from comment #4) > Comment on attachment 434910 [details] > Patch > > I think Antti needs to review this, since he recently established this > behavior based on extensive testing, and there were lots of tradeoffs > involved. > > Can you share which page load metric(s) improve when we make this change? > See radar for more information on this. > I see the logic of not yielding when there is no pending layout (the main > goal of yielding is to show something new to the user, and if there is no > pending layout, there is nothing new to show); but why also test > isVisuallyNonEmpty()? If we have not painted yet, and we have nothing to > paint now, what is the point of yielding the parser? Wouldn't it be better > to keep parsing, in the hopes of finding something new to show to the user? > Yes, that is a good point. I will try measuring performance without the check for isVisuallyNonEmpty(). > Side note: I believe the document->view()->isVisuallyNonEmpty() check tests > for first paint, not first *meaningful* paint. I'm not sure if this > influences the code at all, but assuming my understanding is correct, let's > fix up the ChangeLog. Ah, that is good to know. Thanks for reviewing!
(In reply to zalan from comment #6) > isVisuallyNonEmpty() does not really have much to do with _painting_ (it's a > legacy term, we should probably change it). It only tells you whether we > believe we've got enough content to present to the user. > Ah, thanks, that is good to know! > >document->view()->isVisuallyNonEmpty() && !document->isLayoutTimerActive() > Is this about letting us paint sooner after we established that there's > enough content to show (i.e. we are in between (a)figured we've got enough > content to show (b)paint)? -If so, I am a bit confused by the layout timer > check. The intention behind this was to yield if there is a pending layout in order to give layout and painting priority. Although, as Geoff pointed out, the check for isVisuallyNonEmpty() might be incorrect, then. Thanks for reviewing!
Comment on attachment 434910 [details] Patch ASi comp PLT is showing 2.5% regression from this patch. No data from intel bots.
(In reply to Antti Koivisto from comment #11) > Comment on attachment 434910 [details] > Patch > > ASi comp PLT is showing 2.5% regression from this patch. No data from intel > bots. I will look into if further tuning is possible to avoid this regression while keeping the progression. Thanks for reviewing!
I'd be interested in an A/B test of just adding if (!document->isLayoutTimerActive()) return false; In theory, this might improve Competitive PLT by reducing parser yielding when we have nothing to render. But it might harm PLT5's domContentLoaded metric. I'd also be interested in an A/B test of adding if (!document->isLayoutTimerActive()) return false; and removing if (elapsedTime < elapsedTimeLimit) return false; I believe the check of isVisuallyNonEmpty() is harmful to Competitive PLT (it seems to say "If I have enough content to render, then don't!"). So maybe excluding that check would improve this patch.
(In reply to Geoffrey Garen from comment #13) > I'd be interested in an A/B test of just adding > > if (!document->isLayoutTimerActive()) > return false; > > In theory, this might improve Competitive PLT by reducing parser yielding > when we have nothing to render. But it might harm PLT5's domContentLoaded > metric. > > > I'd also be interested in an A/B test of adding > > if (!document->isLayoutTimerActive()) > return false; > > and removing > > if (elapsedTime < elapsedTimeLimit) > return false; > > I believe the check of isVisuallyNonEmpty() is harmful to Competitive PLT > (it seems to say "If I have enough content to render, then don't!"). So > maybe excluding that check would improve this patch. Thanks, these are great suggestions :) I will be creating A/B tests for this.
Created attachment 435271 [details] Patch
Created attachment 435793 [details] Patch
Pull request: https://github.com/WebKit/WebKit/pull/9394
Committed 260011@main (6ce95a8a6fc3): <https://commits.webkit.org/260011@main> Reviewed commits have been landed. Closing PR #9394 and removing active labels.
Re-opened since this is blocked by bug 251943
Reverted in https://commits.webkit.org/260032@main because it appeared to cause macOS WK1 tests to exit early with timeouts: https://build.webkit.org/results/Apple-Ventura-Release-AppleSilicon-WK1-Tests/260011@main%20(1701)/results.html
This may have also caused the flakiness seen with fast/text/web-font-load-invisible-during-loading.html https://results.webkit.org/?suite=layout-tests&test=fast%2Ftext%2Fweb-font-load-invisible-during-loading.html https://build.webkit.org/results/Apple-Ventura-Release-AppleSilicon-WK2-Tests/260026@main%20(1521)/fast/text/web-font-load-invisible-during-loading-diff.txt
..and these failures on iOS Simulator: https://results.webkit.org/?suite=layout-tests&suite=layout-tests&suite=layout-tests&test=imported%2Fw3c%2Fweb-platform-tests%2Fimport-maps%2Fbare-specifiers.sub.html&test=imported%2Fw3c%2Fweb-platform-tests%2Fimport-maps%2Fdata-url-specifiers.sub.html&test=imported%2Fw3c%2Fweb-platform-tests%2Fimport-maps%2Fhttp-url-like-specifiers.sub.html&platform=ios
Pull request: https://github.com/WebKit/WebKit/pull/9925
Committed 261705@main (f17bb5ab82ff): <https://commits.webkit.org/261705@main> Reviewed commits have been landed. Closing PR #9925 and removing active labels.
Comment on attachment 435793 [details] Patch Clearing review flag, as the bug is resolved now.