Start to shrink storage process.
Created attachment 349160 [details] Patch
Created attachment 349162 [details] Patch
Created attachment 349169 [details] Patch
Created attachment 349176 [details] Patch
Why are we doing this?
Comment on attachment 349176 [details] Patch Attachment 349176 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/9131712 New failing tests: storage/indexeddb/modern/opendatabase-after-storage-crash.html
Created attachment 349185 [details] Archive of layout-test-results from ews104 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Comment on attachment 349176 [details] Patch Attachment 349176 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/9131924 New failing tests: storage/indexeddb/modern/opendatabase-after-storage-crash.html
Created attachment 349191 [details] Archive of layout-test-results from ews124 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews124 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.4
Created attachment 349194 [details] Patch
(In reply to Sam Weinig from comment #5) > Why are we doing this? We want to remove storage process to reduce resource usage.
Created attachment 349198 [details] Patch
(In reply to Sihui Liu from comment #11) > (In reply to Sam Weinig from comment #5) > > Why are we doing this? > > We want to remove storage process to reduce resource usage. I'm not sure I understand. Can you expand on this?
If I understand correctly, we want to merge the storage process into the network process to eliminate the overhead of having a separate process for storage access, and reduce the latency for service workers. An alternative idea we considered was moving this code into UI process but we decided not to do because that would make any bugs in our storage code to potentially give access to the entirely of the file system on macOS in the case the app is not sandboxed. Finally, we have some need to potentially make network/storage processes dedicated per web site storage location (as opposed to per process pool) to avoid data corruptions, and that work would be a lot easier if we had one process instead of two.
(In reply to Ryosuke Niwa from comment #14) > If I understand correctly, we want to merge the storage process into the > network process to eliminate the overhead of having a separate process for > storage access, and reduce the latency for service workers. Seems like an admirable goal. Do you have any measurements you can share that lead you to this decision? I am particularly interested in what kind of latency is being observed for service workers. > An alternative idea we considered was moving this code into UI process but > we decided not to do because that would make any bugs in our storage code to > potentially give access to the entirely of the file system on macOS in the > case the app is not sandboxed. Definitely would be worse from a security perspective to move things to the UI process, but you are not quite right in saying that bugs in the storage code would give access to the entire file system on macOS, it would only have access to the embedding app's sandbox. We definitely moved things out of the UI Process on purpose.
(In reply to Sam Weinig from comment #15) > (In reply to Ryosuke Niwa from comment #14) > > If I understand correctly, we want to merge the storage process into the > > network process to eliminate the overhead of having a separate process for > > storage access, and reduce the latency for service workers. > > Seems like an admirable goal. Do you have any measurements you can share > that lead you to this decision? I am particularly interested in what kind of > latency is being observed for service workers. I believe Youenn & Chris know about this. > > An alternative idea we considered was moving this code into UI process but > > we decided not to do because that would make any bugs in our storage code to > > potentially give access to the entirely of the file system on macOS in the > > case the app is not sandboxed. > > Definitely would be worse from a security perspective to move things to the > UI process, but you are not quite right in saying that bugs in the storage > code would give access to the entire file system on macOS, it would only > have access to the embedding app's sandbox. We definitely moved things out > of the UI Process on purpose. Right, in the case app DOES have a sandbox, it would be limited to that. But I believe there are many macOS apps that don't have any sandboxes, and in those apps, we'd end up letting malicious code access anything if I understand correctly. I think there was also a concern that letting malicious code access the entirely of the app sandbox might be also bad. e.g. being able to read Safari bookmarks & history would be pretty bad.
> > If I understand correctly, we want to merge the storage process into the > > network process to eliminate the overhead of having a separate process for > > storage access, and reduce the latency for service workers. > > Seems like an admirable goal. Do you have any measurements you can share > that lead you to this decision? I am particularly interested in what kind of > latency is being observed for service workers. For memory, Activity Monitor says the Storage process costs about 5MB. For latency, Apple's internal launch time benchmarks measured a regression when we first enabled Service Workers, which required the storage process on startup. (We worked around that problem by deferring storage process startup on systems that had never registered a service worker -- but the potential regression remains, and service workers are becoming more popular.) Relatedly, PerformanceTests/LaunchTime says the latency of WebContent process launch is about 300ms on a MacBook Pro. Storage process launch isn't identical, but it's similar enough.
Comment on attachment 349198 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=349198&action=review > Source/WebKit/ChangeLog:7 > + A few good things to explain here: What's the motivation for this change? Do we expect an observable performance or behavior change? Is this a complete patch, or is it part of an ongoing series of patches? (I think I know the answers to these questions, but other WebKit contributors won't.) > Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:157 > +#if ENABLE(INDEXED_DATABASE) > + if (decoder.messageReceiverName() == Messages::WebIDBConnectionToClient::messageReceiverName()) { > + auto iterator = m_webIDBConnections.find(decoder.destinationID()); > + if (iterator != m_webIDBConnections.end()) > + iterator->value->didReceiveMessage(connection, decoder); > + return; > + } > +#endif This kind of change is good to explain in the ChangeLog entry for NetworkConnectionToWebProcess::didReceiveMessage. In particular, it's interesting to know if you just moved this code from StorageToWebProcessConnection::didReceiveMessage, or if you made any changes to it. > Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:514 > - NetworkProcess::singleton().grantSandboxExtensionsToStorageProcessForBlobs(fileNames, [this, protectedThis = WTFMove(protectedThis), requestIdentifier, fileNames]() { > - if (!m_connection->isValid()) > - return; > - > - m_connection->send(Messages::NetworkProcessConnection::DidWriteBlobsToTemporaryFiles(requestIdentifier, fileNames), 0); > +#if ENABLE(SANDBOX_EXTENSIONS) > + NetworkProcess::singleton().getSandboxExtensionsForBlobFiles(fileNames, [protectedThis = WTFMove(protectedThis), fileNames](SandboxExtension::HandleArray&& handles) { > + ASSERT(fileNames.size() == handles.size()); > + NetworkProcess::singleton().updateTemporaryFileSandboxExtensions(fileNames, handles); > }); > +#endif > + > + if (!m_connection->isValid()) > + return; > + m_connection->send(Messages::NetworkProcessConnection::DidWriteBlobsToTemporaryFiles(requestIdentifier, fileNames), 0); How does this change relate to migrating IndexedDB? > Source/WebKit/NetworkProcess/NetworkProcess.cpp:117 > + , m_queue(WorkQueue::create("com.apple.WebKit.NetworkProcess")) This queue probably deserves a more specific data member name and queue name. Something to indicate that its purpose is to process storage tasks.
(In reply to Geoffrey Garen from comment #17) > > > If I understand correctly, we want to merge the storage process into the > > > network process to eliminate the overhead of having a separate process for > > > storage access, and reduce the latency for service workers. > > > > Seems like an admirable goal. Do you have any measurements you can share > > that lead you to this decision? I am particularly interested in what kind of > > latency is being observed for service workers. > > For memory, Activity Monitor says the Storage process costs about 5MB. I usually see about the same. Do you have an idea of how much of that will now be added to the Networking process? > > For latency, Apple's internal launch time benchmarks measured a regression > when we first enabled Service Workers, which required the storage process on > startup. (We worked around that problem by deferring storage process startup > on systems that had never registered a service worker -- but the potential > regression remains, and service workers are becoming more popular.) What caused the regression here? Just the act of launching another service? Or was something waiting on it?
> > For latency, Apple's internal launch time benchmarks measured a regression > > when we first enabled Service Workers, which required the storage process on > > startup. (We worked around that problem by deferring storage process startup > > on systems that had never registered a service worker -- but the potential > > regression remains, and service workers are becoming more popular.) > > What caused the regression here? Just the act of launching another service? > Or was something waiting on it? The logic of Service Workers delays the page load until you get a response from the storage process.
> The logic of Service Workers delays the page load until you get a response > from the storage process. I believe the main cost is storage process launch. Before answering any SW request, the storage process is loading from disk all the service worker registrations. This cost will keep happening in Network process. We could probably optimize it in the future if needs be.
Comment on attachment 349198 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=349198&action=review >> Source/WebKit/ChangeLog:7 >> + > > A few good things to explain here: What's the motivation for this change? Do we expect an observable performance or behavior change? Is this a complete patch, or is it part of an ongoing series of patches? (I think I know the answers to these questions, but other WebKit contributors won't.) This patch uploaded for checking test results so I haven't added change logs. Will do! >> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:157 >> +#endif > > This kind of change is good to explain in the ChangeLog entry for NetworkConnectionToWebProcess::didReceiveMessage. In particular, it's interesting to know if you just moved this code from StorageToWebProcessConnection::didReceiveMessage, or if you made any changes to it. Got it! >> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:514 >> + m_connection->send(Messages::NetworkProcessConnection::DidWriteBlobsToTemporaryFiles(requestIdentifier, fileNames), 0); > > How does this change relate to migrating IndexedDB? This is for dumping indexedDB blobs to temporary files on disk. See: https://bugs.webkit.org/show_bug.cgi?id=156068. Originally Network Process writes temporary file and in completion handler, asks UI process to grant sandbox extension of those files to Storage Process. Here I change it to get and update sandbox extension in completion handler if ENABLE(SANDBOX_EXTENSIONS). >> Source/WebKit/NetworkProcess/NetworkProcess.cpp:117 >> + , m_queue(WorkQueue::create("com.apple.WebKit.NetworkProcess")) > > This queue probably deserves a more specific data member name and queue name. Something to indicate that its purpose is to process storage tasks. Will do.
Created attachment 349343 [details] Patch
Comment on attachment 349343 [details] Patch r=me
Comment on attachment 349343 [details] Patch Clearing flags on attachment: 349343 Committed r235954: <https://trac.webkit.org/changeset/235954>
All reviewed patches have been landed. Closing bug.
<rdar://problem/44396973>
Looks like this patch https://trac.webkit.org/changeset/235954/webkit has caused two API failures, one crash and one failure. Log: https://build.webkit.org/builders/Apple%20Sierra%20Debug%20WK2%20%28Tests%29/builds/7976/steps/run-api-tests/logs/stdio the failure occurs on all Mac while the Crash only seems to effect Sierra.
(In reply to Truitt Savell from comment #28) > Looks like this patch https://trac.webkit.org/changeset/235954/webkit > has caused two API failures, one crash and one failure. > > Log: > https://build.webkit.org/builders/Apple%20Sierra%20Debug%20WK2%20%28Tests%29/ > builds/7976/steps/run-api-tests/logs/stdio > > the failure occurs on all Mac while the Crash only seems to effect Sierra. It seems like WebKit.WKHTTPCookieStoreNonPersistent failure is caused by this patch, but ProcessSwap.BackWithoutSuspendedPage probably not.
(In reply to Sihui Liu from comment #29) > > It seems like WebKit.WKHTTPCookieStoreNonPersistent failure is caused by > this patch, but ProcessSwap.BackWithoutSuspendedPage probably not. okay I will look into other revisions for that one
(In reply to Truitt Savell from comment #30) > (In reply to Sihui Liu from comment #29) > > > > It seems like WebKit.WKHTTPCookieStoreNonPersistent failure is caused by > > this patch, but ProcessSwap.BackWithoutSuspendedPage probably not. > > okay I will look into other revisions for that one The test is added in https://trac.webkit.org/changeset/235952/webkit, so this might be interesting.
This broke internal builds. See radar for more information.
Reverted r235954 for reason: Breaks the watchOS build. Committed r235995: <https://trac.webkit.org/changeset/235995>
(In reply to Ryan Haddad from comment #33) > Reverted r235954 for reason: > > Breaks the watchOS build. > > Committed r235995: <https://trac.webkit.org/changeset/235995> Once you land the fix for Bug 189605, you should be able to re-land this.
I am going to re-land this patch as: 1. it should not cause WebKit.WKHTTPCookieStoreNonPersistent failure on Sierra now: I added check sessionID.isEphemeral() in NetworkProcess::deleteWebsiteData and NetworkProcess::deleteWebsiteDataForOrigins, so the assertion NetworkProcess::idbServer() won't be hit. 2. it should not break the watch OS build after David's change https://trac.webkit.org/changeset/236007.
Created attachment 349779 [details] Patch for landing
Created attachment 349782 [details] Patch
Comment on attachment 349782 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=349782&action=review r=me with comments > Source/WebKit/ChangeLog:25 > + I think this is 1 blank line too many? > Source/WebKit/ChangeLog:29 > + Why the blank line? > Source/WebKit/ChangeLog:124 > + process just asks for sandbox extension fot itself. Typo: fot. > Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:512 > + if (!m_connection->isValid()) This check should not be needed as IPC::Connection::send() already does this. > Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.h:38 > +#include <pal/SessionID.h> Can probably be forward-declared. > Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.h:245 > + // Messages handlers (Modern IDB) Period at the end. > Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.h:246 > + void establishIDBConnectionToServer(PAL::SessionID, uint64_t& serverConnectionIdentifier); We do not usually put methods in the middle of the data members, please move these methods above, even if you need a new #if ENABLE(INDEXED_DATABASE) block. > Source/WebKit/NetworkProcess/NetworkProcess.cpp:986 > +void NetworkProcess::ensurePathExists(const String& path) Does not seem like this method is needed. Why not use FileSystem::makeAllDirectories() directly? > Source/WebKit/NetworkProcess/NetworkProcess.cpp:1064 > + auto addResult = m_idbDatabasePaths.ensure(sessionID, [path = indexedDatabaseDirectory] { I do not see any benefit to using ensure() here, add() would suffice and likely be more efficient: auto addResult = m_idbDatabasePaths.add(sessionID, indexedDatabaseDirectory); > Source/WebKit/NetworkProcess/NetworkProcess.cpp:1077 > +void NetworkProcess::getSandboxExtensionsForBlobFiles(const Vector<String>& filenames, WTF::Function<void(SandboxExtension::HandleArray&&)>&& completionHandler) WTF:: is not needed. > Source/WebKit/NetworkProcess/NetworkProcess.h:169 > + WorkQueue& queue() { return m_storageTaskQueue.get(); } queue() is too generic a name here given that we're on the Network process. Maybe storageQueue() ? Also does this method need to be public? > Source/WebKit/NetworkProcess/NetworkProcess.h:170 > + void postStorageTask(CrossThreadTask&&); does this method need to be public? > Source/WebKit/NetworkProcess/NetworkProcess.h:173 > + WebCore::IDBServer::IDBServer& idbServer(PAL::SessionID); does this method need to be public? > Source/WebKit/NetworkProcess/NetworkProcess.h:176 > + void prepareForAccessToTemporaryFile(const String& path) final; does this method need to be public? > Source/WebKit/NetworkProcess/NetworkProcess.h:177 > + void accessToTemporaryFileComplete(const String& path) final; does this method need to be public? > Source/WebKit/NetworkProcess/NetworkProcess.h:181 > + void getSandboxExtensionsForBlobFiles(const Vector<String>& filenames, WTF::Function<void(SandboxExtension::HandleArray&&)>&& completionHandler); does this method need to be public? > Source/WebKit/NetworkProcess/NetworkProcess.h:182 > + void updateTemporaryFileSandboxExtensions(const Vector<String>& paths, SandboxExtension::HandleArray&); does this method need to be public? > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1635 > + resolveDirectoriesIfNecessary(); This is already called at the beginning of the method, why is this needed again here? > Source/WebKit/WebProcess/Network/NetworkProcessConnection.cpp:113 > + auto idbConnection = m_webIDBConnectionsByIdentifier.get(decoder.destinationID()); can be: if (auto idbConnection = m_webIDBConnectionsByIdentifier.get(decoder.destinationID())) > Source/WebKit/WebProcess/Network/NetworkProcessConnection.h:68 > + WebIDBConnectionToServer* existingIDBConnectionToServerForIdentifier(uint64_t identifier) { return m_webIDBConnectionsByIdentifier.get(identifier); }; Can probably be const.
Comment on attachment 349782 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=349782&action=review >> Source/WebKit/ChangeLog:25 >> + > > I think this is 1 blank line too many? Yes.. will remove it. >> Source/WebKit/ChangeLog:29 >> + > > Why the blank line? Make it more clear that I am change the make file... will remove it. >> Source/WebKit/ChangeLog:124 >> + process just asks for sandbox extension fot itself. > > Typo: fot. Fixed. >> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:512 >> + if (!m_connection->isValid()) > > This check should not be needed as IPC::Connection::send() already does this. Okay. >> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.h:38 >> +#include <pal/SessionID.h> > > Can probably be forward-declared. Will do. >> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.h:245 >> + // Messages handlers (Modern IDB) > > Period at the end. Okay. >> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.h:246 >> + void establishIDBConnectionToServer(PAL::SessionID, uint64_t& serverConnectionIdentifier); > > We do not usually put methods in the middle of the data members, please move these methods above, even if you need a new #if ENABLE(INDEXED_DATABASE) block. Okay. >> Source/WebKit/NetworkProcess/NetworkProcess.cpp:986 >> +void NetworkProcess::ensurePathExists(const String& path) > > Does not seem like this method is needed. Why not use FileSystem::makeAllDirectories() directly? Because createCrossThreadTask takes a void function? >> Source/WebKit/NetworkProcess/NetworkProcess.cpp:1064 >> + auto addResult = m_idbDatabasePaths.ensure(sessionID, [path = indexedDatabaseDirectory] { > > I do not see any benefit to using ensure() here, add() would suffice and likely be more efficient: > auto addResult = m_idbDatabasePaths.add(sessionID, indexedDatabaseDirectory); Okay. >> Source/WebKit/NetworkProcess/NetworkProcess.cpp:1077 >> +void NetworkProcess::getSandboxExtensionsForBlobFiles(const Vector<String>& filenames, WTF::Function<void(SandboxExtension::HandleArray&&)>&& completionHandler) > > WTF:: is not needed. Got it. >> Source/WebKit/NetworkProcess/NetworkProcess.h:169 >> + WorkQueue& queue() { return m_storageTaskQueue.get(); } > > queue() is too generic a name here given that we're on the Network process. Maybe storageQueue() ? > > Also does this method need to be public? No, and it seemed to be not used, so removed it. >> Source/WebKit/NetworkProcess/NetworkProcess.h:170 >> + void postStorageTask(CrossThreadTask&&); > > does this method need to be public? No, removed to private. >> Source/WebKit/NetworkProcess/NetworkProcess.h:173 >> + WebCore::IDBServer::IDBServer& idbServer(PAL::SessionID); > > does this method need to be public? Yes, called in WebIDBConnectionToClient::WebIDBConnectionToClient. >> Source/WebKit/NetworkProcess/NetworkProcess.h:176 >> + void prepareForAccessToTemporaryFile(const String& path) final; > > does this method need to be public? Yes, called in SQLiteIDBTransaction::moveBlobFilesIfNecessary. >> Source/WebKit/NetworkProcess/NetworkProcess.h:177 >> + void accessToTemporaryFileComplete(const String& path) final; > > does this method need to be public? Yes, called in SQLiteIDBTransaction::moveBlobFilesIfNecessary(). >> Source/WebKit/NetworkProcess/NetworkProcess.h:181 >> + void getSandboxExtensionsForBlobFiles(const Vector<String>& filenames, WTF::Function<void(SandboxExtension::HandleArray&&)>&& completionHandler); > > does this method need to be public? Yes, called in NetworkConnectionToWebProcess::writeBlobsToTemporaryFiles. >> Source/WebKit/NetworkProcess/NetworkProcess.h:182 >> + void updateTemporaryFileSandboxExtensions(const Vector<String>& paths, SandboxExtension::HandleArray&); > > does this method need to be public? Yes, called in NetworkConnectionToWebProcess::writeBlobsToTemporaryFiles. >> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1635 >> + resolveDirectoriesIfNecessary(); > > This is already called at the beginning of the method, why is this needed again here? You mean at the beginning of WebsiteDataStore::parameters() in WebsiteDataStoreCocoa.mm? >> Source/WebKit/WebProcess/Network/NetworkProcessConnection.cpp:113 >> + auto idbConnection = m_webIDBConnectionsByIdentifier.get(decoder.destinationID()); > > can be: > if (auto idbConnection = m_webIDBConnectionsByIdentifier.get(decoder.destinationID())) Done. >> Source/WebKit/WebProcess/Network/NetworkProcessConnection.h:68 >> + WebIDBConnectionToServer* existingIDBConnectionToServerForIdentifier(uint64_t identifier) { return m_webIDBConnectionsByIdentifier.get(identifier); }; > > Can probably be const. Okay.
Created attachment 349877 [details] Patch for landing
Comment on attachment 349877 [details] Patch for landing Clearing flags on attachment: 349877 Committed r236035: <https://trac.webkit.org/changeset/236035>