Refactor MemoryLimiter into PagedPool#1826
Merged
renanmagagnin merged 9 commits intoMay 13, 2026
Merged
Conversation
Signed-off-by: Renan Magagnin <renanmag@amazon.co.uk>
renanmagagnin
requested a deployment
to
PR integration tests
GitHub Actions
Waiting
passaro
requested changes
May 8, 2026
…limits are unchanged Signed-off-by: Renan Magagnin <renanmag@amazon.co.uk>
Signed-off-by: Renan Magagnin <renanmag@amazon.co.uk>
Signed-off-by: Renan Magagnin <renanmag@amazon.co.uk>
Signed-off-by: Renan Magagnin <renanmag@amazon.co.uk>
renanmagagnin
requested a deployment
to
PR integration tests
GitHub Actions
Waiting
…iter Signed-off-by: Renan Magagnin <renanmag@amazon.co.uk>
renanmagagnin
requested a deployment
to
PR integration tests
GitHub Actions
Waiting
Signed-off-by: Renan Magagnin <renanmag@amazon.co.uk>
renanmagagnin
requested a deployment
to
PR integration tests
GitHub Actions
Waiting
Signed-off-by: Renan Magagnin <renanmag@amazon.co.uk>
renanmagagnin
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
Signed-off-by: Renan Magagnin <renanmag@amazon.co.uk>
passaro
reviewed
May 13, 2026
Contributor
passaro
left a comment
passaro
left a comment
There was a problem hiding this comment.
Looks good. A few minor nits, but nothing really blocking.
Comment on lines
+12
to
+14
| use std::collections::hash_map::DefaultHasher; | ||
| use std::hash::{Hash, Hasher}; | ||
| use std::time::Instant; |
Comment on lines
+248
to
251
| fn pool_mem_reserved(&self, stats: &PoolStats) -> u64 { | ||
| // All pool buffer kinds are accounted for here. | ||
| self.pool.total_reserved_bytes() as u64 | ||
| stats.total_reserved_bytes() as u64 | ||
| } |
| } | ||
|
|
||
| /// Create a pool with [MINIMUM_MEM_LIMIT] as the memory limit. | ||
| pub fn new_with_candidate_sizes_minimally_limited(buffer_sizes: impl Into<Vec<usize>>) -> Self { |
Contributor
There was a problem hiding this comment.
We should consider a config/builder pattern instead of multiple constructors (for a later change).
| pub(crate) fn inner_limiter(&self) -> &MemoryLimiter { | ||
| &self.inner.limiter | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
Let's rethink some of the tests that need these methods (for later).
Comment on lines
+7
to
+8
| use crate::memory::BufferArea; | ||
| use crate::memory::PagedPool; |
Contributor
There was a problem hiding this comment.
nit:
Suggested change
| use crate::memory::BufferArea; | |
| use crate::memory::PagedPool; | |
| use crate::memory::{BufferArea, PagedPool}; |
Comment on lines
+1
to
6
| use crate::data_cache::DataCache; | ||
|
|
||
| use mountpoint_s3_client::ObjectClient; | ||
|
|
||
| use crate::{Runtime, mem_limiter::MemoryLimiter}; | ||
| use crate::{Runtime, memory::PagedPool}; | ||
|
|
renanmagagnin
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
passaro
approved these changes
May 13, 2026
Contributor
passaro
left a comment
passaro
left a comment
There was a problem hiding this comment.
Merge it. We can address the remaining comments in a follow up.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Moves MemoryLimiter inside PagedPool to improve the ownership model.
Before: MemoryLimiter owned a PagedPool field to read pool stats, while PagedPool called back into MemoryLimiter via a closure to decrement reservations on buffer allocation. Consumers (prefetcher, uploader, filesystem) had to pass both pool and Arc separately.
After: PagedPool owns the MemoryLimiter internally and calls it directly on buffer allocation — no callback needed. Consumers only need a PagedPool and call pool.reserve(), pool.try_reserve(), pool.available_mem(), etc. directly.
Key changes:
Next steps
Does this change impact existing behavior?
No. This is a pure refactor — no behavioral changes. The same memory limiting logic runs, the same budget checks apply, the same callbacks fire. Only the ownership structure and call sites changed.
Does this change need a changelog entry? Does it require a version change?
No. Internal refactor only — no public API changes, no user-facing behavior changes, no CLI changes.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and I agree to the terms of the Developer Certificate of Origin (DCO).