Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: added limitedByteReader to avoid breader #56

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

orisano
Copy link

@orisano orisano commented Jun 17, 2023

The ByteReader function in some cases creates a breader that reads one byte at a time, resulting in inefficient performance. However, if the passed Reader implements io.ByteReader, it is more efficient to use it instead.

In the case of LZMA2, it is wrapped with io.LimitReader, which inadvertently triggers the usage of breader.
To avoid this, I have added an io.ByteReader with the functionality of io.LimitedReader. This change has shown approximately a 10% performance improvement.

I kindly request your review and feedback on this pull request. Thank you.

@ulikunitz
Copy link
Owner

Many thanks for your PR. I have to review it in detail and will do it this week.

@orisano
Copy link
Author

orisano commented Jun 20, 2023

benchstat:

goos: linux
goarch: amd64
pkg: github.com/bodgit/sevenzip
cpu: Intel(R) Core(TM) i7-8650U CPU @ 1.90GHz
             │  base.txt   │             xz#55.txt              │    xz#55_xz#56_sevenzip#95.txt     │
             │   sec/op    │   sec/op    vs base                │   sec/op    vs base                │
LargeLZMA2-8   10.775 ± 0%   8.704 ± 0%  -19.22% (p=0.000 n=10)   8.384 ± 0%  -22.19% (p=0.000 n=10)

@ulikunitz
Copy link
Owner

Hi, would it be possible for you to adapt the change for the rewrite branch? The new branch supports multithreading and has a number of other performance improvements.

@orisano orisano changed the base branch from master to rewrite October 15, 2023 20:55
@orisano orisano changed the base branch from rewrite to master October 15, 2023 21:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants