1577829 - Add timing measurements for Service Workers e10s

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect, P3)

Description

[Perry Jiang [:perry] [no longer employee, use ni?]]

Specifically, the Service Workers-related measurements here https://searchfox.org/mozilla-central/rev/c8dba8c886609d0fdb8fae68bab865bf03f1165c/netwerk/base/nsITimedChannel.idl#97.

dom/serviceworkers/test/browser_devtools_serviceworker_interception.js will need to be modified to check for these measurements in parent-intercept mode (i.e. pref dom.serviceWorkers.parent_intercept=true).

Comment 2

[Hubert Boma Manilla (:bomsy)]

It'll be useful to expose some of the timings https://searchfox.org/mozilla-central/rev/fe8b30e982c86d26ccf1f14d825c0de870b91f27/netwerk/protocol/http/InterceptedHttpChannel.h#93-184 . These are going to be useful calculating and displaying Service worker timings in devtools

Comment 3

[Hubert Boma Manilla (:bomsy)]

Attachment: Bug 1577829 - Expose service worker timings in the parent process r=edenchuang

Comment 4

[Eden Chuang[:edenchuang]]

As mentioned in the review comment, we will expose ServiceWorker launch corresponding timing information in the P2.

Although ServiceWorkerPrivate saves the ServiceWorkerLaunchStart time stamp, ServiceWorkerPrivate does not save the "LaunchEnd" time stamp.
ServiceWorkerPrivate only has one chance to set up launching timing information to InterceptedHttpChannel when ServiceWorkerPrivate::SendFethcEvent() because ServiceWorkerPrivate(and its interface) is not exposed to the FetchEvent codes after FetchEvent initialiation in ServiceWorkerPrivate::SendFetchEvent(). However, ServiceWorker might still be in the launching process at the time, so ServiceWorkerPrivate might have no "LaunchEnd" timing to set up in the InterceptedHttpChannel.

After looking at the current codes, I think the best place to save the ServiceWorker launch timing would be in RemoteWorkerController. Since ServiceWorkerPrivate uses RemoteWorkerController to set up the ServiceWorker in the remote process. RemoteWorkerController is the actual object that knows the launching start and end. In addition to dispatching the FetchEvent to the remote process, RemoteWorkerController is exposed to FetchEvent to handle the event dispatching and response receiving.

So I think we need to do the following in the P2. It can be separated into two parts.

The first part is saving and exposing timing information in RemoteWorkerControllerChild.

1. Declare TimeStamp mRemoteWorkerLaunchStart/End and getter methods TimeStamp GetRemoteWorkerLaunchStart()/End() in RemoteWorkerControllerChild.h
2. Save mRemoteWorkerLaunchStart in RemoteWorkerControllerChild::RemoteWorkerControllerChild()
3. Save mRemoteWorkerLaunchEnd in RemoteWorkerControllerChild::RecvCreationSucceeded() and RemoteWorkerControllerChild::RecvCreationFailed()

The second part is saving timing data in InterceptedHttpChannel.

1. Declare void SetRemoteWorkerLaunchStart(TimeStamp aTimeStamp); and void SetRemoteWorkerLaunchEnd(TimeStamp aTimeStamp); in nsIInterceptedChannel.idl.
2. Declare and define SetRemoteWorkerLaunchStart/SetRemoteWorkerLaunchEnd in InterceptedHttpChannel.h/.cpp. The definition of SetRemoteWorkerLaunchStart/End() should check if Start timeStamp is later than InterceptedHttpChannel::mInterceptedStart or not, only save the TimeStamp when passed in is later than InterceptedHttpChannel::mInterceptedStart. Otherwise, set the Start/End timestamp to mInterceptedStart.
3. Call nsIInterceptedChannel::SetRemoteWorkerLaunchStart() and nsIInterceptedChannel::SetRemoteWorkerLaunchEnd() in FetchEventOpChild::RecvRespondWith() to setup start time and end time with Manager()->GetRemoteWorkerLaunchStart() and Manager()->GetRemoteWorkerLaunchEnd(). To call GetRemoteWorkerLaunchStart/End(), Manager() should be down-casted to RemoteWorkerControllerChild with static_cast<RemoteWorkerControllerChild>

Bomsy, please let me know if anything is unclear. Feel free to reach out to me directly.

Comment 5

[Hubert Boma Manilla (:bomsy)]

Thanks a lot eden.
This is quite clear, I'll start a patch and if i hit anything i'll reach out.

Comment 6

[Hubert Boma Manilla (:bomsy)]

Attachment: Bug 1577829 - Set the service worker launch timings r=edenchuang

Depends on D194065

Comment 7

[Hubert Boma Manilla (:bomsy)]

(In reply to Eden Chuang[:edenchuang] from comment #4)

Thanks alot for the guide on this.

2. Declare and define SetRemoteWorkerLaunchStart/SetRemoteWorkerLaunchEnd in InterceptedHttpChannel.h/.cpp. The definition of SetRemoteWorkerLaunchStart/End() should check if Start timeStamp is later than InterceptedHttpChannel::mInterceptedStart or not, only save the TimeStamp when passed in is later than InterceptedHttpChannel::mInterceptedStart. Otherwise, set the Start/End timestamp to mInterceptedStart.

To clarify my understanding here. Is the goal here to only track the service worker launch start/end only when it launches after the request has been sent. I think in this case its going to impact the total time it'll take the service worker to respond. Is this correct?

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5ea407d40abb
Expose service worker timings in the parent process r=edenchuang,necko-reviewers

Comment 9

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/5ea407d40abb

Comment 10

[Eden Chuang[:edenchuang]]

(In reply to Hubert Boma Manilla (:bomsy) from comment #7)

(In reply to Eden Chuang[:edenchuang] from comment #4)

Thanks alot for the guide on this.

2. Declare and define SetRemoteWorkerLaunchStart/SetRemoteWorkerLaunchEnd in InterceptedHttpChannel.h/.cpp. The definition of SetRemoteWorkerLaunchStart/End() should check if Start timeStamp is later than InterceptedHttpChannel::mInterceptedStart or not, only save the TimeStamp when passed in is later than InterceptedHttpChannel::mInterceptedStart. Otherwise, set the Start/End timestamp to mInterceptedStart.

To clarify my understanding here. Is the goal here to only track the service worker launch start/end only when it launches after the request has been sent. I think in this case its going to impact the total time it'll take the service worker to respond. Is this correct?

Yes, in fact, our FetchEvent dispatching time includes the ServiceWorker Launching time. We just highlight it if ServiceWorker launching happens during the FetchEvent dispatching. So, the FetchEvents should not have ServiceWorker launching time if ServiceWorker has been running already.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0dac9f18451f
Set the service worker launch timings r=edenchuang,necko-reviewers,jesup

Comment 12

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/0dac9f18451f