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

stream: iter() should yield every so often. #2343

Merged
merged 2 commits into from Mar 25, 2020
Merged

Conversation

carllerche
Copy link
Member

stream::iter() should yield every so often and participate in the coop functionality. However, I'm not sure if it should consume budget on every iteration.

Another option would be to consume budget every ~25 steps... but that would require adding state to the stream. Yet another strategy would be to make consuming budget be weighted. So stream::iter would consume 1 unit every iteration but reading data would consume much more (1 unit per n bytes? not sure)

@carllerche carllerche requested a review from jonhoo March 25, 2020 20:33
@jonhoo
Copy link
Sponsor Contributor

jonhoo commented Mar 25, 2020

I actually don't think iter() should participate in coop budgeting, at least not in that way. It should be left to the underlying resources that feed the underlying streams. What you may want to do though is make it so that iter() detects if a significant number of the underlying streams have returned Pending in a single iteration, it'll check if the budget has been exhausted, and if so return Pending itself rather than poll all the remaining streams (since they will likely also return Pending).

@carllerche
Copy link
Member Author

iter() is an underlying resource IMO. An "underlying resource" is the leaf async fn, which iter is.

I also have seen people hit this specific problem, i.e. use an unbounded (or very large) iter, map it to a stream, and it hangs the executor. Given this, it seems good to fix.

@jonhoo
Copy link
Sponsor Contributor

jonhoo commented Mar 25, 2020

Ohhhh, sorry, I read this as the async stream iter thing that came up a while back (the impl Stream over a collection of impl Stream). This is a Stream over a regular Iterator. Yes, then I agree that it is an underlying resource and should participate in budgeting. I actually also think it's fine to consume one "unit", at least for the time being. I don't think we have a good enough grasp on what budgeting units should be at the moment, so "one" is probably okay. Maybe there's an argument for having a "coop::squander" which temporarily removes the budgeting for a particular scope?

@carllerche
Copy link
Member Author

I'm fine starting w/ this if you are.

@carllerche carllerche merged commit 186196b into master Mar 25, 2020
@carllerche carllerche deleted the stream-iter-coop branch April 15, 2020 20:38
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