Bug 208168 – Remove render update throttling

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Ben Nham]

Created attachment 391600 [details]
loading apple.com/mac PLT content with render throttling on/off

Currently, we disable render updates after the first paint for 500 ms while the page is actively loading. However, oftentimes our first paint heuristic selects a first paint that isn't particularly interesting (mostly background colors) and this paint throttler just makes the user look at a nearly empty page for 500 ms.

Antti tested removing this and found that it was a ~0.5% PLT regression on the Mac. This is because PLT only measures first paint, DOMContentLoaded, and page load done, and page load done regressed for some sites that do a lot of layouts and repaints while the page loads. However, we've committed a lot of PLT wins this cycle and I think we should spend some of that budget on removing render throttling since it's a better user experience to see a page consistently render new content while loading.

I've attached a screen recording of loading apple.com with render throttling on and off. On the left side, render throttling is enabled, and the user has to stare at a nearly empty first paint for 500 ms. On the right side, render throttling is disabled, and we render interesting content much quicker.

[Radar WebKit Bug Importer]

<rdar://problem/59746550>

[zalan]

I like how the progressive rendering reveals those content animations.

[Ben Nham]

Created attachment 391617 [details]
Patch

[Ben Nham]

Comment on attachment 391617 [details]
Patch

This patch only removes throttling from the Mac since we don't have A/B test results from iOS yet and it'll take a day or more to get those results.

[zalan]

(In reply to Ben Nham from comment #4)
> Comment on attachment 391617 [details]
> Patch
> 
> This patch only removes throttling from the Mac since we don't have A/B test
> results from iOS yet and it'll take a day or more to get those results.
Is there a reason to rush a Mac only patch in? I'd rather have this throttling completely removed in one go (in theory we should have the same type of rendering updates on both platforms, at least I don't see why macOS would be any special.)

[Antti Koivisto]

Comment on attachment 391617 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=391617&action=review

> Source/WebKit/ChangeLog:3
> +        Remove render update throttling

Should mention this Mac only for now

[Ben Nham]

Created attachment 391677 [details]
Patch

[Ben Nham]

Comment on attachment 391677 [details]
Patch

Re-submitting for review now that I removed throttling from iOS as well.

[Ben Nham]

Created attachment 391679 [details]
Patch

[Ben Nham]

Comment on attachment 391679 [details]
Patch

Re-requesting review after fixing WPE build.

[zalan]

Comment on attachment 391679 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=391679&action=review

> Source/WebKit/WebProcess/WebPage/RemoteLayerTree/RemoteLayerTreeDrawingArea.mm:197
> +        scheduleRenderingUpdate();

shouldn't the initial paint always be immediate (scheduleImmediateRenderingUpdate())? I wonder if m_updateRenderingTimer is ever active in this case and if it is whether overriding it with a 0ms timeout is actually better.

[Ben Nham]

Comment on attachment 391679 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=391679&action=review

>> Source/WebKit/WebProcess/WebPage/RemoteLayerTree/RemoteLayerTreeDrawingArea.mm:197
>> +        scheduleRenderingUpdate();
> 
> shouldn't the initial paint always be immediate (scheduleImmediateRenderingUpdate())? I wonder if m_updateRenderingTimer is ever active in this case and if it is whether overriding it with a 0ms timeout is actually better.

Hmm good point, looks like the rendering timer shouldn't be active if the layer tree is frozen, so we can just call scheduleImmediateRenderingUpdate directly. I'll change that in the final patch.

[zalan]

(In reply to Ben Nham from comment #12)
> Comment on attachment 391679 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=391679&action=review
> 
> >> Source/WebKit/WebProcess/WebPage/RemoteLayerTree/RemoteLayerTreeDrawingArea.mm:197
> >> +        scheduleRenderingUpdate();
> > 
> > shouldn't the initial paint always be immediate (scheduleImmediateRenderingUpdate())? I wonder if m_updateRenderingTimer is ever active in this case and if it is whether overriding it with a 0ms timeout is actually better.
> 
> Hmm good point, looks like the rendering timer shouldn't be active if the
> layer tree is frozen, so we can just call scheduleImmediateRenderingUpdate
> directly. I'll change that in the final patch.
great!

[Ben Nham]

Created attachment 391700 [details]
Patch

[Ben Nham]

Comment on attachment 391700 [details]
Patch

 - Call scheduleImmediateRenderingUpdate when layer tree is unfrozen on iOS
 - Request cq

[Ben Nham]

Created attachment 391701 [details]
Patch

[EWS]

Comment on attachment 391701 [details]
Patch

Rejecting attachment 391701 [details] from commit-queue.

[e-mail] does not have committer permissions according to https://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[Simon Fraser (smfr)]

Comment on attachment 391701 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=391701&action=review

> Source/WebKit/WebProcess/WebPage/RemoteLayerTree/RemoteLayerTreeDrawingArea.mm:197
> +        scheduleImmediateRenderingUpdate();

"immediate" means "don't wait for the 16ms display timer". Is that what we want here?

[WebKit Commit Bot]

Comment on attachment 391701 [details]
Patch

Clearing flags on attachment: 391701

Committed r257394: <https://trac.webkit.org/changeset/257394>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Jason Lawrence]

This issue appears to have been caused by that patch.
https://bugs.webkit.org/show_bug.cgi?id=208272