Align read window with part boundaries#1618
Closed
vladem wants to merge 3 commits into
Closed
Conversation
vladem
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
vladem
had a problem deploying
to
PR integration tests
GitHub Actions
Failure
vladem
had a problem deploying
to
PR integration tests
GitHub Actions
Failure
vladem
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
vladem
had a problem deploying
to
PR integration tests
GitHub Actions
Failure
vladem
had a problem deploying
to
PR integration tests
GitHub Actions
Failure
vladem
had a problem deploying
to
PR integration tests
GitHub Actions
Failure
vladem
had a problem deploying
to
PR integration tests
GitHub Actions
Failure
vladem
had a problem deploying
to
PR integration tests
GitHub Actions
Failure
vladem
had a problem deploying
to
PR integration tests
GitHub Actions
Failure
vladem
had a problem deploying
to
PR integration tests
GitHub Actions
Failure
vladem
had a problem deploying
to
PR integration tests
GitHub Actions
Failure
vladem
had a problem deploying
to
PR integration tests
GitHub Actions
Failure
vladem
had a problem deploying
to
PR integration tests
GitHub Actions
Failure
vladem
had a problem deploying
to
PR integration tests
GitHub Actions
Failure
vladem
had a problem deploying
to
PR integration tests
GitHub Actions
Failure
vladem
had a problem deploying
to
PR integration tests
GitHub Actions
Failure
vladem
had a problem deploying
to
PR integration tests
GitHub Actions
Failure
vladem
had a problem deploying
to
PR integration tests
GitHub Actions
Failure
vladem
had a problem deploying
to
PR integration tests
GitHub Actions
Failure
vladem
had a problem deploying
to
PR integration tests
GitHub Actions
Failure
vladem
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
vladem
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
vladem
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
vladem
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
vladem
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
vladem
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
vladem
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
vladem
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
vladem
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
vladem
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
vladem
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
vladem
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
vladem
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
vladem
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
vladem
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
vladem
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
vladem
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
vladem
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
Signed-off-by: Vlad Volodkin <vlaad@amazon.com>
Signed-off-by: Vlad Volodkin <vlaad@amazon.com>
Signed-off-by: Vlad Volodkin <vlaad@amazon.co.uk>
vladem
force-pushed
the
aligned-read-window-end
branch
from
c526574 to
eea0347
Compare
vladem
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
passaro
reviewed
Oct 20, 2025
passaro
left a comment
passaro
left a comment
Contributor
There was a problem hiding this comment.
General concerns:
- The fix is quite fragile, since it requires the
BackpressureControllerto know what thePartStreamwill do with the window increment. - As you noted, the first consequence is that there is no way to make it work with the cache.
- Less importantly, the mock client becomes even more complicated.
Let's discuss in person whether there are other viable options.
| let new_read_window_end_offset = Self::round_up_to_part_boundary( | ||
| new_read_window_end_offset, | ||
| self.second_request_start, | ||
| self.min_read_window_size as u64, |
Contributor
There was a problem hiding this comment.
We are assuming this is actually the read part size, right? We should make it explicit.
| // NOTE: in the end of the object we still have up to "part_size" of unaccounted memory. | ||
| // For more accurate memory limiting we could reserve in "full parts", but that would make | ||
| // release more complicated. So accept this inaccuracy, and reserve only till `request_end_offset`. | ||
| let new_read_window_end_offset = new_read_window_end_offset.min(self.request_end_offset); |
Contributor
There was a problem hiding this comment.
Try to avoid shadowing here.
| .saturating_add(self.preferred_read_window_size as u64) | ||
| .min(self.request_end_offset); | ||
| .saturating_add(self.preferred_read_window_size as u64); | ||
| let new_read_window_end_offset = Self::round_up_to_part_boundary( |
Contributor
There was a problem hiding this comment.
Is this the right place? Or should we align in scale_up and scale_down, where we already modify preferred_read_window_size?
Contributor
Author
There was a problem hiding this comment.
With the existing incrementing logic, I think, it is the right place. Above this line we "blindly" adding preferred_read_window_size to new_read_window_end_offset. Having a fixed preferred_read_window_size won't work for arbitrary new_read_window_end_offset, which is dictated by the reading side (e.g. we may need to increment the window at offset 4,194,305 or 4,194,306 depending on how much data was read).
Contributor
Author
|
closing in favour of #1671 |
github-merge-queue Bot
pushed a commit
that referenced
this pull request
Dec 22, 2025
…t_size` (#1707) Re-created #1618, in this PR: - we align read window end to the part boundary for the second request (see `round_up_to_part_boundary` method); - we update mock client to allow testing of this change; - we add `PrefetcherConfig::initial_request_size` field and use it in `mount_from_config.rs` example. ### Does this change impact existing behavior? In a memory constrained environment, this may result in smaller read window sizes and less memory consumption. ### Does this change need a changelog entry? Does it require a version change? Minor version change and a change log to `mountpoint-s3-fs`, will add later. Patch version change to the `mountpoint-s3-fs-client`. --- 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)](https://developercertificate.org/). --------- Signed-off-by: Vlad Volodkin <vlaad@amazon.com> Signed-off-by: Vlad Volodkin <vlaad@amazon.co.uk> Co-authored-by: Vlad Volodkin <vlaad@amazon.com> Co-authored-by: Vlad Volodkin <vlaad@amazon.co.uk>
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.
Memory limiter currently doesn’t account for the “rounded up” memory by CRT, which means up to
8MiBof unaccounted memory per file handle. Based on this flaw in the memory model, limiter may choose larger windows, while there is actually no free memory for those. We should “round up” the read window to the next part boundary on our side. The metrics3.client.read_window_excess_bytesmay be used to track the amount of “rounded up” memory, not accounted in the memory limiter.To test the change, PR introduces
read_window_increment_failedflag toMockClientand adds abasic_read_test_mock_largetest.Note that this fix doesn't apply to
CachingPartStream: for this streamBackpressureControlleris not aware ofsecond_request_startoffset, which is required for the alignment.Does this change impact existing behavior?
In a memory constrained environment, this may result in smaller read window sizes and less memory consumption.
Does this change need a changelog entry? Does it require a version change?
Patch version change and a change log to
mountpoint-s3-fs, will add later.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).