Bug 201155 - Regression: ITP started doing a lot more IPC after its logic was moved to the network process
Summary: Regression: ITP started doing a lot more IPC after its logic was moved to the...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-08-26 15:23 PDT by Chris Dumez
Modified: 2019-08-26 17:21 PDT (History)
9 users (show)

See Also:


Attachments
Patch (28.92 KB, patch)
2019-08-26 15:48 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (29.30 KB, patch)
2019-08-26 16:36 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2019-08-26 15:23:03 PDT
ITP does a lot more IPC that it used to since its logic was moved to the network process. Web processes used to send their statistics to the UIProcess at most every 5 seconds. However, when the logic got moved to the network process, we started notifying the network process via IPC after every sub resource load. This is bad for performance and battery life.
Comment 1 John Wilander 2019-08-26 15:32:55 PDT
Thanks for taking a stab at this, Chris!
Comment 2 Chris Dumez 2019-08-26 15:48:51 PDT
Created attachment 377278 [details]
Patch
Comment 3 John Wilander 2019-08-26 16:23:24 PDT
Comment on attachment 377278 [details]
Patch

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

Looks good to me. See minor comments.

> Source/WebCore/ChangeLog:3
> +        Regression: ITP does a lot more IPC that it used to since its logic was moved to the network process

I think "after its logic was moved" makes it more clear. Otherwise, some may interpret that the move itself causes more IPC.

> Source/WebCore/ChangeLog:8
> +        ITP does a lot more IPC that it used to since its logic was moved to the network process. Web processes used to

since -> after

> Source/WebKit/ChangeLog:3
> +        Regression: ITP does a lot more IPC that it used to since its logic was moved to the network process

since –> after

> Source/WebCore/loader/ResourceLoadObserver.cpp:125
> +        scheduleNotificationIfNeeded();

We used to have a local boolean that captured whether this call to the log function had caused a statistics update or not, and only schedule a notification if so. But the generalized check for the existence of any batched up statistics is fine too. As long as m_perSessionResourceStatisticsMap.isEmpty() is performant which I assume it is.

> Source/WebKit/WebProcess/WebProcess.cpp:228
> +    ResourceLoadObserver::shared().setLogUserInteractionNotificationCallback([this] (PAL::SessionID sessionID, const RegistrableDomain& domain) {    ensureNetworkProcessConnection().connection().send(Messages::NetworkConnectionToWebProcess::LogUserInteraction(sessionID, domain), 0);

I believe we used to have a code comment explaining why this particular IPC call was done directly so that others understand why.
Comment 4 Chris Dumez 2019-08-26 16:27:50 PDT
(In reply to John Wilander from comment #3)
> Comment on attachment 377278 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=377278&action=review
> 
> Looks good to me. See minor comments.
> 
> > Source/WebCore/ChangeLog:3
> > +        Regression: ITP does a lot more IPC that it used to since its logic was moved to the network process
> 
> I think "after its logic was moved" makes it more clear. Otherwise, some may
> interpret that the move itself causes more IPC.
> 
> > Source/WebCore/ChangeLog:8
> > +        ITP does a lot more IPC that it used to since its logic was moved to the network process. Web processes used to
> 
> since -> after
> 
> > Source/WebKit/ChangeLog:3
> > +        Regression: ITP does a lot more IPC that it used to since its logic was moved to the network process
> 
> since –> after
> 
> > Source/WebCore/loader/ResourceLoadObserver.cpp:125
> > +        scheduleNotificationIfNeeded();
> 
> We used to have a local boolean that captured whether this call to the log
> function had caused a statistics update or not, and only schedule a
> notification if so. But the generalized check for the existence of any
> batched up statistics is fine too. As long as
> m_perSessionResourceStatisticsMap.isEmpty() is performant which I assume it
> is.
 
I intentionally killed that boolean. Scheduling a timer is a no-op the second time since that timer has already been started.

> 
> > Source/WebKit/WebProcess/WebProcess.cpp:228
> > +    ResourceLoadObserver::shared().setLogUserInteractionNotificationCallback([this] (PAL::SessionID sessionID, const RegistrableDomain& domain) {    ensureNetworkProcessConnection().connection().send(Messages::NetworkConnectionToWebProcess::LogUserInteraction(sessionID, domain), 0);
> 
> I believe we used to have a code comment explaining why this particular IPC
> call was done directly so that others understand why.

I did not see one but I will add one.
Comment 5 Chris Dumez 2019-08-26 16:36:20 PDT
Created attachment 377287 [details]
Patch
Comment 6 WebKit Commit Bot 2019-08-26 17:20:38 PDT
Comment on attachment 377287 [details]
Patch

Clearing flags on attachment: 377287

Committed r249126: <https://trac.webkit.org/changeset/249126>
Comment 7 WebKit Commit Bot 2019-08-26 17:20:40 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 Radar WebKit Bug Importer 2019-08-26 17:21:18 PDT
<rdar://problem/54729534>