Fix Consistent-Hash Memory Leak #151
Merged
Conversation
This file contains 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
Based upon how bytes.Buffer works, it will continue to grow the underlying slice while reading data and add additional headroom to ensure that there is enough space, this means that when we allocate the bytes.Buffer to a small amount (minChunkSize()) the growth occurs regularly on any reads that push beyond the initial slice. Even at a minChunkSize of ~150M, when downloading SDXL we end up with a balloon of an additional ~10GiB allocated. This has resulted in SigKill (OOMKILL) of pget in lower-ram systems. This does not address the overall memory allocation requirements of pget, which still generally encompasses the entire file size. An additional pass will need to be done to ensure the consumer can read and the buffer can be GC'd to minimize overall memory allocation. This only surfaced in consistent-hash mode as buffer mode was properly setting the buffered reader to the chunkSize by calculating end - start +1. Consistent Hash only used m.minChunkSize() which is appropriate for the initial chunk download, but inappropriate for the subsequent larger chunks. This particular memory leak would not affect files or file chunks that are smaller than the m.minChunkSize() value. Closes: #150
philandstuff
approved these changes
Feb 7, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Based upon how bytes.Buffer works, it will continue to grow the underlying slice while reading data and add additional headroom to ensure that there is enough space, this means that when we allocate the bytes.Buffer to a small amount (minChunkSize()) the growth occurs regularly on any reads that push beyond the initial slice. Even at a minChunkSize of ~150M, when downloading SDXL we end up with a balloon of an additional ~10GiB allocated.
This has resulted in SigKill (OOMKILL) of pget in lower-ram systems. This does not address the overall memory allocation requirements of pget, which still generally encompasses the entire file size. An additional pass will need to be done to ensure the consumer can read and the buffer can be GC'd to minimize overall memory allocation.
This only surfaced in consistent-hash mode as buffer mode was properly setting the buffered reader to the chunkSize by calculating end - start +1. Consistent Hash only used m.minChunkSize() which is appropriate for the initial chunk download, but inappropriate for the subsequent larger chunks.
This particular memory leak would not affect files or file chunks that are smaller than the m.minChunkSize() value.
Closes: #150