From 8a7ca18c480cc75d639fc2461152546a2537492d Mon Sep 17 00:00:00 2001 From: antonio Date: Fri, 22 Jan 2021 09:04:05 -0800 Subject: [PATCH] google_grpc: attempt to reduce lock contention between completionThread() and onCompletedOps() (#14777) Holding a stream's lock while running handleOpCompletion can result in the completion queue having to wait until the lock is released before adding messages on that stream to completed_ops_. In cases where the completion queue is shared across multiple gRPC streams, delivery of new messages on all streams is blocked until the lock held by the first stream while executing onCompletedOps. Signed-off-by: Antonio Vicente --- source/common/grpc/google_async_client_impl.cc | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/source/common/grpc/google_async_client_impl.cc b/source/common/grpc/google_async_client_impl.cc index 8f94df661e21..c3d1cdd89bc4 100644 --- a/source/common/grpc/google_async_client_impl.cc +++ b/source/common/grpc/google_async_client_impl.cc @@ -258,12 +258,22 @@ void GoogleAsyncStreamImpl::writeQueued() { } void GoogleAsyncStreamImpl::onCompletedOps() { - Thread::LockGuard lock(completed_ops_lock_); - while (!completed_ops_.empty()) { + // The items in completed_ops_ execute in the order they were originally added to the queue since + // both the post callback scheduled by the completionThread and the deferred deletion of the + // GoogleAsyncClientThreadLocal happen on the dispatcher thread. + std::deque> completed_ops; + { + Thread::LockGuard lock(completed_ops_lock_); + completed_ops = std::move(completed_ops_); + // completed_ops_ should be empty after the move. + ASSERT(completed_ops_.empty()); + } + + while (!completed_ops.empty()) { GoogleAsyncTag::Operation op; bool ok; - std::tie(op, ok) = completed_ops_.front(); - completed_ops_.pop_front(); + std::tie(op, ok) = completed_ops.front(); + completed_ops.pop_front(); handleOpCompletion(op, ok); } }