Bug 181395 – Do not go to the storage process when loading a main resource if there is no service worker registered

• Component: Service Workers
• Status: RESOLVED FIXED

[youenn fablet]

UIProcess may know whether there is a registered service worker and instructs the web process accordingly.
This may help speeding up the load of the first page in the web process.
To ensure that service worker works fine, the data provided by the UIProcess should only be useful until the storage process instructs the web process precisely.

[youenn fablet]

Created attachment 330724 [details]
Patch

[youenn fablet]

rdar://problem/36163728

[youenn fablet]

This patch is not fixing the issue of triggering launch of the storage process even in the case of no service worker.
With this patch, storage process will be spinned at the time the first HTTP response bytes are received and not before sending the first HTTP request.

I tried with LaunchPerf to check the difference for loading https://mdn.github.io/sw-test/.
I did not see any big difference with or without service worker registered.
It might be that the time spent to load a regular non localhost HTTP request is dictating the results.

[youenn fablet]

Comment on attachment 330724 [details]
Patch

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

> Source/WebKit/UIProcess/ServiceWorkerProcessProxy.cpp:58
> +    return WebCore::FileSystem::fileExists(store.resolvedServiceWorkerRegistrationDirectory());

I guess we should try to check for the actual ServiceWorkerRegistrations.sqlite3 file...

[youenn fablet]

Created attachment 331327 [details]
Patch

[Chris Dumez]

Comment on attachment 331327 [details]
Patch

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

> Source/WebKit/ChangeLog:10
> +        The connection to the storage process is then executed when receiving the first bytes of the main resource.

Does this mean that WebViews that never register service workers will end up with a storage process no matter what? I thought we wanted to avoid this.

> Source/WebCore/workers/service/ServiceWorkerProvider.h:47
> +    virtual SWClientConnection* serviceWorkerConnectionForSessionIfAny(PAL::SessionID) = 0;

I believe we usually call such methods something like existingServiceWorkerConnectionForSession()

> Source/WebCore/workers/service/ServiceWorkerProvider.h:53
> +    bool m_hasSomeServiceWorkersRegistered { true };

m_hasServiceWorkersRegistered ? "Some" seems weird here. Ditto in the method above.

> Source/WebKit/UIProcess/WebProcessPool.cpp:815
> +#if ENABLE(SERVICE_CONTROLS)

Did you mean SERVICE_WORKERS?

> Source/WebKit/WebProcess/Storage/WebServiceWorkerProvider.cpp:62
> +WebCore::SWClientConnection* WebServiceWorkerProvider::serviceWorkerConnectionForSessionIfAny(SessionID sessionID)

existingServiceWorkerConnectionForSession()

[youenn fablet]

Thanks for the feedback.

(In reply to Chris Dumez from comment #6)
> Comment on attachment 331327 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=331327&action=review
> 
> > Source/WebKit/ChangeLog:10
> > +        The connection to the storage process is then executed when receiving the first bytes of the main resource.
> 
> Does this mean that WebViews that never register service workers will end up
> with a storage process no matter what? I thought we wanted to avoid this.

Yes, but this requires a change which is to make async the creation of the connection.
This is a bigger change than this patch and should be tackled as a follow-up

I'll fix your other comments as a follow-up or at landing time.

[Chris Dumez]

(In reply to youenn fablet from comment #7)
> Thanks for the feedback.
> 
> (In reply to Chris Dumez from comment #6)
> > Comment on attachment 331327 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=331327&action=review
> > 
> > > Source/WebKit/ChangeLog:10
> > > +        The connection to the storage process is then executed when receiving the first bytes of the main resource.
> > 
> > Does this mean that WebViews that never register service workers will end up
> > with a storage process no matter what? I thought we wanted to avoid this.
> 
> Yes, but this requires a change which is to make async the creation of the
> connection.

Could you clarify why we need to make async the creation of the connection?

> This is a bigger change than this patch and should be tackled as a follow-up
> 
> I'll fix your other comments as a follow-up or at landing time.

What's the benefit of taking the patch in the current state? Looks like it merely delays the StorageProcess launch from "right before the load" to "as soon as we receive the first bytes of the main resource".
If so, there would be no impact on performance / memory use, right?

[youenn fablet]

Brady mentioned the possibility to do async connections so that we do not have to wait for the connection to be opened to register service worker clients.

To be fully optimal, we might want to not even register service worker clients if there is no need.

Let's say there is no service worker that is registered.
The storage process then does not need to be made aware of service worker clients for each web process.

Let's say now that one web process registers a service worker.
It will create the connection and probably register all its clients.

At that time, the other web processes also need to register all their clients.
We can implement that for instance by going through the UIProcess to broadcast to all web processes that they should register their clients if they have not done so.

[youenn fablet]

> What's the benefit of taking the patch in the current state? Looks like it
> merely delays the StorageProcess launch from "right before the load" to "as
> soon as we receive the first bytes of the main resource".
> If so, there would be no impact on performance / memory use, right?

Before the patch, we wait to send the HTTP request after spinning the storage process.
With the patch, we send the HTTP request as soon as we can, and we spin the storage process while receiving the response in parallel. So there are some loading time gains.

The second reason is that we have two places that might spin the storage process for no good reason. This patch fixes one such place. A follow-up patch will fix the second place.

[youenn fablet]

Filed bug 181740 for the follow-up

[youenn fablet]

Created attachment 331495 [details]
Patch

[youenn fablet]

(In reply to youenn fablet from comment #10)
> > What's the benefit of taking the patch in the current state? Looks like it
> > merely delays the StorageProcess launch from "right before the load" to "as
> > soon as we receive the first bytes of the main resource".
> > If so, there would be no impact on performance / memory use, right?
> 
> Before the patch, we wait to send the HTTP request after spinning the
> storage process.
> With the patch, we send the HTTP request as soon as we can, and we spin the
> storage process while receiving the response in parallel. So there are some
> loading time gains.

To be noted also that for Safari, the storage process is started when Safari starts, not when safari loads the first HTTP page.
By postponing the creation of the web process to storage process connection, this increases the chances that the storage process will not be busy when asked to create the connection.

[WebKit Commit Bot]

Comment on attachment 331495 [details]
Patch

Clearing flags on attachment: 331495

Committed r227161: <https://trac.webkit.org/changeset/227161>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.