Bug 198382 - REGRESSION (r245396): Page load time performance regression
Summary: REGRESSION (r245396): Page load time performance regression
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P1 Normal
Assignee: Ali Juma
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-05-30 10:45 PDT by Per Arne Vollan
Modified: 2019-06-10 10:41 PDT (History)
10 users (show)

See Also:


Attachments
Patch (4.78 KB, patch)
2019-05-30 13:03 PDT, Ali Juma
no flags Details | Formatted Diff | Diff
Patch (4.93 KB, patch)
2019-05-30 16:29 PDT, Ali Juma
no flags Details | Formatted Diff | Diff
Patch (1.37 KB, patch)
2019-06-07 14:16 PDT, Ali Juma
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews115 for mac-highsierra (2.96 MB, application/zip)
2019-06-07 16:51 PDT, Build Bot
no flags Details
Patch (1.82 KB, patch)
2019-06-10 08:19 PDT, Ali Juma
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Per Arne Vollan 2019-05-30 10:45:36 PDT
The change in https://trac.webkit.org/r245396 seems to have introduced a regression in page load time.
Comment 1 Ali Juma 2019-05-30 11:02:22 PDT
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.
Comment 2 Per Arne Vollan 2019-05-30 11:08:54 PDT
(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.
Comment 3 Per Arne Vollan 2019-05-30 11:12:44 PDT
rdar://problem/51140254
Comment 4 Ali Juma 2019-05-30 13:03:10 PDT
Created attachment 370968 [details]
Patch
Comment 5 Brent Fulgham 2019-05-30 13:28:53 PDT
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 6 Per Arne Vollan 2019-05-30 13:33:26 PDT
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 7 Simon Fraser (smfr) 2019-05-30 13:41:38 PDT
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).
Comment 8 Ali Juma 2019-05-30 14:25:18 PDT
(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 9 Simon Fraser (smfr) 2019-05-30 14:34:39 PDT
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?
Comment 10 Ali Juma 2019-05-30 16:29:39 PDT
Created attachment 370992 [details]
Patch
Comment 11 Ali Juma 2019-05-30 16:31:01 PDT
(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 12 WebKit Commit Bot 2019-05-31 04:03:20 PDT
Comment on attachment 370992 [details]
Patch

Clearing flags on attachment: 370992

Committed r245958: <https://trac.webkit.org/changeset/245958>
Comment 13 WebKit Commit Bot 2019-05-31 04:03:22 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 Per Arne Vollan 2019-06-06 09:24:11 PDT
Reopened, since no progression was observed after this change.
Comment 15 Per Arne Vollan 2019-06-06 09:25:07 PDT
Perhaps the initial update delay of 500ms is too short?
Comment 16 Ali Juma 2019-06-06 09:40:07 PDT
(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)
Comment 17 Per Arne Vollan 2019-06-06 10:01:32 PDT
(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%.
Comment 18 Ali Juma 2019-06-07 13:56:27 PDT
(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.
Comment 19 Ali Juma 2019-06-07 14:16:59 PDT
Created attachment 371610 [details]
Patch
Comment 20 Per Arne Vollan 2019-06-07 14:23:57 PDT
(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 21 Per Arne Vollan 2019-06-07 14:52:54 PDT
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 22 Build Bot 2019-06-07 16:51:43 PDT
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
Comment 23 Build Bot 2019-06-07 16:51:45 PDT
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
Comment 24 Ali Juma 2019-06-10 08:19:50 PDT
Created attachment 371745 [details]
Patch
Comment 25 Ali Juma 2019-06-10 08:20:25 PDT
(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 26 Per Arne Vollan 2019-06-10 10:05:57 PDT
Comment on attachment 371745 [details]
Patch

R=me.
Comment 27 WebKit Commit Bot 2019-06-10 10:41:38 PDT
Comment on attachment 371745 [details]
Patch

Clearing flags on attachment: 371745

Committed r246267: <https://trac.webkit.org/changeset/246267>
Comment 28 WebKit Commit Bot 2019-06-10 10:41:40 PDT
All reviewed patches have been landed.  Closing bug.