add support for reentrant callback group to EventsCBGExecutor#3178
add support for reentrant callback group to EventsCBGExecutor#3178armaho wants to merge 3 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.
|
|
||
| // 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.
There was a problem hiding this comment.
I wasn't able to find anything in the handle itself that indicates whether it's in the queue or not. There was an idle field, but I had trouble using it to determine that. I ended up adding an in_queue field to the handle instead, which is simple to understand and implement. What do you think about that?
|
@armaho - do you have bandwidth to respond to the feedback / still want to work on this? |
|
@skyegalaxy Yes. Sorry I’ve been busy I will address the problems of current code and check out that variant idea tomorrow and get back to you with the results. |
Signed-off-by: Arman Hosseini <armahosseini878787@gmail.com>
armaho
force-pushed
the
events_cbg_executor_reentrant
branch
from
cfebaef to
490c6b0
Compare
|
I also think that the current implementation of EventCBGExecutor is hard to understand. At least, it was for me. But I don't think that creating a separate handle for reentrant cbgs would help that much. Right now, the only conditional piece of code within the CallbackGroupHandle itself is inside void mark_as_executing()
{
if (type != CallbackGroupType::Reentrant) {
not_ready = true;
}
}Which is not hard to understand, and I don't think that creating a separate handle would fundamentally reduce the conditional logic elsewhere in the code. To me, it doesn’t seem worth the amount of code duplication it would introduce. That said, you know this codebase much better than I do, so I'll go in whichever direction you think is best. |
|
The commit looks good so far. I started playing around with the API to see if splitting the handle base class makes it more readable. I'll report back next week. Have a nice weekend. |
3 participants
Description
Fixes #3175
Is this user-facing behavior change?
Did you use Generative AI?
No
Additional Information