-
Notifications
You must be signed in to change notification settings - Fork 10.6k
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.
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
Do not call grpc_init() for per-call-completion-queues created by a C++ sync server #10348
Conversation
synchronous server
synchronous server
} | ||
|
||
private: | ||
bool grpc_init_called; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Member variable should end with underscore
Beside the minor comment above, this seems LGTM-able. Can you put benchmark results about this into this PR description? |
@vjpai, Did the changes you requested. PTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
Issue: #9261
Before this PR, every C++
CompletionQueue
object creation ended up callinggrpc_init
which acquires a mutexg_init_mu
to increment the number of grpc-init invocations.This is ok if the number of completion queues being created was small. However, in case of C++ Synchronous server, a completion queue is create for every call. This ends up making
g_init_mu
one of top created mutexes in sync servers.This PR makes calling
grpc_init
optional when creating C++CompletionQueue
objects. It is always set totrue
by default but if a C++CompletionQueue
is being created by passing agrpc_completion_queue *
pointer i.e the following constructor (which is what Sync C++ server calls when creating a per-call completion queue), it is okay to NOT callgrpc_init
.This change is safe because if we got a
grpc_completion_queue *
structure, we MUST have already calledgrpc_init()
.Benchmark results
I added a new benchmark BM_CreateDestroyCpp2 (didn't previously include it as a part of the PR because I thought it is trivial - but on a second thought, i think it is a good idea :))
Locks are down from 3 to 1
On master (with BM_CreateDestroyCpp2 temporarily ported):
On this PR branch: