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.
<rdar://problem/59746550>
I like how the progressive rendering reveals those content animations.
Created attachment 391617 [details] Patch
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.
(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.)
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
Created attachment 391677 [details] Patch
Comment on attachment 391677 [details] Patch Re-submitting for review now that I removed throttling from iOS as well.
Created attachment 391679 [details] Patch
Comment on attachment 391679 [details] Patch Re-requesting review after fixing WPE build.
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.
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.
(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!
Created attachment 391700 [details] Patch
Comment on attachment 391700 [details] Patch - Call scheduleImmediateRenderingUpdate when layer tree is unfrozen on iOS - Request cq
Created attachment 391701 [details] Patch
Comment on attachment 391701 [details] Patch Rejecting attachment 391701 [details] from commit-queue. nham@apple.com 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.
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?
Comment on attachment 391701 [details] Patch Clearing flags on attachment: 391701 Committed r257394: <https://trac.webkit.org/changeset/257394>
All reviewed patches have been landed. Closing bug.
This issue appears to have been caused by that patch. https://bugs.webkit.org/show_bug.cgi?id=208272