Bug 198382 – REGRESSION (r245396): Page load time performance regression

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

[Per Arne Vollan]

The change in https://trac.webkit.org/r245396 seems to have introduced a regression in page load time.

[Ali Juma]

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.

[Per Arne Vollan]

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

[Per Arne Vollan]

rdar://problem/51140254

[Ali Juma]

Created attachment 370968 [details]
Patch

[Brent Fulgham]

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?

[Per Arne Vollan]

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.

[Simon Fraser (smfr)]

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

[Ali Juma]

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

[Simon Fraser (smfr)]

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?

[Ali Juma]

Created attachment 370992 [details]
Patch

[Ali Juma]

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

[WebKit Commit Bot]

Comment on attachment 370992 [details]
Patch

Clearing flags on attachment: 370992

Committed r245958: <https://trac.webkit.org/changeset/245958>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Per Arne Vollan]

Reopened, since no progression was observed after this change.

[Per Arne Vollan]

Perhaps the initial update delay of 500ms is too short?

[Ali Juma]

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

[Per Arne Vollan]

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

[Ali Juma]

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

[Ali Juma]

Created attachment 371610 [details]
Patch

[Per Arne Vollan]

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

[Per Arne Vollan]

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()?

[EWS Watchlist]

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

[EWS Watchlist]

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

[Ali Juma]

Created attachment 371745 [details]
Patch

[Ali Juma]

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

[Per Arne Vollan]

Comment on attachment 371745 [details]
Patch

R=me.

[WebKit Commit Bot]

Comment on attachment 371745 [details]
Patch

Clearing flags on attachment: 371745

Committed r246267: <https://trac.webkit.org/changeset/246267>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.