chore: refactor poll_reserve logic to be a seperate function #7610
Conversation
|
Ahh just saw #7609 from mattsse. Apologies for the duplicate effort |
|
ah I forgot that there was an issue for this already -.- do you want to continue this following my docs on #7609? |
onbjerg
changed the title
|
@mattsse yeah would love to! |
|
@mattsse solved most of your Todos. Not the most confident on the PR, but would love more insight/context. Also would love if there's any tests you suggest I can add for this to make it airtight |
crates/exex/src/manager.rs
Outdated
| // it is a logic error for this to ever underflow since the manager manages the | ||
| // notification IDs | ||
| let notification_id = exex | ||
| .next_notification_id | ||
| .checked_sub(self.min_id) | ||
| .expect("exex expected notification ID outside the manager's range"); | ||
| if let Some(notification) = self.buffer.get(notification_id) { | ||
| debug!(exex.id, notification_id, "sent notification to exex"); | ||
| if let Poll::Ready(Err(err)) = exex.send(notification) { | ||
| // the channel was closed, which is irrecoverable for the manager | ||
| return Poll::Ready(Err(err.into())) | ||
| } |
There was a problem hiding this comment.
I think this is no longer accurate here, sending should only happen after we ensured there's capacity
There was a problem hiding this comment.
yes, we should not send notifications in 2 places in the loop, just the first when a slot in the channel has been reserved is needed
There was a problem hiding this comment.
Now senfing when Poll:Ready(Ok()) is matched
crates/exex/src/manager.rs
Outdated
| match reserve_result { | ||
| Poll::Ready(Ok(())) => { | ||
| // try send next notification to exex if we have it | ||
| let notification_id = exex.next_notification_id; |
There was a problem hiding this comment.
not exactly sure about this, the previous logic looks different.
crates/exex/src/manager.rs
Outdated
| match reserve_result { | ||
| Poll::Ready(Ok(())) => { | ||
| // try send next notification to exex if we have it | ||
| let notification_id = exex.next_notification_id; |
There was a problem hiding this comment.
this is incorrect, you need to restore the previous logic for getting the notification id - the notification id is not a 1:1 index into the buffer
There was a problem hiding this comment.
Makes sense. I changed this so we also don't send the notification twice, and moved previous logic from outside the loop in here.
crates/exex/src/manager.rs
Outdated
| /// Sends a slot in the `PollSender` channel and sends the notification if the slot was | ||
| /// successfully reserved. |
There was a problem hiding this comment.
please correct this comment - it just sends the notification, it does not do anything w slots or reservations
crates/exex/src/manager.rs
Outdated
| for idx in (0..self.exex_handles.len()).rev() { | ||
| let mut exex = self.exex_handles.swap_remove(idx); |
There was a problem hiding this comment.
please format this so it is correctly indented
crates/exex/src/manager.rs
Outdated
| let _ = self.finished_height.send(Some(finished_height)); | ||
| } | ||
|
|
||
| // only return pending if the buffer is not empty |
There was a problem hiding this comment.
this returns Poll::Pending if the buffer is empty, so the comment is not accurate
There was a problem hiding this comment.
Changed this to returning pending only when it is not empty and Poll:Ready(Ok()) otherwise
|
Hey guys, appreciate the comments. This gives a lot of context, thank you. I'll push an update to this today/tmrw |
mattsse
added
A-meta
|
Hey @mw2000, what's the status on this? Something I can help with? 😄 |
|
@onbjerg Added the changes that you and @mattsse discussed here. I think this one looks better, but I haven't been able to unit test it. Can you suggest what I can do to make sure the implementation is good? I think i understand high level what's happening, but would love to have tests that i can then run my implementation against. |
crates/exex/src/manager.rs
Outdated
| if !self.buffer.is_empty() { | ||
| return Poll::Pending; | ||
| } else { | ||
| return Poll::Ready(Ok(())); | ||
| } |
There was a problem hiding this comment.
this might immediately return Poll::Ready(Ok(())) once the manager is spawned, since the buffer is empty from the beginning
i think what we want is to have the entire inner body of poll be in a loop {} and only return Poll::Pending if the buffer is not empty. we should never return Poll::Ready(Ok(())) since this terminates the manager
see the comments at the bottom of the manager file in #7609
github-actions
bot
added
the
S-stale
mattsse
added
M-prevent-stale
|
reopened because this would be nice to have |
mw2000
requested review from
gakonst,
DaniPopes,
rkrasiuk,
rakita,
joshieDo,
Rjected and
emhane
as code owners
|
hey, I think something went wrong with merging or rebasing, since the diff is now huge, maybe it needs to be merged with main again? |
|
@Rjected yeah let me fix that, sorry picked this up after a while |
mw2000
force-pushed
the
mw2000/poll-reserve-to-exexhandle
branch
2 times, most recently
from
3490c1a
to
a2f152a
Compare
mw2000
force-pushed
the
mw2000/poll-reserve-to-exexhandle
branch
from
a2f152a
to
217703e
Compare
|
Fixed, moved this back to draft, apologies for the pings here |
mw2000
force-pushed
the
mw2000/poll-reserve-to-exexhandle
branch
4 times, most recently
from
96ac3a3
to
ef8c7eb
Compare
mw2000
force-pushed
the
mw2000/poll-reserve-to-exexhandle
branch
from
ef8c7eb
to
4ddf85c
Compare
|
Opening this back up. With the progress on the repo since the last time i took a crack at this issue, it looks like I don't need to add much beyond the poll reserve function itself. Feel free to close unmerged if not needed anymore. |
Separating the poll_reserve logic out into a new function on the ExExHandle level that the manager can call.
Fixes: #7570