Optimize IPC::Connection::SyncMessageState methods
https://bugs.webkit.org/show_bug.cgi?id=204890
Reviewed by Alex Christensen.
Optimize IPC::Connection::SyncMessageState methods. We are seeing lock contention on some (app launch)
benchmarks, resulting in the main thread yielding for 10ms.
* Platform/IPC/Connection.cpp:
(IPC::Connection::SyncMessageState): Make constructor private since this is a singleton class.
(IPC::Connection::ConnectionAndIncomingMessage): Add convenience dispatch() method.
(IPC::Connection::SyncMessageState::processIncomingMessage):
Drop the lock as early as possible, *before* calling RunLoop::main().dispatch().
(IPC::Connection::SyncMessageState::dispatchMessages):
Drop allowedConnection parameter and simplify the code a lot as a result. Only dispatchMessagesAndResetDidScheduleDispatchMessagesForConnection()
needed the pass an allowedConnection but having dispatchMessagesAndResetDidScheduleDispatchMessagesForConnection() call dispatchMessages() was
inefficient since it would cause us to grab the lock in dispatchMessagesAndResetDidScheduleDispatchMessagesForConnection() to update
m_didScheduleDispatchMessagesWorkSet, then release it, then grab the lock again in dispatchMessages() for m_messagesToDispatchWhileWaitingForSyncReply.
(IPC::Connection::SyncMessageState::dispatchMessagesAndResetDidScheduleDispatchMessagesForConnection):
Grab the lock only once to update m_didScheduleDispatchMessagesWorkSet and m_messagesToDispatchWhileWaitingForSyncReply, instead of doing it in 2
separate steps, each one taking the lock.
(IPC::Connection::waitForMessage):
(IPC::Connection::waitForSyncReply):
(IPC::Connection::dispatchSyncMessage):
stop passing a null allowedConnection when calling dispatchMessages().
git-svn-id: http://svn.webkit.org/repository/webkit/trunk@253183 268f45cc-cd09-0410-ab3c-d52691b4dbfc
diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
index fa56e97..5d086a6 100644
--- a/Source/WebKit/ChangeLog
+++ b/Source/WebKit/ChangeLog
@@ -1,5 +1,37 @@
2019-12-05 Chris Dumez <cdumez@apple.com>
+ Optimize IPC::Connection::SyncMessageState methods
+ https://bugs.webkit.org/show_bug.cgi?id=204890
+
+ Reviewed by Alex Christensen.
+
+ Optimize IPC::Connection::SyncMessageState methods. We are seeing lock contention on some (app launch)
+ benchmarks, resulting in the main thread yielding for 10ms.
+
+ * Platform/IPC/Connection.cpp:
+ (IPC::Connection::SyncMessageState): Make constructor private since this is a singleton class.
+ (IPC::Connection::ConnectionAndIncomingMessage): Add convenience dispatch() method.
+
+ (IPC::Connection::SyncMessageState::processIncomingMessage):
+ Drop the lock as early as possible, *before* calling RunLoop::main().dispatch().
+
+ (IPC::Connection::SyncMessageState::dispatchMessages):
+ Drop allowedConnection parameter and simplify the code a lot as a result. Only dispatchMessagesAndResetDidScheduleDispatchMessagesForConnection()
+ needed the pass an allowedConnection but having dispatchMessagesAndResetDidScheduleDispatchMessagesForConnection() call dispatchMessages() was
+ inefficient since it would cause us to grab the lock in dispatchMessagesAndResetDidScheduleDispatchMessagesForConnection() to update
+ m_didScheduleDispatchMessagesWorkSet, then release it, then grab the lock again in dispatchMessages() for m_messagesToDispatchWhileWaitingForSyncReply.
+
+ (IPC::Connection::SyncMessageState::dispatchMessagesAndResetDidScheduleDispatchMessagesForConnection):
+ Grab the lock only once to update m_didScheduleDispatchMessagesWorkSet and m_messagesToDispatchWhileWaitingForSyncReply, instead of doing it in 2
+ separate steps, each one taking the lock.
+
+ (IPC::Connection::waitForMessage):
+ (IPC::Connection::waitForSyncReply):
+ (IPC::Connection::dispatchSyncMessage):
+ stop passing a null allowedConnection when calling dispatchMessages().
+
+2019-12-05 Chris Dumez <cdumez@apple.com>
+
PageConfiguration::dragClient should use a smart pointer
https://bugs.webkit.org/show_bug.cgi?id=204816
diff --git a/Source/WebKit/Platform/IPC/Connection.cpp b/Source/WebKit/Platform/IPC/Connection.cpp
index 62fa039..92bcdd4 100644
--- a/Source/WebKit/Platform/IPC/Connection.cpp
+++ b/Source/WebKit/Platform/IPC/Connection.cpp
@@ -76,7 +76,6 @@
public:
static SyncMessageState& singleton();
- SyncMessageState();
~SyncMessageState() = delete;
void wakeUpClientRunLoop()
@@ -93,12 +92,15 @@
// waiting for a reply to a synchronous message.
bool processIncomingMessage(Connection&, std::unique_ptr<Decoder>&);
- // Dispatch pending sync messages. if allowedConnection is not null, will only dispatch messages
- // from that connection and put the other messages back in the queue.
- void dispatchMessages(Connection* allowedConnection);
+ // Dispatch pending sync messages.
+ void dispatchMessages();
private:
- void dispatchMessageAndResetDidScheduleDispatchMessagesForConnection(Connection&);
+ friend class LazyNeverDestroyed<Connection::SyncMessageState>;
+ SyncMessageState() = default;
+
+ // Dispatch pending sync messages for given connection.
+ void dispatchMessagesAndResetDidScheduleDispatchMessagesForConnection(Connection&);
BinarySemaphore m_waitForSyncReplySemaphore;
@@ -111,6 +113,11 @@
struct ConnectionAndIncomingMessage {
Ref<Connection> connection;
std::unique_ptr<Decoder> message;
+
+ void dispatch()
+ {
+ connection->dispatchMessage(WTFMove(message));
+ }
};
Vector<ConnectionAndIncomingMessage> m_messagesToDispatchWhileWaitingForSyncReply;
};
@@ -127,10 +134,6 @@
return syncMessageState;
}
-Connection::SyncMessageState::SyncMessageState()
-{
-}
-
bool Connection::SyncMessageState::processIncomingMessage(Connection& connection, std::unique_ptr<Decoder>& message)
{
switch (message->shouldDispatchMessageWhenWaitingForSyncReply()) {
@@ -144,18 +147,17 @@
break;
}
- ConnectionAndIncomingMessage connectionAndIncomingMessage { connection, WTFMove(message) };
-
+ bool shouldDispatch;
{
std::lock_guard<Lock> lock(m_mutex);
-
- if (m_didScheduleDispatchMessagesWorkSet.add(&connection).isNewEntry) {
- RunLoop::main().dispatch([this, protectedConnection = Ref<Connection>(connection)]() mutable {
- dispatchMessageAndResetDidScheduleDispatchMessagesForConnection(protectedConnection);
- });
- }
+ shouldDispatch = m_didScheduleDispatchMessagesWorkSet.add(&connection).isNewEntry;
+ m_messagesToDispatchWhileWaitingForSyncReply.append(ConnectionAndIncomingMessage { connection, WTFMove(message) });
+ }
- m_messagesToDispatchWhileWaitingForSyncReply.append(WTFMove(connectionAndIncomingMessage));
+ if (shouldDispatch) {
+ RunLoop::main().dispatch([this, protectedConnection = makeRef(connection)]() mutable {
+ dispatchMessagesAndResetDidScheduleDispatchMessagesForConnection(protectedConnection);
+ });
}
wakeUpClientRunLoop();
@@ -163,28 +165,38 @@
return true;
}
-void Connection::SyncMessageState::dispatchMessages(Connection* allowedConnection)
+void Connection::SyncMessageState::dispatchMessages()
{
ASSERT(RunLoop::isMain());
Vector<ConnectionAndIncomingMessage> messagesToDispatchWhileWaitingForSyncReply;
-
{
std::lock_guard<Lock> lock(m_mutex);
m_messagesToDispatchWhileWaitingForSyncReply.swap(messagesToDispatchWhileWaitingForSyncReply);
}
+ for (auto& connectionAndIncomingMessage : messagesToDispatchWhileWaitingForSyncReply)
+ connectionAndIncomingMessage.dispatch();
+}
+
+void Connection::SyncMessageState::dispatchMessagesAndResetDidScheduleDispatchMessagesForConnection(Connection& connection)
+{
+ ASSERT(RunLoop::isMain());
+
+ Vector<ConnectionAndIncomingMessage> messagesToDispatchWhileWaitingForSyncReply;
+ {
+ std::lock_guard<Lock> lock(m_mutex);
+ ASSERT(m_didScheduleDispatchMessagesWorkSet.contains(&connection));
+ m_didScheduleDispatchMessagesWorkSet.remove(&connection);
+ m_messagesToDispatchWhileWaitingForSyncReply.swap(messagesToDispatchWhileWaitingForSyncReply);
+ }
+
Vector<ConnectionAndIncomingMessage> messagesToPutBack;
-
for (auto& connectionAndIncomingMessage : messagesToDispatchWhileWaitingForSyncReply) {
- if (allowedConnection && allowedConnection != connectionAndIncomingMessage.connection.ptr()) {
- // This incoming message belongs to another connection and we don't want to dispatch it now
- // so mark it to be put back in the message queue.
+ if (&connection == connectionAndIncomingMessage.connection.ptr())
+ connectionAndIncomingMessage.dispatch();
+ else
messagesToPutBack.append(WTFMove(connectionAndIncomingMessage));
- continue;
- }
-
- connectionAndIncomingMessage.connection->dispatchMessage(WTFMove(connectionAndIncomingMessage.message));
}
if (!messagesToPutBack.isEmpty()) {
@@ -194,17 +206,6 @@
}
}
-void Connection::SyncMessageState::dispatchMessageAndResetDidScheduleDispatchMessagesForConnection(Connection& connection)
-{
- {
- std::lock_guard<Lock> lock(m_mutex);
- ASSERT(m_didScheduleDispatchMessagesWorkSet.contains(&connection));
- m_didScheduleDispatchMessagesWorkSet.remove(&connection);
- }
-
- dispatchMessages(&connection);
-}
-
// Represents a sync request for which we're waiting on a reply.
struct Connection::PendingSyncReply {
// The request ID.
@@ -550,7 +551,7 @@
// Now wait for it to be set.
while (true) {
// Handle any messages that are blocked on a response from us.
- SyncMessageState::singleton().dispatchMessages(nullptr);
+ SyncMessageState::singleton().dispatchMessages();
std::unique_lock<Lock> lock(m_waitForMessageMutex);
@@ -631,7 +632,7 @@
bool timedOut = false;
while (!timedOut) {
// First, check if we have any messages that we need to process.
- SyncMessageState::singleton().dispatchMessages(nullptr);
+ SyncMessageState::singleton().dispatchMessages();
{
LockHolder locker(m_syncReplyStateMutex);
@@ -915,7 +916,7 @@
RELEASE_ASSERT(unwrappedDecoder);
processIncomingMessage(WTFMove(unwrappedDecoder));
- SyncMessageState::singleton().dispatchMessages(nullptr);
+ SyncMessageState::singleton().dispatchMessages();
} else {
// Hand off both the decoder and encoder to the client.
m_client.didReceiveSyncMessage(*this, decoder, replyEncoder);
