Continue to shrink storage process.
Does this mean we're going to run JavaScript in the Network Process?
Created attachment 350498 [details] Patch
Created attachment 350529 [details] Patch
Will service worker JavaScript code also be running in the Networking process? Separate to that, adding more and more responsibilities to the Networking process is a dangerous approach. More code is more crashes, and a crash in Networking has particularly unpleasant user symptoms.
(In reply to Alexey Proskuryakov from comment #4) > Will service worker JavaScript code also be running in the Networking > process? No, this is just about moving code running in storage process to network process.
(In reply to youenn fablet from comment #5) > (In reply to Alexey Proskuryakov from comment #4) > > Will service worker JavaScript code also be running in the Networking > > process? > > No, this is just about moving code running in storage process to network > process. Gotcha. What code already runs in the storage process?
(In reply to Saam Barati from comment #6) > (In reply to youenn fablet from comment #5) > > (In reply to Alexey Proskuryakov from comment #4) > > > Will service worker JavaScript code also be running in the Networking > > > process? > > > > No, this is just about moving code running in storage process to network > > process. > > Gotcha. What code already runs in the storage process? Storage of service worker registrations (including scripts), handling of service worker instance lifetime and passing events between service worker instances and service worker clients.
(In reply to youenn fablet from comment #7) > (In reply to Saam Barati from comment #6) > > (In reply to youenn fablet from comment #5) > > > (In reply to Alexey Proskuryakov from comment #4) > > > > Will service worker JavaScript code also be running in the Networking > > > > process? > > > > > > No, this is just about moving code running in storage process to network > > > process. > > > > Gotcha. What code already runs in the storage process? > > Storage of service worker registrations (including scripts), handling of > service worker instance lifetime and passing events between service worker > instances and service worker clients. Makes sense. It feels like if this is what’s happening this bug is slightly misnamed. It should probably specify something about the storage process
Created attachment 350605 [details] Patch
Created attachment 350609 [details] Patch
Created attachment 350636 [details] Patch
Some build failures.
Comment on attachment 350636 [details] Patch Some nits. View in context: https://bugs.webkit.org/attachment.cgi?id=350636&action=review > Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:65 > +#if ENABLE(SERVICE_WORKER) Do we need this #if? > Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:758 > +void NetworkConnectionToWebProcess::establishSWServerConnection(PAL::SessionID sessionID, SWServerConnectionIdentifier& serverConnectionIdentifier) I think we now go with SWServerConnectionIdentifier and not SWServerConnectionIdentifier&. > Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.h:193 > + HashMap<WebCore::SWServerConnectionIdentifier, WeakPtr<WebSWServerConnection>> m_swConnections; We usually do not mix members and methods. > Source/WebKit/NetworkProcess/NetworkProcess.cpp:1235 > + auto result = m_swServers.add(sessionID, nullptr); Can we try to use HashMap::ensure? > Source/WebKit/NetworkProcess/NetworkProcess.cpp:1372 > +void NetworkProcess::addServiceWorkerSession(PAL::SessionID sessionID, String& serviceWorkerRegistrationDirectory, SandboxExtension::Handle& handle) String&& serviceWorkerRegistrationDirectory const SandboxExtension::Handle& > Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerToContextConnection.cpp:41 > + : SWServerToContextConnection(securityOrigin) SWServerToContextConnection should probably take a SecurityOriginData&& > Source/WebKit/NetworkProcess/mac/com.apple.WebKit.NetworkProcess.sb.in:65 > + (allow file-issue-extension (require-all (extension-class "com.apple.app-sandbox.read-write") (subpath path)))))) Can you explain why we need these sandbox changes?
Created attachment 350650 [details] Patch
Comment on attachment 350636 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=350636&action=review >> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:65 >> +#if ENABLE(SERVICE_WORKER) > > Do we need this #if? It seems that we only need these two header files & these two files only have content when we enable service worker. So this #if can make this more clear. >> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:758 >> +void NetworkConnectionToWebProcess::establishSWServerConnection(PAL::SessionID sessionID, SWServerConnectionIdentifier& serverConnectionIdentifier) > > I think we now go with SWServerConnectionIdentifier and not SWServerConnectionIdentifier&. Okay. >> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.h:193 >> + HashMap<WebCore::SWServerConnectionIdentifier, WeakPtr<WebSWServerConnection>> m_swConnections; > > We usually do not mix members and methods. Okay. >> Source/WebKit/NetworkProcess/NetworkProcess.cpp:1235 >> + auto result = m_swServers.add(sessionID, nullptr); > > Can we try to use HashMap::ensure? Okay. >> Source/WebKit/NetworkProcess/NetworkProcess.cpp:1372 >> +void NetworkProcess::addServiceWorkerSession(PAL::SessionID sessionID, String& serviceWorkerRegistrationDirectory, SandboxExtension::Handle& handle) > > String&& serviceWorkerRegistrationDirectory > const SandboxExtension::Handle& Okay. >> Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerToContextConnection.cpp:41 >> + : SWServerToContextConnection(securityOrigin) > > SWServerToContextConnection should probably take a SecurityOriginData&& In NetworkProcess::createNetworkConnectionToWebProcess, m_serverToContextConnections also takes SecurityOriginData&&. Do you think we need to make a change there? >> Source/WebKit/NetworkProcess/mac/com.apple.WebKit.NetworkProcess.sb.in:65 >> + (allow file-issue-extension (require-all (extension-class "com.apple.app-sandbox.read-write") (subpath path)))))) > > Can you explain why we need these sandbox changes? I compared the entitlements of storage process and network process, and found storage process had more permissions on "DARWIN_USER_CACHE_DIR", so I moved the code here.
Created attachment 350661 [details] Patch
Comment on attachment 350661 [details] Patch Attachment 350661 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/9334358 Number of test failures exceeded the failure limit.
Created attachment 350675 [details] Archive of layout-test-results from ews106 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Comment on attachment 350661 [details] Patch Attachment 350661 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/9334421 Number of test failures exceeded the failure limit.
Created attachment 350679 [details] Archive of layout-test-results from ews126 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.6
> >> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:65 > >> +#if ENABLE(SERVICE_WORKER) > > > > Do we need this #if? > > It seems that we only need these two header files & these two files only > have content when we enable service worker. So this #if can make this more > clear. If it is not needed, it is best to remove these, the preprocessed file will otherwise have a #if embedded in a #if. We probably include other service worker header files in the same cpp without the #if. > >> Source/WebKit/NetworkProcess/mac/com.apple.WebKit.NetworkProcess.sb.in:65 > >> + (allow file-issue-extension (require-all (extension-class "com.apple.app-sandbox.read-write") (subpath path)))))) > > > > Can you explain why we need these sandbox changes? > > I compared the entitlements of storage process and network process, and > found storage process had more permissions on "DARWIN_USER_CACHE_DIR", so I > moved the code here. Maybe Brent would know the reason of these rules. If these rules are not service worker related, I would tend to not add them.
(In reply to youenn fablet from comment #21) > > >> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:65 > > >> +#if ENABLE(SERVICE_WORKER) > > > > > > Do we need this #if? > > > > It seems that we only need these two header files & these two files only > > have content when we enable service worker. So this #if can make this more > > clear. > > If it is not needed, it is best to remove these, the preprocessed file will > otherwise have a #if embedded in a #if. > We probably include other service worker header files in the same cpp > without the #if. > Okay. > > >> Source/WebKit/NetworkProcess/mac/com.apple.WebKit.NetworkProcess.sb.in:65 > > >> + (allow file-issue-extension (require-all (extension-class "com.apple.app-sandbox.read-write") (subpath path)))))) > > > > > > Can you explain why we need these sandbox changes? > > > > I compared the entitlements of storage process and network process, and > > found storage process had more permissions on "DARWIN_USER_CACHE_DIR", so I > > moved the code here. > > Maybe Brent would know the reason of these rules. > If these rules are not service worker related, I would tend to not add them. It seems to be unrelated to service worker. I will revert the change.
Created attachment 350742 [details] Patch
Created attachment 350753 [details] Patch
Comment on attachment 350753 [details] Patch LGTM once bots are happy. A few nits. View in context: https://bugs.webkit.org/attachment.cgi?id=350753&action=review > Source/WebKit/NetworkProcess/NetworkProcess.messages.in:28 > + CreateNetworkConnectionToWebProcess(bool isServiceWorkerProcess, struct WebCore::SecurityOriginData origin) In the future, we might want to go with service worker instances run in a web process that also loads web pages, in which case this will break. This is fine for now. > Source/WebKit/NetworkProcess/ServiceWorker/WebSWOriginStore.h:44 > + void importComplete() final; Can it be made private? > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1637 > +#if ENABLE(SERVICE_WORKER) This is for non cocoa ports that do not support yet service workers. Probably fine to have this change although untested. Looking at the cocoa version, there is some redundant code for IDB and resolve directories. The main difference seems cookie handling. A follow-up patch could try to share more code. > Source/WebKit/WebProcess/Network/NetworkProcessConnection.h:32 > +#include "WebSWClientConnection.h" Maybe a forward declaration of WebSWClientConnection will be all that is needed.
Comment on attachment 350753 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=350753&action=review >> Source/WebKit/NetworkProcess/NetworkProcess.messages.in:28 >> + CreateNetworkConnectionToWebProcess(bool isServiceWorkerProcess, struct WebCore::SecurityOriginData origin) > > In the future, we might want to go with service worker instances run in a web process that also loads web pages, in which case this will break. > This is fine for now. In that case, there will be only be one type of web process? Then we could make the change when we need to remove service worker process. >> Source/WebKit/NetworkProcess/ServiceWorker/WebSWOriginStore.h:44 >> + void importComplete() final; > > Can it be made private? It's used in SWServer::registrationStoreImportComplete(). >> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1637 >> +#if ENABLE(SERVICE_WORKER) > > This is for non cocoa ports that do not support yet service workers. > Probably fine to have this change although untested. > Looking at the cocoa version, there is some redundant code for IDB and resolve directories. > The main difference seems cookie handling. A follow-up patch could try to share more code. I agree. Let's make optimizations in follow-up patch. >> Source/WebKit/WebProcess/Network/NetworkProcessConnection.h:32 >> +#include "WebSWClientConnection.h" > > Maybe a forward declaration of WebSWClientConnection will be all that is needed. Okay.
> >> Source/WebKit/NetworkProcess/NetworkProcess.messages.in:28 > >> + CreateNetworkConnectionToWebProcess(bool isServiceWorkerProcess, struct WebCore::SecurityOriginData origin) > > > > In the future, we might want to go with service worker instances run in a web process that also loads web pages, in which case this will break. > > This is fine for now. > > In that case, there will be only be one type of web process? Then we could > make the change when we need to remove service worker process. Yep. > >> Source/WebKit/NetworkProcess/ServiceWorker/WebSWOriginStore.h:44 > >> + void importComplete() final; > > > > Can it be made private? > > It's used in SWServer::registrationStoreImportComplete(). SWServer owns a SWOriginStore which declares importComplete as public. WebSWOriginStore probably does not need to expose its overriding importComplete as public.
Created attachment 350775 [details] Patch for landing
Comment on attachment 350775 [details] Patch for landing Rejecting attachment 350775 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'apply-attachment', '--no-update', '--non-interactive', 350775, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 5000 characters of output: /ServiceWorker/WebSWServerToContextConnection.cpp patching file Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerToContextConnection.h patching file Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerToContextConnection.messages.in patching file Source/WebKit/NetworkProcess/ios/NetworkProcessIOS.mm patching file Source/WebKit/Shared/Storage/StorageProcessCreationParameters.cpp patching file Source/WebKit/Shared/Storage/StorageProcessCreationParameters.h patching file Source/WebKit/Shared/WebsiteDataStoreParameters.cpp patching file Source/WebKit/Shared/WebsiteDataStoreParameters.h patching file Source/WebKit/Sources.txt patching file Source/WebKit/StorageProcess/ServiceWorker/WebSWOriginStore.cpp rm 'Source/WebKit/StorageProcess/ServiceWorker/WebSWOriginStore.cpp' patching file Source/WebKit/StorageProcess/ServiceWorker/WebSWOriginStore.h rm 'Source/WebKit/StorageProcess/ServiceWorker/WebSWOriginStore.h' patching file Source/WebKit/StorageProcess/ServiceWorker/WebSWServerConnection.cpp rm 'Source/WebKit/StorageProcess/ServiceWorker/WebSWServerConnection.cpp' patching file Source/WebKit/StorageProcess/ServiceWorker/WebSWServerConnection.h rm 'Source/WebKit/StorageProcess/ServiceWorker/WebSWServerConnection.h' patching file Source/WebKit/StorageProcess/ServiceWorker/WebSWServerConnection.messages.in rm 'Source/WebKit/StorageProcess/ServiceWorker/WebSWServerConnection.messages.in' patching file Source/WebKit/StorageProcess/ServiceWorker/WebSWServerToContextConnection.cpp rm 'Source/WebKit/StorageProcess/ServiceWorker/WebSWServerToContextConnection.cpp' patching file Source/WebKit/StorageProcess/ServiceWorker/WebSWServerToContextConnection.h rm 'Source/WebKit/StorageProcess/ServiceWorker/WebSWServerToContextConnection.h' patching file Source/WebKit/StorageProcess/ServiceWorker/WebSWServerToContextConnection.messages.in rm 'Source/WebKit/StorageProcess/ServiceWorker/WebSWServerToContextConnection.messages.in' patching file Source/WebKit/StorageProcess/StorageProcess.cpp patching file Source/WebKit/StorageProcess/StorageProcess.h patching file Source/WebKit/StorageProcess/StorageProcess.messages.in patching file Source/WebKit/StorageProcess/StorageToWebProcessConnection.cpp patching file Source/WebKit/StorageProcess/StorageToWebProcessConnection.h patching file Source/WebKit/StorageProcess/StorageToWebProcessConnection.messages.in patching file Source/WebKit/StorageProcess/ios/StorageProcessIOS.mm patching file Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp Hunk #2 succeeded at 116 (offset -5 lines). Hunk #3 succeeded at 203 (offset -5 lines). Hunk #4 succeeded at 289 (offset -5 lines). Hunk #5 succeeded at 346 (offset -5 lines). Hunk #6 succeeded at 687 (offset -5 lines). patching file Source/WebKit/UIProcess/Network/NetworkProcessProxy.h Hunk #1 succeeded at 69 with fuzz 1. Hunk #2 succeeded at 160 (offset -2 lines). patching file Source/WebKit/UIProcess/Network/NetworkProcessProxy.messages.in patching file Source/WebKit/UIProcess/ServiceWorkerProcessProxy.cpp patching file Source/WebKit/UIProcess/Storage/StorageProcessProxy.cpp Hunk #1 succeeded at 135 (offset -5 lines). Hunk #2 succeeded at 225 (offset -5 lines). patching file Source/WebKit/UIProcess/Storage/StorageProcessProxy.h Hunk #1 succeeded at 77 (offset -2 lines). patching file Source/WebKit/UIProcess/Storage/StorageProcessProxy.messages.in patching file Source/WebKit/UIProcess/WebProcessPool.cpp Hunk #4 FAILED at 613. Hunk #5 FAILED at 647. 2 out of 9 hunks FAILED -- saving rejects to file Source/WebKit/UIProcess/WebProcessPool.cpp.rej patching file Source/WebKit/UIProcess/WebProcessPool.h patching file Source/WebKit/UIProcess/WebProcessProxy.cpp patching file Source/WebKit/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm patching file Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp patching file Source/WebKit/WebKit.xcodeproj/project.pbxproj patching file Source/WebKit/WebProcess/Network/NetworkProcessConnection.cpp patching file Source/WebKit/WebProcess/Network/NetworkProcessConnection.h patching file Source/WebKit/WebProcess/Storage/WebSWClientConnection.cpp patching file Source/WebKit/WebProcess/Storage/WebSWContextManagerConnection.cpp patching file Source/WebKit/WebProcess/Storage/WebSWContextManagerConnection.h patching file Source/WebKit/WebProcess/Storage/WebServiceWorkerFetchTaskClient.cpp patching file Source/WebKit/WebProcess/Storage/WebServiceWorkerProvider.cpp patching file Source/WebKit/WebProcess/Storage/WebToStorageProcessConnection.cpp patching file Source/WebKit/WebProcess/Storage/WebToStorageProcessConnection.h patching file Source/WebKit/WebProcess/WebProcess.cpp patching file Source/WebKit/WebProcess/WebProcess.h patching file Source/WebKit/WebProcess/WebProcess.messages.in patching file Tools/ChangeLog patching file Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Full output: https://webkit-queues.webkit.org/results/9346202
Created attachment 350790 [details] Patch for landing
Created attachment 350791 [details] Patch for landing
Attachment 350791 [details] did not pass style-queue: ERROR: Source/WebKit/WebProcess/Storage/WebServiceWorkerProvider.cpp:34: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 46 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 350791 [details] Patch for landing Clearing flags on attachment: 350791 Committed r236477: <https://trac.webkit.org/changeset/236477>
All reviewed patches have been landed. Closing bug.
<rdar://problem/44775415>
Created attachment 351328 [details] LoadingTimeTest
With this patch, it could take ~250ms less to load first web page with Service Worker.
(In reply to Sihui Liu from comment #37) > With this patch, it could take ~250ms less to load first web page with > Service Worker. Nice!
A Membuster A/B test reported a net memory savings of 0MB - 1.5MB. Unfortunately, the test is too noisy to be more specific.
Typically, some zones in breakdown are noisy, and others aren't. For a change like this, I expect a prominent change in a non-noisy zone.