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

Implement Stream::size_hint #128

Closed
yoshuawuyts opened this issue Aug 29, 2019 · 8 comments · Fixed by #287
Closed

Implement Stream::size_hint #128

yoshuawuyts opened this issue Aug 29, 2019 · 8 comments · Fixed by #287
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@yoshuawuyts
Copy link
Contributor

Equal to https://doc.rust-lang.org/std/iter/trait.Iterator.html#method.size_hint. Don't think this needs to be marked async. This would've been useful to calculate the right pre-alloc size for calling collect into a vec in #125. Thanks!

@yoshuawuyts yoshuawuyts added enhancement New feature or request good first issue Good for newcomers labels Aug 29, 2019
@taiki-e
Copy link
Contributor

taiki-e commented Aug 31, 2019

Could you explain the value of size_hint that cannot be implemented manually?

@yoshuawuyts
Copy link
Contributor Author

yoshuawuyts commented Aug 31, 2019

@taiki-e I feel its mostly API consistency?

Though with iter size hint it's useful when pre-allocating structures. For example when using collect for vec it would've been useful to get an estimate of how much to allocate. Right now we start with an empty vector and allocate as we go, which is not nearly as efficient as it could be.

@taiki-e
Copy link
Contributor

taiki-e commented Aug 31, 2019

The problem is that size_hint always returns the default value because async_std::stream::Stream cannot be implemented manually. So, currently, calling size_hint for optimization is a needless operation.

@taiki-e
Copy link
Contributor

taiki-e commented Aug 31, 2019

IMO, we should wait for the upstream to add size_hint, instead of adding such a “useless API”.

@yoshuawuyts
Copy link
Contributor Author

@taiki-e ah yeah, fair point.

Though I don't disagree, I do see value in implementing all APIs sooner rather than later though. This way we can have the external API that folks can call, which can be incorporated in app logic — and later when we fix the impl nobody needs to rewrite their code.

Or well, that's the theory.

@yoshuawuyts
Copy link
Contributor Author

Yay, rust-lang/futures-rs#1853 landed!

@yoshuawuyts
Copy link
Contributor Author

We're now blocked on futures-preview@0.3.0-alpha.19 landing to implement this.

@yoshuawuyts
Copy link
Contributor Author

Sweet, this has landed! This means we can now rely on size_hint for optimizing allocations in FromStream impls!

cc/ @sunjay

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants