Skip to content

google_grpc: attempt to reduce lock contention between completionThread() and onCompletedOps() #14777

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ggreenway merged 2 commits into from
Jan 22, 2021
Merged

google_grpc: attempt to reduce lock contention between completionThread() and onCompletedOps() #14777

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
47c86ee
google_grpc: attempt to reduce lock contention between completionThre…
antoniovicente Jan 21, 2021
77cd497
ASSERT that completed_ops_ is empty after the move
antoniovicente Jan 22, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 12 additions & 4 deletions source/common/grpc/google_async_client_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,12 +258,20 @@ 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<std::pair<GoogleAsyncTag::Operation, bool>> completed_ops;
{
Thread::LockGuard lock(completed_ops_lock_);
completed_ops = std::move(completed_ops_);

@jmarantz jmarantz Jan 21, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

completed_opts_.clear() here?

@htuch htuch Jan 22, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this implicit in move semantics?

@antoniovicente antoniovicente Jan 22, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels like a repeat of: https://github.com/envoyproxy/envoy/pull/14289/files/606f7af97a41567673a049b609df81cb3b7c99a9#r536419940

Added an ASSERT similar to the one in the dispatcher after a similar std::move.

@jmarantz jmarantz Jan 22, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh right; I remember. OK that sounds fine and faster.

}

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);
}
}
Expand Down