Fix memory allocation in countingReader #38
Conversation
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
tyaps
force-pushed
the
fix-memory-allocation
branch
3 times, most recently
from
3749ace to
e197fa8
Compare
Currently, countingReader.Seek() uses io.ReadFull() to read data, so memory is allocated for all content of rpm file (and content has no meaning here because only offset of reader must be changed). So while parsing header of big rpm file, a lot of memory allocates. Use reading by small chunks. Signed-off-by: Sergey Tyapkin <[email protected]>
tyaps
force-pushed
the
fix-memory-allocation
branch
from
e197fa8 to
3e4227d
Compare
|
Hello. Is there a chance for this PR to be merged?)) |
mtharp
reviewed
Mar 18, 2025
Comment on lines
+149
to
+174
| totalRead := int64(0) | ||
| const chunkSize = int64(1024 * 1024) | ||
| buf := make([]byte, chunkSize) | ||
|
|
||
| remaining := offset | ||
| for remaining > 0 { | ||
| // Define chunk size to read | ||
| toRead := chunkSize | ||
| if remaining < chunkSize { | ||
| toRead = remaining | ||
| } | ||
|
|
||
| n, err := cr.Read(buf[:toRead]) | ||
| totalRead += int64(n) | ||
| remaining -= int64(n) | ||
| if err != nil && err != io.EOF { | ||
| // If all was read, skip error | ||
| if totalRead >= offset { | ||
| err = nil | ||
| } | ||
| return 0, err | ||
| } | ||
|
|
||
| if err == io.EOF { | ||
| break | ||
| } |
There was a problem hiding this comment.
Hi, thanks for your contribution! This is definitely worth fixing.
I believe this can be addressed much more simply by using io.Discard:
Suggested change
| totalRead := int64(0) | |
| const chunkSize = int64(1024 * 1024) | |
| buf := make([]byte, chunkSize) | |
| remaining := offset | |
| for remaining > 0 { | |
| // Define chunk size to read | |
| toRead := chunkSize | |
| if remaining < chunkSize { | |
| toRead = remaining | |
| } | |
| n, err := cr.Read(buf[:toRead]) | |
| totalRead += int64(n) | |
| remaining -= int64(n) | |
| if err != nil && err != io.EOF { | |
| // If all was read, skip error | |
| if totalRead >= offset { | |
| err = nil | |
| } | |
| return 0, err | |
| } | |
| if err == io.EOF { | |
| break | |
| } | |
| return io.CopyN(io.Discard, cr, offset) |
It may be a tiny bit less efficient but the simplicity is worth it in my opinion.
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.
Currently, countingReader.Seek() uses io.ReadFull() to read data, so memory is allocated for all content of rpm file (and content has no meaning here because only offset of reader must be changed).
So while parsing header of big rpm file, a lot of
memory allocates.
Use reading by small chunks.