Speed up StorageManager::getValues()
https://bugs.webkit.org/show_bug.cgi?id=199812
Reviewed by Alex Christensen.
Source/WebCore:
* storage/StorageMap.cpp:
(WebCore::StorageMap::importItems):
* storage/StorageMap.h:
Source/WebKit:
Made the following performance improvements:
- Made StorageManager a WorkQueueMessageReceiver again (like it was before it
got moved from the UIProcess to the Network process). This avoids a lot of
thread hopping (IPC thread -> Main thread -> StorageManagerThread -> Main Thread)
and a lot of isolatedCopying of the strings.
- Move values around when possible to avoid copying.
- Add fast path to StorageMap::importItems() for when the StorageMap is
empty when importing (15ms -> 2.5ms).
* NetworkProcess/NetworkConnectionToWebProcess.cpp:
(WebKit::NetworkConnectionToWebProcess::didReceiveMessage):
(WebKit::NetworkConnectionToWebProcess::didReceiveSyncMessage):
* NetworkProcess/WebStorage/LocalStorageDatabase.cpp:
(WebKit::LocalStorageDatabase::importItems):
* NetworkProcess/WebStorage/StorageManager.cpp:
(WebKit::StorageManager::addAllowedSessionStorageNamespaceConnection):
(WebKit::StorageManager::removeAllowedSessionStorageNamespaceConnection):
(WebKit::StorageManager::processDidCloseConnection):
(WebKit::StorageManager::createLocalStorageMap):
(WebKit::StorageManager::createTransientLocalStorageMap):
(WebKit::StorageManager::createSessionStorageMap):
(WebKit::StorageManager::destroyStorageMap):
(WebKit::StorageManager::getValues):
(WebKit::StorageManager::setItem):
(WebKit::StorageManager::setItems):
(WebKit::StorageManager::removeItem):
(WebKit::StorageManager::clear):
* NetworkProcess/WebStorage/StorageManager.h:
* Platform/IPC/Connection.cpp:
(IPC::Connection::addWorkQueueMessageReceiver):
(IPC::Connection::removeWorkQueueMessageReceiver):
(IPC::Connection::processIncomingMessage):
(IPC::Connection::dispatchMessage):
(IPC::Connection::dispatchMessageToWorkQueueReceiver):
* Platform/IPC/Connection.h:
* WebProcess/WebStorage/StorageAreaMap.cpp:
(WebKit::StorageAreaMap::loadValuesIfNeeded):
Messages to WorkQueueMessageReceivers are normally dispatched from the IPC WorkQueue. However, there is a race if
a client (here StorageManager) adds itself as a WorkQueueMessageReceiver as a result of receiving an IPC message
on the main thread (here NetworkConnectionToWebProcess::WebPageWasAdded).
The message might have already been dispatched from the IPC WorkQueue to the main thread by the time the
client registers itself as a WorkQueueMessageReceiver. To address this, we check again for messages receivers
once the message arrives on the main thread.
Source/WebKitLegacy:
* Storage/StorageAreaImpl.cpp:
(WebKit::StorageAreaImpl::importItems):
* Storage/StorageAreaImpl.h:
* Storage/StorageAreaSync.cpp:
(WebKit::StorageAreaSync::performImport):
git-svn-id: http://svn.webkit.org/repository/webkit/trunk@247486 268f45cc-cd09-0410-ab3c-d52691b4dbfc
diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
index c0d7c77..5dabf2b 100644
--- a/Source/WebCore/ChangeLog
+++ b/Source/WebCore/ChangeLog
@@ -1,3 +1,14 @@
+2019-07-16 Chris Dumez <cdumez@apple.com>
+
+ Speed up StorageManager::getValues()
+ https://bugs.webkit.org/show_bug.cgi?id=199812
+
+ Reviewed by Alex Christensen.
+
+ * storage/StorageMap.cpp:
+ (WebCore::StorageMap::importItems):
+ * storage/StorageMap.h:
+
2019-07-16 Wenson Hsieh <wenson_hsieh@apple.com>
[Text autosizing] [iPadOS] Paragraph text on the front page of LinkedIn.com is not boosted
diff --git a/Source/WebCore/storage/StorageMap.cpp b/Source/WebCore/storage/StorageMap.cpp
index 15598a8..ea5198d 100644
--- a/Source/WebCore/storage/StorageMap.cpp
+++ b/Source/WebCore/storage/StorageMap.cpp
@@ -182,19 +182,27 @@
return m_map.contains(key);
}
-void StorageMap::importItems(const HashMap<String, String>& items)
+void StorageMap::importItems(HashMap<String, String>&& items)
{
+ if (m_map.isEmpty()) {
+ // Fast path.
+ m_map = WTFMove(items);
+ for (auto& pair : m_map) {
+ ASSERT(m_currentLength + pair.key.length() + pair.value.length() >= m_currentLength);
+ m_currentLength += (pair.key.length() + pair.value.length());
+ }
+ return;
+ }
+
for (auto& item : items) {
- const String& key = item.key;
- const String& value = item.value;
+ auto& key = item.key;
+ auto& value = item.value;
- HashMap<String, String>::AddResult result = m_map.add(key, value);
+ ASSERT(m_currentLength + key.length() + value.length() >= m_currentLength);
+ m_currentLength += (key.length() + value.length());
+
+ auto result = m_map.add(WTFMove(key), WTFMove(value));
ASSERT_UNUSED(result, result.isNewEntry); // True if the key didn't exist previously.
-
- ASSERT(m_currentLength + key.length() >= m_currentLength);
- m_currentLength += key.length();
- ASSERT(m_currentLength + value.length() >= m_currentLength);
- m_currentLength += value.length();
}
}
diff --git a/Source/WebCore/storage/StorageMap.h b/Source/WebCore/storage/StorageMap.h
index fcdcca4..3a295ee 100644
--- a/Source/WebCore/storage/StorageMap.h
+++ b/Source/WebCore/storage/StorageMap.h
@@ -46,7 +46,7 @@
WEBCORE_EXPORT bool contains(const String& key) const;
- WEBCORE_EXPORT void importItems(const HashMap<String, String>&);
+ WEBCORE_EXPORT void importItems(HashMap<String, String>&&);
const HashMap<String, String>& items() const { return m_map; }
unsigned quota() const { return m_quotaSize; }
diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
index 639ed9c..c36bc25 100644
--- a/Source/WebKit/ChangeLog
+++ b/Source/WebKit/ChangeLog
@@ -1,3 +1,55 @@
+2019-07-16 Chris Dumez <cdumez@apple.com>
+
+ Speed up StorageManager::getValues()
+ https://bugs.webkit.org/show_bug.cgi?id=199812
+
+ Reviewed by Alex Christensen.
+
+ Made the following performance improvements:
+ - Made StorageManager a WorkQueueMessageReceiver again (like it was before it
+ got moved from the UIProcess to the Network process). This avoids a lot of
+ thread hopping (IPC thread -> Main thread -> StorageManagerThread -> Main Thread)
+ and a lot of isolatedCopying of the strings.
+ - Move values around when possible to avoid copying.
+ - Add fast path to StorageMap::importItems() for when the StorageMap is
+ empty when importing (15ms -> 2.5ms).
+
+ * NetworkProcess/NetworkConnectionToWebProcess.cpp:
+ (WebKit::NetworkConnectionToWebProcess::didReceiveMessage):
+ (WebKit::NetworkConnectionToWebProcess::didReceiveSyncMessage):
+ * NetworkProcess/WebStorage/LocalStorageDatabase.cpp:
+ (WebKit::LocalStorageDatabase::importItems):
+ * NetworkProcess/WebStorage/StorageManager.cpp:
+ (WebKit::StorageManager::addAllowedSessionStorageNamespaceConnection):
+ (WebKit::StorageManager::removeAllowedSessionStorageNamespaceConnection):
+ (WebKit::StorageManager::processDidCloseConnection):
+ (WebKit::StorageManager::createLocalStorageMap):
+ (WebKit::StorageManager::createTransientLocalStorageMap):
+ (WebKit::StorageManager::createSessionStorageMap):
+ (WebKit::StorageManager::destroyStorageMap):
+ (WebKit::StorageManager::getValues):
+ (WebKit::StorageManager::setItem):
+ (WebKit::StorageManager::setItems):
+ (WebKit::StorageManager::removeItem):
+ (WebKit::StorageManager::clear):
+ * NetworkProcess/WebStorage/StorageManager.h:
+
+ * Platform/IPC/Connection.cpp:
+ (IPC::Connection::addWorkQueueMessageReceiver):
+ (IPC::Connection::removeWorkQueueMessageReceiver):
+ (IPC::Connection::processIncomingMessage):
+ (IPC::Connection::dispatchMessage):
+ (IPC::Connection::dispatchMessageToWorkQueueReceiver):
+ * Platform/IPC/Connection.h:
+ * WebProcess/WebStorage/StorageAreaMap.cpp:
+ (WebKit::StorageAreaMap::loadValuesIfNeeded):
+ Messages to WorkQueueMessageReceivers are normally dispatched from the IPC WorkQueue. However, there is a race if
+ a client (here StorageManager) adds itself as a WorkQueueMessageReceiver as a result of receiving an IPC message
+ on the main thread (here NetworkConnectionToWebProcess::WebPageWasAdded).
+ The message might have already been dispatched from the IPC WorkQueue to the main thread by the time the
+ client registers itself as a WorkQueueMessageReceiver. To address this, we check again for messages receivers
+ once the message arrives on the main thread.
+
2019-07-16 Zalan Bujtas <zalan@apple.com>
[ContentChangeObserver] Cancel ongoing content observation when tap is failed/cancelled
diff --git a/Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp b/Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp
index b5bc8a3..87a6e5c 100644
--- a/Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp
+++ b/Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp
@@ -49,7 +49,6 @@
#include "PreconnectTask.h"
#include "ServiceWorkerFetchTaskMessages.h"
#include "StorageManager.h"
-#include "StorageManagerMessages.h"
#include "WebCoreArgumentCoders.h"
#include "WebErrors.h"
#include "WebIDBConnectionToClient.h"
@@ -232,13 +231,6 @@
return paymentCoordinator().didReceiveMessage(connection, decoder);
#endif
- if (decoder.messageReceiverName() == Messages::StorageManager::messageReceiverName()) {
- if (auto* session = m_networkProcess->networkSessionByConnection(connection)) {
- session->storageManager().didReceiveMessage(connection, decoder);
- return;
- }
- }
-
ASSERT_NOT_REACHED();
}
@@ -278,13 +270,6 @@
return paymentCoordinator().didReceiveSyncMessage(connection, decoder, reply);
#endif
- if (decoder.messageReceiverName() == Messages::StorageManager::messageReceiverName()) {
- if (auto* session = m_networkProcess->networkSessionByConnection(connection)) {
- session->storageManager().didReceiveSyncMessage(connection, decoder, reply);
- return;
- }
- }
-
ASSERT_NOT_REACHED();
}
diff --git a/Source/WebKit/NetworkProcess/WebStorage/LocalStorageDatabase.cpp b/Source/WebKit/NetworkProcess/WebStorage/LocalStorageDatabase.cpp
index 3f2f4f3..bf51578 100644
--- a/Source/WebKit/NetworkProcess/WebStorage/LocalStorageDatabase.cpp
+++ b/Source/WebKit/NetworkProcess/WebStorage/LocalStorageDatabase.cpp
@@ -171,7 +171,7 @@
if (!m_database.isOpen())
return;
- SQLiteStatement query(m_database, "SELECT key, value FROM ItemTable");
+ SQLiteStatement query(m_database, "SELECT key, value FROM ItemTable"_str);
if (query.prepare() != SQLITE_OK) {
LOG_ERROR("Unable to select items from ItemTable for local storage");
return;
@@ -184,7 +184,7 @@
String key = query.getColumnText(0);
String value = query.getColumnBlobAsString(1);
if (!key.isNull() && !value.isNull())
- items.set(key, value);
+ items.add(WTFMove(key), WTFMove(value));
result = query.step();
}
@@ -193,7 +193,7 @@
return;
}
- storageMap.importItems(items);
+ storageMap.importItems(WTFMove(items));
}
void LocalStorageDatabase::setItem(const String& key, const String& value)
diff --git a/Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp b/Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp
index 5e8f929..7e0336f 100644
--- a/Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp
+++ b/Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp
@@ -523,6 +523,10 @@
void StorageManager::addAllowedSessionStorageNamespaceConnection(uint64_t storageNamespaceID, IPC::Connection& allowedConnection)
{
auto allowedConnectionID = allowedConnection.uniqueID();
+ auto addResult = m_connections.add(allowedConnectionID);
+ if (addResult.isNewEntry)
+ allowedConnection.addWorkQueueMessageReceiver(Messages::StorageManager::messageReceiverName(), m_queue.get(), this);
+
m_queue->dispatch([this, protectedThis = makeRef(*this), allowedConnectionID, storageNamespaceID]() mutable {
ASSERT(m_sessionStorageNamespaces.contains(storageNamespaceID));
@@ -533,6 +537,9 @@
void StorageManager::removeAllowedSessionStorageNamespaceConnection(uint64_t storageNamespaceID, IPC::Connection& allowedConnection)
{
auto allowedConnectionID = allowedConnection.uniqueID();
+ if (m_connections.remove(allowedConnectionID))
+ allowedConnection.removeWorkQueueMessageReceiver(Messages::StorageManager::messageReceiverName());
+
m_queue->dispatch([this, protectedThis = makeRef(*this), allowedConnectionID, storageNamespaceID]() mutable {
ASSERT(m_sessionStorageNamespaces.contains(storageNamespaceID));
if (auto* sessionStorageNamespace = m_sessionStorageNamespaces.get(storageNamespaceID))
@@ -567,6 +574,9 @@
void StorageManager::processDidCloseConnection(IPC::Connection& connection)
{
+ if (m_connections.removeAll(connection.uniqueID()))
+ connection.removeWorkQueueMessageReceiver(Messages::StorageManager::messageReceiverName());
+
m_queue->dispatch([this, protectedThis = makeRef(*this), connectionID = connection.uniqueID()]() mutable {
Vector<std::pair<IPC::Connection::UniqueID, uint64_t>> connectionAndStorageMapIDPairsToRemove;
for (auto& storageArea : m_storageAreasByConnection) {
@@ -729,180 +739,170 @@
void StorageManager::createLocalStorageMap(IPC::Connection& connection, uint64_t storageMapID, uint64_t storageNamespaceID, SecurityOriginData&& securityOriginData)
{
- m_queue->dispatch([this, protectedThis = makeRef(*this), connectionID = connection.uniqueID(), storageMapID, storageNamespaceID, securityOriginData = securityOriginData.isolatedCopy()]() mutable {
- std::pair<IPC::Connection::UniqueID, uint64_t> connectionAndStorageMapIDPair(connectionID, storageMapID);
+ ASSERT(!RunLoop::isMain());
+ auto connectionID = connection.uniqueID();
+ std::pair<IPC::Connection::UniqueID, uint64_t> connectionAndStorageMapIDPair(connectionID, storageMapID);
- ASSERT((HashMap<std::pair<IPC::Connection::UniqueID, uint64_t>, RefPtr<StorageArea>>::isValidKey(connectionAndStorageMapIDPair)));
+ ASSERT((HashMap<std::pair<IPC::Connection::UniqueID, uint64_t>, RefPtr<StorageArea>>::isValidKey(connectionAndStorageMapIDPair)));
- auto result = m_storageAreasByConnection.add(connectionAndStorageMapIDPair, nullptr);
- ASSERT(result.isNewEntry);
- ASSERT((HashMap<uint64_t, RefPtr<LocalStorageNamespace>>::isValidKey(storageNamespaceID)));
+ auto result = m_storageAreasByConnection.add(connectionAndStorageMapIDPair, nullptr);
+ ASSERT(result.isNewEntry);
+ ASSERT((HashMap<uint64_t, RefPtr<LocalStorageNamespace>>::isValidKey(storageNamespaceID)));
- LocalStorageNamespace* localStorageNamespace = getOrCreateLocalStorageNamespace(storageNamespaceID);
- ASSERT(localStorageNamespace);
+ LocalStorageNamespace* localStorageNamespace = getOrCreateLocalStorageNamespace(storageNamespaceID);
+ ASSERT(localStorageNamespace);
- auto storageArea = localStorageNamespace->getOrCreateStorageArea(WTFMove(securityOriginData), m_localStorageDatabaseTracker ? StorageManager::LocalStorageNamespace::IsEphemeral::No : StorageManager::LocalStorageNamespace::IsEphemeral::Yes);
- storageArea->addListener(connectionID, storageMapID);
+ auto storageArea = localStorageNamespace->getOrCreateStorageArea(WTFMove(securityOriginData), m_localStorageDatabaseTracker ? StorageManager::LocalStorageNamespace::IsEphemeral::No : StorageManager::LocalStorageNamespace::IsEphemeral::Yes);
+ storageArea->addListener(connectionID, storageMapID);
- result.iterator->value = WTFMove(storageArea);
- });
+ result.iterator->value = WTFMove(storageArea);
}
void StorageManager::createTransientLocalStorageMap(IPC::Connection& connection, uint64_t storageMapID, uint64_t storageNamespaceID, SecurityOriginData&& topLevelOriginData, SecurityOriginData&& origin)
{
- m_queue->dispatch([this, protectedThis = makeRef(*this), connectionID = connection.uniqueID(), storageMapID, storageNamespaceID, topLevelOriginData = topLevelOriginData.isolatedCopy(), origin = origin.isolatedCopy()]() mutable {
- ASSERT(m_storageAreasByConnection.isValidKey({ connectionID, storageMapID }));
+ ASSERT(!RunLoop::isMain());
+ auto connectionID = connection.uniqueID();
- // See if we already have session storage for this connection/origin combo.
- // If so, update the map with the new ID, otherwise keep on trucking.
- for (auto it = m_storageAreasByConnection.begin(), end = m_storageAreasByConnection.end(); it != end; ++it) {
- if (it->key.first != connectionID)
- continue;
- Ref<StorageArea> area = *it->value;
- if (!area->isEphemeral())
- continue;
- if (!origin.securityOrigin()->isSameSchemeHostPort(area->securityOrigin().securityOrigin().get()))
- continue;
- area->addListener(connectionID, storageMapID);
- // If the storageMapID used as key in m_storageAreasByConnection is no longer one of the StorageArea's listeners, then this means
- // that destroyStorageMap() was already called for that storageMapID but it decided not to remove it from m_storageAreasByConnection
- // so that we could reuse it later on for the same connection/origin combo. In this case, it is safe to remove the previous
- // storageMapID from m_storageAreasByConnection.
- if (!area->hasListener(connectionID, it->key.second))
- m_storageAreasByConnection.remove(it);
- m_storageAreasByConnection.add({ connectionID, storageMapID }, WTFMove(area));
- return;
- }
+ ASSERT(m_storageAreasByConnection.isValidKey({ connectionID, storageMapID }));
- auto& slot = m_storageAreasByConnection.add({ connectionID, storageMapID }, nullptr).iterator->value;
- ASSERT(!slot);
+ // See if we already have session storage for this connection/origin combo.
+ // If so, update the map with the new ID, otherwise keep on trucking.
+ for (auto it = m_storageAreasByConnection.begin(), end = m_storageAreasByConnection.end(); it != end; ++it) {
+ if (it->key.first != connectionID)
+ continue;
+ Ref<StorageArea> area = *it->value;
+ if (!area->isEphemeral())
+ continue;
+ if (!origin.securityOrigin()->isSameSchemeHostPort(area->securityOrigin().securityOrigin().get()))
+ continue;
+ area->addListener(connectionID, storageMapID);
+ // If the storageMapID used as key in m_storageAreasByConnection is no longer one of the StorageArea's listeners, then this means
+ // that destroyStorageMap() was already called for that storageMapID but it decided not to remove it from m_storageAreasByConnection
+ // so that we could reuse it later on for the same connection/origin combo. In this case, it is safe to remove the previous
+ // storageMapID from m_storageAreasByConnection.
+ if (!area->hasListener(connectionID, it->key.second))
+ m_storageAreasByConnection.remove(it);
+ m_storageAreasByConnection.add({ connectionID, storageMapID }, WTFMove(area));
+ return;
+ }
- auto* transientLocalStorageNamespace = getOrCreateTransientLocalStorageNamespace(storageNamespaceID, WTFMove(topLevelOriginData));
+ auto& slot = m_storageAreasByConnection.add({ connectionID, storageMapID }, nullptr).iterator->value;
+ ASSERT(!slot);
- auto storageArea = transientLocalStorageNamespace->getOrCreateStorageArea(WTFMove(origin));
- storageArea->addListener(connectionID, storageMapID);
+ auto* transientLocalStorageNamespace = getOrCreateTransientLocalStorageNamespace(storageNamespaceID, WTFMove(topLevelOriginData));
- slot = WTFMove(storageArea);
- });
+ auto storageArea = transientLocalStorageNamespace->getOrCreateStorageArea(WTFMove(origin));
+ storageArea->addListener(connectionID, storageMapID);
+
+ slot = WTFMove(storageArea);
}
void StorageManager::createSessionStorageMap(IPC::Connection& connection, uint64_t storageMapID, uint64_t storageNamespaceID, SecurityOriginData&& securityOriginData)
{
- m_queue->dispatch([this, protectedThis = makeRef(*this), connectionID = connection.uniqueID(), storageMapID, storageNamespaceID, securityOriginData = securityOriginData.isolatedCopy()]() mutable {
- ASSERT(m_sessionStorageNamespaces.isValidKey(storageNamespaceID));
+ ASSERT(!RunLoop::isMain());
+ auto connectionID = connection.uniqueID();
+ ASSERT(m_sessionStorageNamespaces.isValidKey(storageNamespaceID));
- SessionStorageNamespace* sessionStorageNamespace = m_sessionStorageNamespaces.get(storageNamespaceID);
- if (!sessionStorageNamespace) {
- // We're getting an incoming message from the web process that's for session storage for a web page
- // that has already been closed, just ignore it.
- return;
- }
+ SessionStorageNamespace* sessionStorageNamespace = m_sessionStorageNamespaces.get(storageNamespaceID);
+ if (!sessionStorageNamespace) {
+ // We're getting an incoming message from the web process that's for session storage for a web page
+ // that has already been closed, just ignore it.
+ return;
+ }
- ASSERT(m_storageAreasByConnection.isValidKey({ connectionID, storageMapID }));
+ ASSERT(m_storageAreasByConnection.isValidKey({ connectionID, storageMapID }));
- auto& slot = m_storageAreasByConnection.add({ connectionID, storageMapID }, nullptr).iterator->value;
- ASSERT(!slot);
- ASSERT(sessionStorageNamespace->allowedConnections().contains(connectionID));
+ auto& slot = m_storageAreasByConnection.add({ connectionID, storageMapID }, nullptr).iterator->value;
+ ASSERT(!slot);
+ ASSERT(sessionStorageNamespace->allowedConnections().contains(connectionID));
- auto storageArea = sessionStorageNamespace->getOrCreateStorageArea(WTFMove(securityOriginData));
- storageArea->addListener(connectionID, storageMapID);
+ auto storageArea = sessionStorageNamespace->getOrCreateStorageArea(WTFMove(securityOriginData));
+ storageArea->addListener(connectionID, storageMapID);
- slot = WTFMove(storageArea);
- });
+ slot = WTFMove(storageArea);
}
void StorageManager::destroyStorageMap(IPC::Connection& connection, uint64_t storageMapID)
{
- m_queue->dispatch([this, protectedThis = makeRef(*this), connectionID = connection.uniqueID(), storageMapID]() mutable {
- std::pair<IPC::Connection::UniqueID, uint64_t> connectionAndStorageMapIDPair(connectionID, storageMapID);
- ASSERT(m_storageAreasByConnection.isValidKey(connectionAndStorageMapIDPair));
+ ASSERT(!RunLoop::isMain());
+ auto connectionID = connection.uniqueID();
- auto it = m_storageAreasByConnection.find(connectionAndStorageMapIDPair);
- if (it == m_storageAreasByConnection.end()) {
- // The connection has been removed because the last page was closed.
- return;
- }
+ std::pair<IPC::Connection::UniqueID, uint64_t> connectionAndStorageMapIDPair(connectionID, storageMapID);
+ ASSERT(m_storageAreasByConnection.isValidKey(connectionAndStorageMapIDPair));
- it->value->removeListener(connectionID, storageMapID);
+ auto it = m_storageAreasByConnection.find(connectionAndStorageMapIDPair);
+ if (it == m_storageAreasByConnection.end()) {
+ // The connection has been removed because the last page was closed.
+ return;
+ }
- // Don't remove session storage maps. The web process may reconnect and expect the data to still be around.
- if (it->value->isEphemeral())
- return;
+ it->value->removeListener(connectionID, storageMapID);
- m_storageAreasByConnection.remove(connectionAndStorageMapIDPair);
- });
-}
+ // Don't remove session storage maps. The web process may reconnect and expect the data to still be around.
+ if (it->value->isEphemeral())
+ return;
-static void didGetValues(IPC::Connection& connection, uint64_t storageMapID, const HashMap<String, String>& items, GetValuesCallback&& completionHandler)
-{
- RunLoop::main().dispatch([items = crossThreadCopy(items), completionHandler = WTFMove(completionHandler)]() mutable {
- completionHandler(items);
- });
+ m_storageAreasByConnection.remove(connectionAndStorageMapIDPair);
}
void StorageManager::getValues(IPC::Connection& connection, WebCore::SecurityOriginData&& securityOriginData, uint64_t storageMapID, uint64_t storageMapSeed, GetValuesCallback&& completionHandler)
{
- m_queue->dispatch([this, protectedThis = makeRef(*this), connection = makeRef(connection), securityOriginData = securityOriginData.isolatedCopy(), storageMapID, storageMapSeed, completionHandler = WTFMove(completionHandler)]() mutable {
- auto* storageArea = findStorageArea(connection.get(), storageMapID);
+ ASSERT(!RunLoop::isMain());
+ auto* storageArea = findStorageArea(connection, storageMapID);
- // This is a session storage area for a page that has already been closed. Ignore it.
- if (!storageArea)
- return didGetValues(connection.get(), storageMapID, { }, WTFMove(completionHandler));
+ // This is a session storage area for a page that has already been closed. Ignore it.
+ if (!storageArea)
+ return completionHandler({ });
- didGetValues(connection.get(), storageMapID, storageArea->items(), WTFMove(completionHandler));
- connection->send(Messages::StorageAreaMap::DidGetValues(storageMapSeed), storageMapID);
- });
+ completionHandler(storageArea->items());
+ connection.send(Messages::StorageAreaMap::DidGetValues(storageMapSeed), storageMapID);
}
void StorageManager::setItem(IPC::Connection& connection, WebCore::SecurityOriginData&& securityOriginData, uint64_t storageMapID, uint64_t sourceStorageAreaID, uint64_t storageMapSeed, const String& key, const String& value, const String& urlString)
{
- m_queue->dispatch([this, protectedThis = makeRef(*this), connection = makeRef(connection), securityOriginData = securityOriginData.isolatedCopy(), storageMapID, sourceStorageAreaID, storageMapSeed, key = key.isolatedCopy(), value = value.isolatedCopy(), urlString = urlString.isolatedCopy()]() mutable {
- auto* storageArea = findStorageArea(connection.get(), storageMapID);
+ ASSERT(!RunLoop::isMain());
+ auto* storageArea = findStorageArea(connection, storageMapID);
- // This is a session storage area for a page that has already been closed. Ignore it.
- if (!storageArea)
- return;
+ // This is a session storage area for a page that has already been closed. Ignore it.
+ if (!storageArea)
+ return;
- bool quotaError;
- storageArea->setItem(connection->uniqueID(), sourceStorageAreaID, key, value, urlString, quotaError);
- connection->send(Messages::StorageAreaMap::DidSetItem(storageMapSeed, key, quotaError), storageMapID);
- });
+ bool quotaError;
+ storageArea->setItem(connection.uniqueID(), sourceStorageAreaID, key, value, urlString, quotaError);
+ connection.send(Messages::StorageAreaMap::DidSetItem(storageMapSeed, key, quotaError), storageMapID);
}
void StorageManager::setItems(IPC::Connection& connection, uint64_t storageMapID, const HashMap<String, String>& items)
{
- m_queue->dispatch([this, protectedThis = makeRef(*this), connection = makeRef(connection), storageMapID, items = crossThreadCopy(items)]() mutable {
- if (auto* storageArea = findStorageArea(connection.get(), storageMapID))
- storageArea->setItems(items);
- });
+ ASSERT(!RunLoop::isMain());
+ if (auto* storageArea = findStorageArea(connection, storageMapID))
+ storageArea->setItems(items);
}
void StorageManager::removeItem(IPC::Connection& connection, WebCore::SecurityOriginData&& securityOriginData, uint64_t storageMapID, uint64_t sourceStorageAreaID, uint64_t storageMapSeed, const String& key, const String& urlString)
{
- m_queue->dispatch([this, protectedThis = makeRef(*this), connection = makeRef(connection), securityOriginData = securityOriginData.isolatedCopy(), storageMapID, sourceStorageAreaID, storageMapSeed, key = key.isolatedCopy(), urlString = urlString.isolatedCopy()]() mutable {
- auto* storageArea = findStorageArea(connection.get(), storageMapID);
+ ASSERT(!RunLoop::isMain());
+ auto* storageArea = findStorageArea(connection, storageMapID);
- // This is a session storage area for a page that has already been closed. Ignore it.
- if (!storageArea)
- return;
+ // This is a session storage area for a page that has already been closed. Ignore it.
+ if (!storageArea)
+ return;
- storageArea->removeItem(connection->uniqueID(), sourceStorageAreaID, key, urlString);
- connection->send(Messages::StorageAreaMap::DidRemoveItem(storageMapSeed, key), storageMapID);
- });
+ storageArea->removeItem(connection.uniqueID(), sourceStorageAreaID, key, urlString);
+ connection.send(Messages::StorageAreaMap::DidRemoveItem(storageMapSeed, key), storageMapID);
}
void StorageManager::clear(IPC::Connection& connection, WebCore::SecurityOriginData&& securityOriginData, uint64_t storageMapID, uint64_t sourceStorageAreaID, uint64_t storageMapSeed, const String& urlString)
{
- m_queue->dispatch([this, protectedThis = makeRef(*this), connection = makeRef(connection), securityOriginData = securityOriginData.isolatedCopy(), storageMapID, sourceStorageAreaID, storageMapSeed, urlString = urlString.isolatedCopy()]() mutable {
- auto* storageArea = findStorageArea(connection.get(), storageMapID);
+ ASSERT(!RunLoop::isMain());
+ auto* storageArea = findStorageArea(connection, storageMapID);
- // This is a session storage area for a page that has already been closed. Ignore it.
- if (!storageArea)
- return;
+ // This is a session storage area for a page that has already been closed. Ignore it.
+ if (!storageArea)
+ return;
- storageArea->clear(connection->uniqueID(), sourceStorageAreaID, urlString);
- connection->send(Messages::StorageAreaMap::DidClear(storageMapSeed), storageMapID);
- });
+ storageArea->clear(connection.uniqueID(), sourceStorageAreaID, urlString);
+ connection.send(Messages::StorageAreaMap::DidClear(storageMapSeed), storageMapID);
}
void StorageManager::waitUntilTasksFinished()
diff --git a/Source/WebKit/NetworkProcess/WebStorage/StorageManager.h b/Source/WebKit/NetworkProcess/WebStorage/StorageManager.h
index a5afada..4cdf501 100644
--- a/Source/WebKit/NetworkProcess/WebStorage/StorageManager.h
+++ b/Source/WebKit/NetworkProcess/WebStorage/StorageManager.h
@@ -31,6 +31,7 @@
#include <WebCore/StorageMap.h>
#include <wtf/Forward.h>
#include <wtf/Function.h>
+#include <wtf/HashCountedSet.h>
#include <wtf/HashSet.h>
#include <wtf/text/StringHash.h>
@@ -45,7 +46,7 @@
using GetValuesCallback = CompletionHandler<void(const HashMap<String, String>&)>;
-class StorageManager : public ThreadSafeRefCounted<StorageManager, WTF::DestructionThread::MainRunLoop> {
+class StorageManager : public IPC::Connection::WorkQueueMessageReceiver {
public:
static Ref<StorageManager> create(String&& localStorageDirectory);
~StorageManager();
@@ -111,6 +112,7 @@
HashMap<uint64_t, RefPtr<SessionStorageNamespace>> m_sessionStorageNamespaces;
HashMap<std::pair<IPC::Connection::UniqueID, uint64_t>, RefPtr<StorageArea>> m_storageAreasByConnection;
+ HashCountedSet<IPC::Connection::UniqueID> m_connections;
enum class State {
Running,
diff --git a/Source/WebKit/Platform/IPC/Connection.cpp b/Source/WebKit/Platform/IPC/Connection.cpp
index 8f69c87..019fbc8 100644
--- a/Source/WebKit/Platform/IPC/Connection.cpp
+++ b/Source/WebKit/Platform/IPC/Connection.cpp
@@ -307,21 +307,19 @@
{
ASSERT(RunLoop::isMain());
- m_connectionQueue->dispatch([protectedThis = makeRef(*this), messageReceiverName = WTFMove(messageReceiverName), workQueue = &workQueue, workQueueMessageReceiver]() mutable {
- ASSERT(!protectedThis->m_workQueueMessageReceivers.contains(messageReceiverName));
+ std::lock_guard<Lock> lock(m_workQueueMessageReceiversMutex);
+ ASSERT(!m_workQueueMessageReceivers.contains(messageReceiverName));
- protectedThis->m_workQueueMessageReceivers.add(messageReceiverName, std::make_pair(workQueue, workQueueMessageReceiver));
- });
+ m_workQueueMessageReceivers.add(messageReceiverName, std::make_pair(&workQueue, workQueueMessageReceiver));
}
void Connection::removeWorkQueueMessageReceiver(StringReference messageReceiverName)
{
ASSERT(RunLoop::isMain());
- m_connectionQueue->dispatch([protectedThis = makeRef(*this), messageReceiverName = WTFMove(messageReceiverName)]() mutable {
- ASSERT(protectedThis->m_workQueueMessageReceivers.contains(messageReceiverName));
- protectedThis->m_workQueueMessageReceivers.remove(messageReceiverName);
- });
+ std::lock_guard<Lock> lock(m_workQueueMessageReceiversMutex);
+ ASSERT(m_workQueueMessageReceivers.contains(messageReceiverName));
+ m_workQueueMessageReceivers.remove(messageReceiverName);
}
void Connection::dispatchWorkQueueMessageReceiverMessage(WorkQueueMessageReceiver& workQueueMessageReceiver, Decoder& decoder)
@@ -693,7 +691,7 @@
return;
}
- if (!m_workQueueMessageReceivers.isValidKey(message->messageReceiverName())) {
+ if (!WorkQueueMessageReceiverMap::isValidKey(message->messageReceiverName())) {
RefPtr<Connection> protectedThis(this);
StringReference messageReceiverNameReference = message->messageReceiverName();
String messageReceiverName(messageReceiverNameReference.isEmpty() ? "<unknown message receiver>" : String(messageReceiverNameReference.data(), messageReceiverNameReference.size()));
@@ -706,13 +704,8 @@
return;
}
- auto it = m_workQueueMessageReceivers.find(message->messageReceiverName());
- if (it != m_workQueueMessageReceivers.end()) {
- it->value.first->dispatch([protectedThis = makeRef(*this), workQueueMessageReceiver = it->value.second, decoder = WTFMove(message)]() mutable {
- protectedThis->dispatchWorkQueueMessageReceiverMessage(*workQueueMessageReceiver, *decoder);
- });
+ if (dispatchMessageToWorkQueueReceiver(message))
return;
- }
#if HAVE(QOS_CLASSES)
if (message->isSyncMessage() && m_shouldBoostMainThreadOnSyncMessage) {
@@ -987,14 +980,37 @@
handler(&decoder);
return;
}
+
m_client.didReceiveMessage(*this, decoder);
}
+bool Connection::dispatchMessageToWorkQueueReceiver(std::unique_ptr<Decoder>& message)
+{
+ std::lock_guard<Lock> lock(m_workQueueMessageReceiversMutex);
+ auto it = m_workQueueMessageReceivers.find(message->messageReceiverName());
+ if (it != m_workQueueMessageReceivers.end()) {
+ it->value.first->dispatch([protectedThis = makeRef(*this), workQueueMessageReceiver = it->value.second, decoder = WTFMove(message)]() mutable {
+ protectedThis->dispatchWorkQueueMessageReceiverMessage(*workQueueMessageReceiver, *decoder);
+ });
+ return true;
+ }
+ return false;
+}
+
void Connection::dispatchMessage(std::unique_ptr<Decoder> message)
{
+ ASSERT(RunLoop::isMain());
if (!isValid())
return;
+ // Messages to WorkQueueMessageReceivers are normally dispatched from the IPC WorkQueue. However, there is a race if
+ // a client adds itself as a WorkQueueMessageReceiver as a result of receiving an IPC message on the main thread.
+ // The message might have already been dispatched from the IPC WorkQueue to the main thread by the time the
+ // client registers itself as a WorkQueueMessageReceiver. To address this, we check again for messages receivers
+ // once the message arrives on the main thread.
+ if (dispatchMessageToWorkQueueReceiver(message))
+ return;
+
if (message->shouldUseFullySynchronousModeForTesting()) {
if (!m_fullySynchronousModeIsAllowedForTesting) {
m_client.didReceiveInvalidMessage(*this, message->messageReceiverName(), message->messageName());
diff --git a/Source/WebKit/Platform/IPC/Connection.h b/Source/WebKit/Platform/IPC/Connection.h
index d7e1d28..42f836d 100644
--- a/Source/WebKit/Platform/IPC/Connection.h
+++ b/Source/WebKit/Platform/IPC/Connection.h
@@ -262,6 +262,8 @@
std::unique_ptr<Decoder> waitForSyncReply(uint64_t syncRequestID, Seconds timeout, OptionSet<SendSyncOption>);
+ bool dispatchMessageToWorkQueueReceiver(std::unique_ptr<Decoder>&);
+
// Called on the connection work queue.
void processIncomingMessage(std::unique_ptr<Decoder>);
void processIncomingSyncReply(std::unique_ptr<Decoder>);
@@ -325,7 +327,9 @@
bool m_isConnected;
Ref<WorkQueue> m_connectionQueue;
- HashMap<StringReference, std::pair<RefPtr<WorkQueue>, RefPtr<WorkQueueMessageReceiver>>> m_workQueueMessageReceivers;
+ Lock m_workQueueMessageReceiversMutex;
+ using WorkQueueMessageReceiverMap = HashMap<StringReference, std::pair<RefPtr<WorkQueue>, RefPtr<WorkQueueMessageReceiver>>>;
+ WorkQueueMessageReceiverMap m_workQueueMessageReceivers;
unsigned m_inSendSyncCount;
unsigned m_inDispatchMessageCount;
diff --git a/Source/WebKit/WebProcess/WebStorage/StorageAreaMap.cpp b/Source/WebKit/WebProcess/WebStorage/StorageAreaMap.cpp
index dd743a9..bf0dc6c 100644
--- a/Source/WebKit/WebProcess/WebStorage/StorageAreaMap.cpp
+++ b/Source/WebKit/WebProcess/WebStorage/StorageAreaMap.cpp
@@ -181,7 +181,7 @@
WebProcess::singleton().ensureNetworkProcessConnection().connection().sendSync(Messages::StorageManager::GetValues(m_securityOrigin->data(), m_storageMapID, m_currentSeed), Messages::StorageManager::GetValues::Reply(values), 0);
m_storageMap = StorageMap::create(m_quotaInBytes);
- m_storageMap->importItems(values);
+ m_storageMap->importItems(WTFMove(values));
// We want to ignore all changes until we get the DidGetValues message.
m_hasPendingGetValues = true;
diff --git a/Source/WebKitLegacy/ChangeLog b/Source/WebKitLegacy/ChangeLog
index 803836a..060d346 100644
--- a/Source/WebKitLegacy/ChangeLog
+++ b/Source/WebKitLegacy/ChangeLog
@@ -1,3 +1,16 @@
+2019-07-16 Chris Dumez <cdumez@apple.com>
+
+ Speed up StorageManager::getValues()
+ https://bugs.webkit.org/show_bug.cgi?id=199812
+
+ Reviewed by Alex Christensen.
+
+ * Storage/StorageAreaImpl.cpp:
+ (WebKit::StorageAreaImpl::importItems):
+ * Storage/StorageAreaImpl.h:
+ * Storage/StorageAreaSync.cpp:
+ (WebKit::StorageAreaSync::performImport):
+
2019-07-12 Alex Christensen <achristensen@webkit.org>
Begin unifying WebKitLegacy sources
diff --git a/Source/WebKitLegacy/Storage/StorageAreaImpl.cpp b/Source/WebKitLegacy/Storage/StorageAreaImpl.cpp
index fc9562f..7da5c72 100644
--- a/Source/WebKitLegacy/Storage/StorageAreaImpl.cpp
+++ b/Source/WebKitLegacy/Storage/StorageAreaImpl.cpp
@@ -194,11 +194,11 @@
return m_storageMap->contains(key);
}
-void StorageAreaImpl::importItems(const HashMap<String, String>& items)
+void StorageAreaImpl::importItems(HashMap<String, String>&& items)
{
ASSERT(!m_isShutdown);
- m_storageMap->importItems(items);
+ m_storageMap->importItems(WTFMove(items));
}
void StorageAreaImpl::close()
diff --git a/Source/WebKitLegacy/Storage/StorageAreaImpl.h b/Source/WebKitLegacy/Storage/StorageAreaImpl.h
index 386d48d..58fcba0 100644
--- a/Source/WebKitLegacy/Storage/StorageAreaImpl.h
+++ b/Source/WebKitLegacy/Storage/StorageAreaImpl.h
@@ -67,7 +67,7 @@
void close();
// Only called from a background thread.
- void importItems(const HashMap<String, String>& items);
+ void importItems(HashMap<String, String>&& items);
// Used to clear a StorageArea and close db before backing db file is deleted.
void clearForOriginDeletion();
diff --git a/Source/WebKitLegacy/Storage/StorageAreaSync.cpp b/Source/WebKitLegacy/Storage/StorageAreaSync.cpp
index 17d4894..ce5c29b 100644
--- a/Source/WebKitLegacy/Storage/StorageAreaSync.cpp
+++ b/Source/WebKitLegacy/Storage/StorageAreaSync.cpp
@@ -347,7 +347,7 @@
return;
}
- m_storageArea->importItems(itemMap);
+ m_storageArea->importItems(WTFMove(itemMap));
markImported();
}
