[MemoryPool] Add support for overdrafts in MemoryPool - #5057
Conversation
There was a problem hiding this comment.
Thanks for creating this PR @MohamedBassem. LGTM. I just had a question for what we are going to use the overdraft functionality. Probably, I'll learn in the following PRs.
| /// Returns the number of bytes in overdraft (used - capacity). Returns zero if | ||
| /// usage is still within capacity. | ||
| #[inline] | ||
| pub fn overdraft(&self) -> usize { |
There was a problem hiding this comment.
The overdraft is needed because we might consume a message from a channel that no longer fits into the free capacity of the memory budget?
There was a problem hiding this comment.
I hope the next PR made it a bit clear, but the idea is that to avoid parking any items that we successfully poll out of the stream, we'll just admit whatever gets polled (even if it takes us in overdraft) and then hold off polling any more items until we have capacity. Basically, we charge for memory after admission not before.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 503892d9c2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let decision = rpc::RpcHandler::handle(context, body).await; | ||
|
|
||
| match decision { | ||
| rpc::Decision::Propose(proposal) => permit.buffer_rpc_proposal(proposal, response_tx), |
There was a problem hiding this comment.
When a VQueue PauseInvocation reaches this branch, the pause proposal is only queued for the later NetworkService stream instead of being appended and fenced before on_pp_rpc_request returns. If an invoker effect for the same invocation is ready before that queued event is handled, handle_events can process LeaderEvent::Invoker first (leader_state.rs:451-469) while the old fencing token is still present, appending the effect ahead of the pause that was already accepted by the RPC path (pause_invocation.rs:63-72). That breaks the pause fencing invariant; handle ApplyAndFence synchronously or prioritize the queued pause before invoker effects.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d657d84332
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let prev = inner.used.fetch_add(size, Ordering::Relaxed); | ||
| debug_assert!( | ||
| prev.checked_add(size).is_some(), | ||
| "MemoryPool used counter overflowed" | ||
| ); |
There was a problem hiding this comment.
When a caller forces a reservation with size > usize::MAX - used, this fetch_add wraps the pool's used counter in release builds; the debug assertion runs only after the counter has already been corrupted. That can make available() become positive while oversized leases are still outstanding, allowing ordinary reservations through and later causing return_memory() to subtract from an invalid counter. Use a checked update or fail before mutating the counter.
Useful? React with 👍 / 👎.
b6137c7 to
ad620ab
Compare
This PR adds support for overdrafts in `MemoryPool` via the `force_reserve` API. This allows the memory pool to issue leases for more capacity than it holds. It also introduces a new `overdraft` API to query how much in the negative the memory pool is. Also, it introduces a new `wait_until_available` which waits until the pool is out of the overdraft mode (just as a notification without reserving anything).
2 participants
This PR adds support for overdrafts in
MemoryPoolvia theforce_reserveAPI. This allows the memory pool to issue leases for more capacity than it holds. It also introduces a newoverdraftAPI to query how much in the negative the memory pool is. Also, it introduces a newwait_until_availablewhich waits until the pool is out of the overdraft mode (just as a notification without reserving anything).Stack created with Sapling. Best reviewed with ReviewStack.