add support for reentrant callback group to EventsCBGExecutor#3178
add support for reentrant callback group to EventsCBGExecutor#3178armaho wants to merge 2 commits into
Conversation
Signed-off-by: Arman Hosseini <armahosseini878787@gmail.com>
skyegalaxy
requested review from
jmachowinski and
skyegalaxy
and removed request for
jmachowinski
skyegalaxy
left a comment
skyegalaxy
left a comment
There was a problem hiding this comment.
lgtm with green CI. will let @jmachowinski take a pass as well
| { | ||
| ready_callback_groups.erase(it); | ||
| } | ||
| return CBGScheduler::ExecutableEntityWithInfo{ |
There was a problem hiding this comment.
note to self to remove C++20 initialization when backporting this to Jazzy and Kilted specifically
|
actually @armaho can you add a test which exercises the reentrant path like we discussed at the client library WG meeting? |
|
Sure let me see how to test it. |
|
Tick the box to add this pull request to the merge queue (same as
|
…er EventCBGExecutor Signed-off-by: Arman Hosseini <armahosseini878787@gmail.com>
armaho
force-pushed
the
events_cbg_executor_reentrant
branch
from
554512d to
855d1b2
Compare
|
I just added a test covering the reentrant callback group behavior. Could you take another look when you have time? |
There was a problem hiding this comment.
Sorry for the late reply.
I wonder if it makes sense to introduce a second class of callback handle, providing a seperate logic.
In this case the ready queue would be something like
std::deque<std::variant<ReentrantCallbackGroupHandle *, MutalExclusiveCallbackGroupHandle *>
Even though this would lead to some code duplication, I think it would make the code easier and better understandable in the end. The current approach of selectively doing things made it hard to understand what is going on.
I also think the compiler might optimize the variant better.
| ready_cbg->get_type() == CallbackGroupType::MutuallyExclusive || | ||
| !ready_cbg->has_ready_entities()) | ||
| { | ||
| ready_callback_groups.erase(it); |
There was a problem hiding this comment.
This seems like a bad idea, I would rather delete it always and push it back later.
Otherwise we might get starvation on one callback group.
| if (type != CallbackGroupType::Reentrant) { | ||
| not_ready = false; | ||
| } |
There was a problem hiding this comment.
This is odd, this function is called after execution.
So you are basically blocking the callback group with this.
Note the inverted naming, not_ready = false actually means it is ready.
|
|
||
| // Reentrant callback groups might not be removed from the queue when one of | ||
| // their entities starts executing. | ||
| if (handle->get_type() == CallbackGroupType::Reentrant) { | ||
| bool already_in_queue = false; | ||
|
|
||
| for (auto it = ready_callback_groups.begin(); it != ready_callback_groups.end(); it++) { | ||
| if (*it == handle) { | ||
| already_in_queue = true; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| if (!already_in_queue) { | ||
| ready_callback_groups.push_back(handle); | ||
| } | ||
| } else { | ||
| ready_callback_groups.push_back(handle); | ||
| } |
There was a problem hiding this comment.
This is a big no go, you introduce O(n) characteristics by searching the ready list.
This code should actually not be needed as the callback group handle should hold the information
if it is in the queue or not.
3 participants
Description
Fixes #3175
Is this user-facing behavior change?
Did you use Generative AI?
No
Additional Information