-
Notifications
You must be signed in to change notification settings - Fork 29
feat: allow specifying stream buffer size #74
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: allow specifying stream buffer size #74
Conversation
|
@mdsteele I'm not so sure about the proliferation of the functions for creating streams but did not want to break backwards compatibility. What are your thoughts? I like to keep the buffer size per stream. Clients can then choose to only set it for big streams where it matters. I have some further more involving refactors that I will create a PR for once this is merged. |
|
Interesting, I'm surprised that a buffer larger than 8192 bytes makes such a large difference. Maybe there is just too much overhead right now with refilling the buffer? Regardless, I agree it makes sense to give the caller control over the buffer size when needed; I do share your concern over the proliferation of stream-creation methods, especially if we ever need to add another parameter in the future. One possibility to consider would be to create a new type akin to |
|
(To clarify, BTW, I'm open to being convinced either way on the API question; just raising another possible option.) |
bcd2a60 to
c0a39dc
Compare
|
@mdsteele switched to options |
|
Thanks! |
|
@mdsteele could we get a release containing these last two changes? |
|
Published as v0.13.0. |
With some more benchmarks I found that the buffer size is a serious bottleneck for larger streams >= 1MB