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

DecoderReader probably should accept BufRead instead of Read #230

Open
Kixunil opened this issue Mar 31, 2023 · 1 comment · May be fixed by #231
Open

DecoderReader probably should accept BufRead instead of Read #230

Kixunil opened this issue Mar 31, 2023 · 1 comment · May be fixed by #231

Comments

@Kixunil
Copy link

Kixunil commented Mar 31, 2023

I noticed DecoderReader holds its internal buffer of bytes. This has a number of disadvantages:

  • Needless copying from in-memory readers (slices)
  • Double-buffering already-buffered readers
  • The bytes are lost when dropping or calling into_inner
  • BUF_SIZE is not configurable
  • std::io::BufReader has access to some unstable optimizations which this crate cannot use
  • Reinvents the wheel; there already is std::io::BufReader

Unless I'm missing something this can be easily fixed just by requiring BufRead instead of Read and using it. It's a breaking change so it's better to do it before 1.0.

Related: maybe it itself could provide BufRead but I'm not sure if 3-byte buffer is fine or not.

@Kixunil Kixunil linked a pull request Mar 31, 2023 that will close this issue
@Kixunil
Copy link
Author

Kixunil commented Mar 31, 2023

I was curious enough to give it a shot and indeed, this improves performance. See the PR.

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 a pull request may close this issue.

1 participant