Bug 201155 – Regression: ITP started doing a lot more IPC after its logic was moved to the network process

• Component: WebKit2
• Status: RESOLVED FIXED

[Chris Dumez]

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.

[John Wilander]

Thanks for taking a stab at this, Chris!

[Chris Dumez]

Created attachment 377278 [details]
Patch

[John Wilander]

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.

[Chris Dumez]

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

[Chris Dumez]

Created attachment 377287 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 377287 [details]
Patch

Clearing flags on attachment: 377287

Committed r249126: <https://trac.webkit.org/changeset/249126>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/54729534>