From fbcc8ab840aaa212934da0082a039038c8a81e53 Mon Sep 17 00:00:00 2001 From: Andreas Kling Date: Fri, 15 Feb 2019 11:43:43 +0100 Subject: [PATCH] WindowServer: Slurp all available client messages when checking them. We were reading one client message per client per event loop iteration. That was not very snappy. Make the sockets non-blocking and read() until there are no messages left. It would be even better to make as few calls to read() as possible to reduce context switching, but this is already a huge improvement. --- Kernel/Process.cpp | 33 ++++++++++++++++++--------------- WindowServer/WSMessageLoop.cpp | 24 +++++++++++++----------- WindowServer/WSMessageLoop.h | 3 ++- 3 files changed, 33 insertions(+), 27 deletions(-) diff --git a/Kernel/Process.cpp b/Kernel/Process.cpp index 39ce65120ac..2b80a641809 100644 --- a/Kernel/Process.cpp +++ b/Kernel/Process.cpp @@ -2304,7 +2304,7 @@ int Process::sys$listen(int sockfd, int backlog) return 0; } -int Process::sys$accept(int sockfd, sockaddr* address, socklen_t* address_size) +int Process::sys$accept(int accepting_socket_fd, sockaddr* address, socklen_t* address_size) { if (!validate_write_typed(address_size)) return -EFAULT; @@ -2312,28 +2312,31 @@ int Process::sys$accept(int sockfd, sockaddr* address, socklen_t* address_size) return -EFAULT; if (number_of_open_file_descriptors() >= m_max_open_file_descriptors) return -EMFILE; - int fd = 0; - for (; fd < (int)m_max_open_file_descriptors; ++fd) { - if (!m_fds[fd]) + int accepted_socket_fd = 0; + for (; accepted_socket_fd < (int)m_max_open_file_descriptors; ++accepted_socket_fd) { + if (!m_fds[accepted_socket_fd]) break; } - auto* descriptor = file_descriptor(sockfd); - if (!descriptor) + auto* accepting_socket_descriptor = file_descriptor(accepting_socket_fd); + if (!accepting_socket_descriptor) return -EBADF; - if (!descriptor->is_socket()) + if (!accepting_socket_descriptor->is_socket()) return -ENOTSOCK; - auto& socket = *descriptor->socket(); + auto& socket = *accepting_socket_descriptor->socket(); if (!socket.can_accept()) { - ASSERT(!descriptor->is_blocking()); + ASSERT(!accepting_socket_descriptor->is_blocking()); return -EAGAIN; } - auto client = socket.accept(); - ASSERT(client); - bool success = client->get_address(address, address_size); + auto accepted_socket = socket.accept(); + ASSERT(accepted_socket); + bool success = accepted_socket->get_address(address, address_size); ASSERT(success); - auto client_descriptor = FileDescriptor::create(move(client), SocketRole::Accepted); - m_fds[fd].set(move(client_descriptor)); - return fd; + auto accepted_socket_descriptor = FileDescriptor::create(move(accepted_socket), SocketRole::Accepted); + // NOTE: The accepted socket inherits fd flags from the accepting socket. + // I'm not sure if this matches other systems but it makes sense to me. + accepted_socket_descriptor->set_blocking(accepting_socket_descriptor->is_blocking()); + m_fds[accepted_socket_fd].set(move(accepted_socket_descriptor), m_fds[accepting_socket_fd].flags); + return accepted_socket_fd; } int Process::sys$connect(int sockfd, const sockaddr* address, socklen_t address_size) diff --git a/WindowServer/WSMessageLoop.cpp b/WindowServer/WSMessageLoop.cpp index de550148c27..c9f75bdfba0 100644 --- a/WindowServer/WSMessageLoop.cpp +++ b/WindowServer/WSMessageLoop.cpp @@ -40,7 +40,7 @@ int WSMessageLoop::exec() m_server_process->sys$unlink("/wsportal"); - m_server_fd = m_server_process->sys$socket(AF_LOCAL, SOCK_STREAM, 0); + m_server_fd = m_server_process->sys$socket(AF_LOCAL, SOCK_STREAM | SOCK_NONBLOCK, 0); ASSERT(m_server_fd >= 0); sockaddr_un address; address.sun_family = AF_LOCAL; @@ -244,9 +244,17 @@ void WSMessageLoop::wait_for_message() } WSClientConnection::for_each_client([&] (WSClientConnection& client) { if (bitmap.get(client.fd())) { - byte buffer[4096]; - ssize_t nread = m_server_process->sys$read(client.fd(), buffer, sizeof(WSAPI_ClientMessage)); - on_receive_from_client(client.client_id(), buffer, nread); + for (;;) { + WSAPI_ClientMessage message; + // FIXME: Don't go one message at a time, that's so much context switching, oof. + ssize_t nread = m_server_process->sys$read(client.fd(), &message, sizeof(WSAPI_ClientMessage)); + if (nread == 0) + break; + if (nread < 0) { + ASSERT_NOT_REACHED(); + } + on_receive_from_client(client.client_id(), message); + } } }); } @@ -313,18 +321,14 @@ void WSMessageLoop::notify_client_died(int client_id) post_message(client, make(client_id)); } -ssize_t WSMessageLoop::on_receive_from_client(int client_id, const byte* data, size_t size) +void WSMessageLoop::on_receive_from_client(int client_id, const WSAPI_ClientMessage& message) { // FIXME: This should not be necessary.. why is this necessary? while (!running()) Scheduler::yield(); LOCKER(m_lock); - WSClientConnection* client = WSClientConnection::ensure_for_client_id(client_id); - - ASSERT(size == sizeof(WSAPI_ClientMessage)); - auto& message = *reinterpret_cast(data); switch (message.type) { case WSAPI_ClientMessage::Type::CreateMenubar: post_message(client, make(client_id)); @@ -386,6 +390,4 @@ ssize_t WSMessageLoop::on_receive_from_client(int client_id, const byte* data, s post_message(client, make(client_id, message.window_id, message.value)); break; } - server_process().request_wakeup(); - return size; } diff --git a/WindowServer/WSMessageLoop.h b/WindowServer/WSMessageLoop.h index 51b064589d1..338bf6c63e5 100644 --- a/WindowServer/WSMessageLoop.h +++ b/WindowServer/WSMessageLoop.h @@ -9,6 +9,7 @@ class WSMessageReceiver; class Process; +struct WSAPI_ClientMessage; struct WSAPI_ServerMessage; class WSMessageLoop { @@ -31,7 +32,7 @@ public: int stop_timer(int timer_id); void post_message_to_client(int client_id, const WSAPI_ServerMessage&); - ssize_t on_receive_from_client(int client_id, const byte*, size_t); + void on_receive_from_client(int client_id, const WSAPI_ClientMessage&); static Process* process_from_client_id(int client_id); void notify_client_died(int client_id);