Bug 228780 – Tweak HTML parser yielding

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Per Arne Vollan]

Tweak html parser yielding to attempt to improve page load performance with respect to certain page load metrics.

[Radar WebKit Bug Importer]

<rdar://problem/81517847>

[Per Arne Vollan]

Created attachment 434910 [details]
Patch

[Brent Fulgham]

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.

[Geoffrey Garen]

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.

[Geoffrey Garen]

Also: Please see my comment in Radar.

[alan]

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.

[alan]

>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.

[Per Arne Vollan]

(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!

[Per Arne Vollan]

(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!

[Per Arne Vollan]

(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!

[Antti Koivisto]

Comment on attachment 434910 [details]
Patch

ASi comp PLT is showing 2.5% regression from this patch. No data from intel bots.

[Per Arne Vollan]

(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!

[Geoffrey Garen]

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.

[Per Arne Vollan]

(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.

[Per Arne Vollan]

Created attachment 435271 [details]
Patch

[Per Arne Vollan]

Created attachment 435793 [details]
Patch

[Per Arne Vollan]

Pull request: https://github.com/WebKit/WebKit/pull/9394

[EWS]

Committed 260011@main (6ce95a8a6fc3): <https://commits.webkit.org/260011@main>

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

[WebKit Commit Bot]

Re-opened since this is blocked by bug 251943

[Ryan Haddad]

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

[Ryan Haddad]

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

[Ryan Haddad]

..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

[Per Arne Vollan]

Pull request: https://github.com/WebKit/WebKit/pull/9925

[EWS]

Committed 261705@main (f17bb5ab82ff): <https://commits.webkit.org/261705@main>

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

[Alexey Proskuryakov]

Comment on attachment 435793 [details]
Patch

Clearing review flag, as the bug is resolved now.