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.
Thanks for taking a stab at this, Chris!
Created attachment 377278 [details] Patch
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.
(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.
Created attachment 377287 [details] Patch
Comment on attachment 377287 [details] Patch Clearing flags on attachment: 377287 Committed r249126: <https://trac.webkit.org/changeset/249126>
All reviewed patches have been landed. Closing bug.
<rdar://problem/54729534>