All instances of exec_ctx being passed around in src/core removed#13058
Merged
yashykt merged 33 commits intogrpc:masterfrom Dec 6, 2017
Merged
All instances of exec_ctx being passed around in src/core removed#13058yashykt merged 33 commits intogrpc:masterfrom
yashykt merged 33 commits intogrpc:masterfrom
Conversation
src/core. exec_ctx is now a thread_local pointer of type ExecCtx instead of grpc_exec_ctx which is initialized whenever ExecCtx is instantiated. ExecCtx also keeps track of the previous exec_ctx so that nesting of exec_ctx is allowed. This means that there is only one exec_ctx being used at any time. Also, grpc_exec_ctx_finish is called in the destructor of the object, and the previous exec_ctx is restored to avoid breaking current functionality. The code still explicitly calls grpc_exec_ctx_finish because removing all such instances causes the code to break.
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
Member
Author
|
@jtattermusch https://sponge.corp.google.com/target?id=9545da84-c304-4e33-99a4-e31b146d6b79&target=github/grpc/csharp_macos_dbg_native&searchFor=&show=FAILED&sortBy=STATUS The tests (even more than earlier) still seem to be failing. It still looks like missing grpc_init to me. Your PR to fix these will still be needed. |
|
|
Member
Author
|
@ctiller friendly ping for approval. I've taken the pony and I want to land this tomorrow. (All the requested changes are done. The current failures are only csharp failures which Jan is going to fix in a separate PR) :) |
|
|
|
|
ctiller
reviewed
Dec 5, 2017
| } | ||
| gpr_timers_global_destroy(); |
Member
There was a problem hiding this comment.
Ordering this before iomgr shutdown seems risky: have you tried a basicprof build?
Member
Author
There was a problem hiding this comment.
That's blank, so wouldn't have affected any tests but something could have gone wrong in the future. Moved it back.
ctiller
reviewed
Dec 5, 2017
| } | ||
| gpr_timers_global_destroy(); | ||
| grpc_tracer_shutdown(); |
Member
There was a problem hiding this comment.
Seems risky to move before iomgr shutdown: iomgr certainly uses tracers
Member
Author
There was a problem hiding this comment.
That's blank, so wouldn't have affected any tests but something could have gone wrong in the future. Moved it back.That's blank, so wouldn't have affected any tests but something could have gone wrong in the future. Moved it back.
Merged
ctiller
approved these changes
Dec 5, 2017
Member
ctiller
left a comment
ctiller
left a comment
There was a problem hiding this comment.
Needs a merge, and there's a couple of things to address in the shutdown ordering prior to submission.
Otherwise, LGTM - nice work!
|
|
|
|
|
|
Member
Author
|
All green! Thanks for reviewing Craig! Pulling the trigger on this |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
THIS IS READY FOR REVIEW.
Background
grpc_exec_ctx is like a bag of data that lives on the stack, is private to each thread and is passed around as a function parameter.
It is used for things like scheduling closures and caching the current time.
What’s Changing
As part of the C++ization effort of gRPC src/core, grpc_exec_ctx will now be a class named ExecCtx which lives in the namespace grpc_core.
At any point of time for a thread, there will be only one active exec_ctx which has to be allocated on the stack by user code. The pointer to this instance will be stored in a thread-local variable. This means that exec_ctx does not need to be passed around.
Also, the work that was previously done in grpc_exec_ctx_finish() which would complete all the work in closures, will now be done in the destructor. So, there is no need for grpc_exec_ctx_finish() anymore.
Example :-
void func(grpc_exec_ctx *exec_ctx) {
// func body
grpc_millis time = grpc_exec_ctx_now(exec_ctx);
// rest of the function
}
void func1() {
grpc_exec_ctx exec_ctx = GRPC_EXEC_CTX_INIT;
func(&exec_ctx);
}
This would change to
void func() {
// func body
Grpc_millis time = grpc_core::ExecCtx::Get()->Now();
// rest of the function
}
void func1() {
// The default constructor is similar to initializing
// exec_ctx with GRPC_EXEC_CTX_INIT
grpc_core::ExecCtx exec_ctx;
func(); // No need to pass exec_ctx
// No need to call finish
}
Special Case
There are cases in gRPC where multiple execution contexts exist on the same stack. We should be able to remove them, but for now the approach is to save the old exec_ctx and make the new exec_ctx as the current execution context which will be the one pointed to by the thread_local variable.
When the current execution context goes out of scope, the previous execution context will be loaded, so as not to break any current functionality.