Service config parser to core configuration #28883
Conversation
ctiller
added
the
release notes: no
…a3a3 Automated fix for refs/heads/parserconfig
…669d Automated fix for refs/heads/parserconfig
…3e53 Automated fix for refs/heads/parserconfig
| #include <grpc/support/log.h> | ||
|
|
||
| namespace grpc_core { | ||
|
|
||
| std::atomic<Mutex*> CoreConfiguration::builder_mu_; |
There was a problem hiding this comment.
Why do we need this mutex now, when we didn't before?
If we are going to have a mutex here, do we still need atomics? Could we just use gpr_once_init() to set a non-atomic global Mutex* variable, and then just use that mutex for the remaining data?
Can we add lock annotations, so that it's clear to both readers and to compiler checks which variables are being guarded by this mutex?
Can I have a sentence in this comment that is not in the form of a question? :)
There was a problem hiding this comment.
We used to allow multiple builders to run concurrently and race to provide the finished configuration: since that happens relatively infrequently, and the configuration is relatively fast to construct, it seemed like a good trade-off.
What changes here is that the service config code stores some of its configuration in static variables (specifically the indices of the configured service config slots).
We could eliminate this mutex if we stored those indices as part of the configuration, which I considered... however that would mean some additional indirections I believe along the data path to load the core configuration and chase a pointer to get the relevant index. I guess if we had some scheme to locate those indices on the channel that might alleviate it and re-simplify this code.
There was a problem hiding this comment.
I'm not sure I stated this strongly enough:
I would be strongly in favor of removing this mutex and finding some other way of storing those indices.
| @@ -50,16 +50,12 @@ void grpc_resolver_sockaddr_shutdown(void); | |||
| void grpc_message_size_filter_init(void); | |||
There was a problem hiding this comment.
I think these two forward declarations are no longer needed.
…f0f9 Automated fix for refs/heads/parserconfig
|
@markdroth made the changes we'd talked about, ptal |
grpc-checks
bot
added
perf-change/medium
perf-change/none
and removed
perf-change/none
perf-change/medium
labels
| @@ -1,5 +1,5 @@ | |||
| // | |||
| // Copyright 2015 gRPC authors. | |||
| // Copyright 2022 gRPC authors. | |||
There was a problem hiding this comment.
This should probably retain its original 2015 copyright date.
|
|
||
| namespace grpc_core { | ||
|
|
||
| // TODO(roth): Consider stripping this down further to the completely minimal |
There was a problem hiding this comment.
I think we've now addressed this TODO. :)
| @@ -0,0 +1,127 @@ | |||
| // | |||
| // Copyright 2022 gRPC authors. | |||
There was a problem hiding this comment.
This should probably say 2015, since this code just moved around.
| } | ||
| // Save service config. | ||
| saved_service_config_ = std::move(service_config); | ||
| // Swap out the data used by GetChannelInfo(). | ||
| UniquePtr<char> lb_policy_name_owned(gpr_strdup(lb_policy_name)); | ||
| std::string lb_policy_name_owned = lb_policy_name; |
There was a problem hiding this comment.
How about just changing this method to take the lb_policy_name parameter as a std::string, so it doesn't need to convert internally?
copybara-service
bot
added
the
imported
@nicolasnoble