Reusable buffers #174
Closed
Reusable buffers #174
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
tempusfrangit
commented
Mar 5, 2024
tempusfrangit
commented
Mar 5, 2024
tempusfrangit
force-pushed
the
reusable-buffers
branch
2 times, most recently
from
730e35b
to
2677b32
Compare
To cleanup the use of the signal channel and enable easy re-use of the BufferedReader this commit converts the channel to a sync.Cond. This allows a reset to simply call: b.ready = false b.buf.Reset() It also eliminates the need to re-create the channel on a reset case.
Supporting reusable readers will help us reduce memory needs of pget. This commit implements the buffered reader pool and wires up CH and buffer mode to use it to acquire the buffered readers. This lightens the logic even further guaranteeing that we always use the same capacity for all readers as it's defined in the `New` function used by the sync.Pool This commit does not return buffers to the pool.
When we're done with a bvuffered reader, call `.Close()` which will do the reset and re-add to the pool. This change also pivots the BufferedReader to a ReadCloser type interface. This change follows through on the promise of allowing reuse of buffers, minimizing allocations, and reducing overall memory requirements of pget. This will help further limit OOM situations and lead towards a future of also limiting maximum memory utilization of pget.
Move the reset logic to .Close as that makes the bufferedReader responsible for managing it's state not the pool. The pool is just a broker to hand out readers.
philandstuff
force-pushed
the
reusable-buffers
branch
from
2677b32
to
0485e1c
Compare
philandstuff
added a commit
that referenced
this pull request
Mar 6, 2024
This is an alternative to #174. The key differences are: - we use sync.Pool to store *bytes.Buffer instances rather than bufferedReader instances - we keep the ready channel instead of using sync.Cond - we return the buffer to the pool in Read() rather than requiring an explicit Close() Overall this changes fewer parts of the codebase and is (IMHO) simpler and easier to understand.
|
I've looked at this and implemented an alternative in #177. |
philandstuff
added a commit
that referenced
this pull request
Mar 6, 2024
* Reuse buffers used by bufferedReader This is an alternative to #174. The key differences are: - we use sync.Pool to store *bytes.Buffer instances rather than bufferedReader instances - we keep the ready channel instead of using sync.Cond - we return the buffer to the pool in Read() rather than requiring an explicit Close() Overall this changes fewer parts of the codebase and is (IMHO) simpler and easier to understand.
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.
In an effort to limit the OOM scenarios within PGET this PR implements buffers that are fully reusable. This is done with a sync.Pool and builds upon the change to use fixed buffer sizes for any given invocation of PGET.
This PR does not limit overall memory use of PGET.
Partial: #2