The change in https://trac.webkit.org/r245396 seems to have introduced a regression in page load time.
We could add a delay before scheduling a rendering update -- that should prevent us from adding extra updates during page load, while still ensuring that if nothing else triggers an update, we do eventually schedule one.
(In reply to Ali Juma from comment #1) > We could add a delay before scheduling a rendering update -- that should > prevent us from adding extra updates during page load, while still ensuring > that if nothing else triggers an update, we do eventually schedule one. Sounds good! I think this would resolve the regression.
rdar://problem/51140254
Created attachment 370968 [details] Patch
Comment on attachment 370968 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=370968&action=review > Source/WebCore/page/IntersectionObserver.cpp:161 > + document->scheduleInitialIntersectionObservationUpdate(); It seems like ResizeObserver::observe has a similar pattern -- I wonder why that isn't a page load time issue (perhaps it is, and it existed prior to our baseline?). Perhaps we should apply this pattern everywhere an observer is added?
Comment on attachment 370968 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=370968&action=review >> Source/WebCore/page/IntersectionObserver.cpp:161 >> + document->scheduleInitialIntersectionObservationUpdate(); > > It seems like ResizeObserver::observe has a similar pattern -- I wonder why that isn't a page load time issue (perhaps it is, and it existed prior to our baseline?). Perhaps we should apply this pattern everywhere an observer is added? That's a good point. I think this would be well worth testing, although it can be done in a separate patch.
Comment on attachment 370968 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=370968&action=review > Source/WebCore/dom/Document.cpp:336 > +#if ENABLE(INTERSECTION_OBSERVER) > +static const Seconds intersectionObserversInitialUpdateDelay { 500_ms }; > +#endif Rather than IntersectionObserver having its own delay, we should piggy-back off the existing painting-delayed-during-page-load mechanisms (layer tree freezing).
(In reply to Simon Fraser (smfr) from comment #7) > Comment on attachment 370968 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=370968&action=review > > > Source/WebCore/dom/Document.cpp:336 > > +#if ENABLE(INTERSECTION_OBSERVER) > > +static const Seconds intersectionObserversInitialUpdateDelay { 500_ms }; > > +#endif > > Rather than IntersectionObserver having its own delay, we should piggy-back > off the existing painting-delayed-during-page-load mechanisms (layer tree > freezing). The current code already respects layer tree freezing -- Document::scheduleRenderingUpdate schedules a compositing layer flush, and both RemoteLayerTreeDrawingArea::scheduleCompositingLayerFlush and TiledCoreAnimationDrawingArea::scheduleCompositingLayerFlush defer flushes while the layer tree is frozen. The actual intersection observation update happens at flush time, so won't happen until after the layer tree is unfrozen. The regression here seems to be from additional flushes happening after the layer tree has been unfrozen but before page load has finished.
Comment on attachment 370968 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=370968&action=review >>> Source/WebCore/dom/Document.cpp:336 >>> +#endif >> >> Rather than IntersectionObserver having its own delay, we should piggy-back off the existing painting-delayed-during-page-load mechanisms (layer tree freezing). > > The current code already respects layer tree freezing -- Document::scheduleRenderingUpdate schedules a compositing layer flush, and both RemoteLayerTreeDrawingArea::scheduleCompositingLayerFlush and TiledCoreAnimationDrawingArea::scheduleCompositingLayerFlush defer flushes while the layer tree is frozen. The actual intersection observation update happens at flush time, so won't happen until after the layer tree is unfrozen. > > The regression here seems to be from additional flushes happening after the layer tree has been unfrozen but before page load has finished. This code will delay the first IO callback by 500ms whether or not not happens near the beginning of page load, which seems non-ideal. Can we only add the 500ms if we think we're still loading? Maybe we can look at whether paint milestones have fired?
Created attachment 370992 [details] Patch
(In reply to Simon Fraser (smfr) from comment #9) > > This code will delay the first IO callback by 500ms whether or not not > happens near the beginning of page load, which seems non-ideal. Can we only > add the 500ms if we think we're still loading? Maybe we can look at whether > paint milestones have fired? Good idea, changed to only delay during page load.
Comment on attachment 370992 [details] Patch Clearing flags on attachment: 370992 Committed r245958: <https://trac.webkit.org/changeset/245958>
All reviewed patches have been landed. Closing bug.
Reopened, since no progression was observed after this change.
Perhaps the initial update delay of 500ms is too short?
(In reply to Per Arne Vollan from comment #15) > Perhaps the initial update delay of 500ms is too short? We could certainly try a longer delay. But a couple questions: 1) Are we certain that r245396 caused the original regression? 2) Would it be possible to share a particular URL where page load time regressed, so I can try to reproduce and debug/test locally? (to avoid having to guess how much of a delay we need)
(In reply to Ali Juma from comment #16) > (In reply to Per Arne Vollan from comment #15) > > Perhaps the initial update delay of 500ms is too short? > > We could certainly try a longer delay. But a couple questions: > 1) Are we certain that r245396 caused the original regression? Yes, we are pretty confident in this. > 2) Would it be possible to share a particular URL where page load time > regressed, so I can try to reproduce and debug/test locally? (to avoid > having to guess how much of a delay we need) Yes, we see that google.com is regressed with ~10%.
(In reply to Per Arne Vollan from comment #17) > (In reply to Ali Juma from comment #16) > > (In reply to Per Arne Vollan from comment #15) > > > Perhaps the initial update delay of 500ms is too short? > > > > We could certainly try a longer delay. But a couple questions: > > 1) Are we certain that r245396 caused the original regression? > > Yes, we are pretty confident in this. > > > 2) Would it be possible to share a particular URL where page load time > > regressed, so I can try to reproduce and debug/test locally? (to avoid > > having to guess how much of a delay we need) > > Yes, we see that google.com is regressed with ~10%. Interestingly, when I load google.com on iOS, I don't see any calls to IntersectionObserver::observe until I do a search and then start scrolling down the search result page. In other words, the logic added in r245396 isn't triggered at all during page load. On cnn.com, I do see several calls to IntersectionObserver::observe during page load, so increasing the delay might help page load time there.
Created attachment 371610 [details] Patch
(In reply to Ali Juma from comment #18) > (In reply to Per Arne Vollan from comment #17) > > (In reply to Ali Juma from comment #16) > > > (In reply to Per Arne Vollan from comment #15) > > > > Perhaps the initial update delay of 500ms is too short? > > > > > > We could certainly try a longer delay. But a couple questions: > > > 1) Are we certain that r245396 caused the original regression? > > > > Yes, we are pretty confident in this. > > > > > 2) Would it be possible to share a particular URL where page load time > > > regressed, so I can try to reproduce and debug/test locally? (to avoid > > > having to guess how much of a delay we need) > > > > Yes, we see that google.com is regressed with ~10%. > > Interestingly, when I load google.com on iOS, I don't see any calls to > IntersectionObserver::observe until I do a search and then start scrolling > down the search result page. In other words, the logic added in r245396 > isn't triggered at all during page load. > Sorry, I was incorrect in saying the google home page regressed. As you say, we observed the regression when loading a google.com url with a search term. > On cnn.com, I do see several calls to IntersectionObserver::observe during > page load, so increasing the delay might help page load time there.
Comment on attachment 371610 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=371610&action=review > Source/WebCore/dom/Document.cpp:357 > #if ENABLE(INTERSECTION_OBSERVER) > -static const Seconds intersectionObserversInitialUpdateDelay { 500_ms }; > +static const Seconds intersectionObserversInitialUpdateDelay { 2000_ms }; > #endif Should we stop the update timer in Document::scheduleRenderingUpdate()?
Comment on attachment 371610 [details] Patch Attachment 371610 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/12413135 New failing tests: webgl/2.0.0/conformance/textures/misc/origin-clean-conformance.html
Created attachment 371631 [details] Archive of layout-test-results from ews115 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews115 Port: mac-highsierra Platform: Mac OS X 10.13.6
Created attachment 371745 [details] Patch
(In reply to Per Arne Vollan from comment #21) > Comment on attachment 371610 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=371610&action=review > > > Source/WebCore/dom/Document.cpp:357 > > #if ENABLE(INTERSECTION_OBSERVER) > > -static const Seconds intersectionObserversInitialUpdateDelay { 500_ms }; > > +static const Seconds intersectionObserversInitialUpdateDelay { 2000_ms }; > > #endif > > Should we stop the update timer in Document::scheduleRenderingUpdate()? Done.
Comment on attachment 371745 [details] Patch R=me.
Comment on attachment 371745 [details] Patch Clearing flags on attachment: 371745 Committed r246267: <https://trac.webkit.org/changeset/246267>