Bug 181395 - Do not go to the storage process when loading a main resource if there is no service worker registered
Summary: Do not go to the storage process when loading a main resource if there is no ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Service Workers (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-01-08 12:35 PST by youenn fablet
Modified: 2019-10-16 15:02 PDT (History)
9 users (show)

See Also:


Attachments
Patch (13.82 KB, patch)
2018-01-08 12:41 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (15.77 KB, patch)
2018-01-15 04:33 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (15.88 KB, patch)
2018-01-17 08:18 PST, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2018-01-08 12:35:30 PST
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.
Comment 1 youenn fablet 2018-01-08 12:41:37 PST
Created attachment 330724 [details]
Patch
Comment 2 youenn fablet 2018-01-09 07:08:54 PST
rdar://problem/36163728
Comment 3 youenn fablet 2018-01-09 10:28:14 PST
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.
Comment 4 youenn fablet 2018-01-09 11:08:59 PST
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...
Comment 5 youenn fablet 2018-01-15 04:33:52 PST
Created attachment 331327 [details]
Patch
Comment 6 Chris Dumez 2018-01-16 08:55:21 PST
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()
Comment 7 youenn fablet 2018-01-16 10:50:03 PST
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.
Comment 8 Chris Dumez 2018-01-16 10:55:55 PST
(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?
Comment 9 youenn fablet 2018-01-16 11:05:45 PST
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.
Comment 10 youenn fablet 2018-01-17 02:00:08 PST
> 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.
Comment 11 youenn fablet 2018-01-17 06:28:12 PST
Filed bug 181740 for the follow-up
Comment 12 youenn fablet 2018-01-17 08:18:22 PST
Created attachment 331495 [details]
Patch
Comment 13 youenn fablet 2018-01-18 00:17:52 PST
(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.
Comment 14 WebKit Commit Bot 2018-01-18 13:13:00 PST
Comment on attachment 331495 [details]
Patch

Clearing flags on attachment: 331495

Committed r227161: <https://trac.webkit.org/changeset/227161>
Comment 15 WebKit Commit Bot 2018-01-18 13:13:01 PST
All reviewed patches have been landed.  Closing bug.