Enable use of pollset_set for a channel call #6190
Conversation
dgquintas
force-pushed
the
lb_pollset_propagation
branch
2 times, most recently
from
d8eb13d to
1dcba6c
Compare
dgquintas
force-pushed
the
lb_pollset_propagation
branch
from
1dcba6c to
4afce7e
Compare
| size_t count = call_stack->count; | ||
| grpc_call_element *call_elems; | ||
| char *user_data; | ||
| size_t i; | ||
|
|
||
| GPR_ASSERT(!(pollset != NULL && or_pollset_set != NULL)); |
There was a problem hiding this comment.
makes my brain bleed: rename parameter? (I know I suggested the name)
There was a problem hiding this comment.
I've always kind of liked:
GPR_ASSERT((a == NULL) + (b == NULL) == 1);
There was a problem hiding this comment.
Done and done. The new name isn't brilliant either, but...
dgquintas
force-pushed
the
lb_pollset_propagation
branch
5 times, most recently
from
178aaa0 to
870860c
Compare
dgquintas
force-pushed
the
lb_pollset_propagation
branch
from
870860c to
e5c485d
Compare
| @@ -68,6 +68,7 @@ void grpc_subchannel_call_holder_init( | |||
| holder->waiting_ops_capacity = 0; | |||
| holder->creation_phase = GRPC_SUBCHANNEL_CALL_HOLDER_NOT_CREATING; | |||
| holder->owning_call = owning_call; | |||
| holder->pollset_set = grpc_pollset_set_create(); | |||
There was a problem hiding this comment.
This is going to be quite bad for performance. The typical case is that we just want to set a pollset and not go ahead and create a new big container of object. Suggest bifurcating here and only creating the pollset_set when necessary.
For some definition of "smart"... client_channel simply passes along pollset/pollset_set, removing the need to instantiate a pollset_set in the subchannel_call_holder: it's now up to the LB policies to handle the pollset/pollset_set.
|
Needs a merge. Looking at the code now I'm a bit worried about pops too: the variable name is ambiguous as to whether it represents a grpc_pops, or a pointer to some operations. grpc_polling_entity? Apologies for the bikeshed. |
dgquintas
force-pushed
the
lb_pollset_propagation
branch
from
d24301c to
2a50dfe
Compare
|
No worries :) I'm getting really good at grep+xargs+sed Done. PTAL. |
| enum pops_tag { POPS_NONE, POPS_POLLSET, POPS_POLLSET_SET } tag; | ||
| } grpc_polling_entity; | ||
|
|
||
| grpc_polling_entity grpc_pops_create_from_pollset_set( |
There was a problem hiding this comment.
We probably want to rename these functions for consistency also
|
LGTM... feel free to merge on green. |
|
Are we 100% the ruby failure is unrelated? https://grpc-testing.appspot.com/job/gRPC_pull_requests/9612/config=opt,language=ruby,platform=linux/testReport/junit/(root)/tests/tools_run_tests_run_ruby_sh/ |
|
Re-running to make sure |
|
FWIW, a local run under docker passes fine on my machine (yes, anecdata, let's see what jenkins thinks). |
|
The current jenkins run (without any further changes to this PR) is passing for ruby (https://grpc-testing.appspot.com/job/gRPC_pull_requests/9633/). There are a couple red exclamation marks for python/windows due to infrastructure hiccups. I'd argue that this run plus the previous one show this PR is good to be merged. |
| @@ -278,7 +278,8 @@ static int refill_queue(shard_type *shard, gpr_timespec now) { | |||
| return !grpc_timer_heap_is_empty(&shard->heap); | |||
| } | |||
|
|
|||
| /* This pops the next non-cancelled timer with deadline <= now from the queue, | |||
| /* This pollent the next non-cancelled timer with deadline <= now from the | |||
There was a problem hiding this comment.
Ugh, yes. The perils of global sed search-and-replace. I'll send a fixing PR momentarily.
There was a problem hiding this comment.
Done! #6832 Thanks!
No description provided.