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

Task.select is no longer used and can potentially impact performance negatively #217

Merged
merged 1 commit into from
Oct 20, 2022

Conversation

phausler
Copy link
Member

After iteration with folks on a number of the algorithms we no longer need Task.select. This particular item has some distinct performance impacts that are perhaps not ideal for general use and ends up pushing (in the realms of AsyncSequence iteration) perhaps dubious value.

Since we are at a pre-1.0 it would be better to remove this now and re-evaluate its need later (if at all).

@stephencelis
Copy link

Is there any public discussion on this? Curious what alternatives folks should be using to "race" tasks, and what performance pitfalls should be avoided.

@phausler
Copy link
Member Author

So the discussion has been centered around the Sendability of the AsyncIterator's of the different algorithms. Basically the issue becomes that spawning a full-on Task (not a child task) per iteration is not only incorrect per the requirement of Sendable for said iterators but also has a grave impact for performance. We have seen full on orders of magnitude performance improvement by going the route of having 1 singular task handling the state machine transitions rather than taking the "simpler" route of spawning a task to race two iterators' progression.

The tl;dr is that you should avoid creating a Task per each call to next().

@phausler
Copy link
Member Author

Since this is a deletion without replacement I do want to have discussion on what use cases we still need to address in a more general sense.

Copy link
Member

@FranzBusch FranzBusch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that we should remove this at least for the initial release since it encourages to use unstructured Concurrency. As we have learned ourselves here, the issues are solvable with the usage of some structured concurrency. While we still need to spawn Tasks for the consumption of upstream sequences we never need to race them.

@stephencelis
Copy link

Racing things is quite useful, though. We do so in an ad hoc way with task groups in some of our projects that I think could be nicer. If not with Task.select maybe with some other API, like TaskGroup.race?

@phausler
Copy link
Member Author

I agree on the general utility; however I really think that it needs to occur at a much lower level than the current interfaces available. My thought is to collect information on it and the use cases to provide solid ground for someone to take up the mantle as a pitch/proposal for the concurrency library.

@ktoso
Copy link
Member

ktoso commented Oct 19, 2022

This sounds good to me, let's remove and re-add when we gain more experience and use-cases, hopefully a more efficient take on it then.

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

4 participants