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

Use iteration instead of indexing in Threads.@threads #40788

Closed
wants to merge 2 commits into from

Conversation

sostock
Copy link
Contributor

@sostock sostock commented May 11, 2021

The code created by Threads.@threads for x = y; ...; end currently indexes into y. Therefore, it does not work for iterators which do not support indexing. This PR changes that by using Iterators.drop and Iterators.take to pick the correct chunk of y for each thread.

Fixes #40704.

@simeonschaub
Copy link
Member

This will iterate through the entire vector O(nthreads) times, which will have a large penalty. If we wanted to support Threads.@threads for more iterators, we would want some kind of generic API for splitting collections. There is SplittablesBase which has something like this. Perhaps something like it could be moved into Base, but that would have to be well thought through.
It doesn't make much sense in my mind to try to support any kind of iterator, since this will inherently never work for stateful iterators and be very slow for Iterators where iterate is expensive.

@sostock
Copy link
Contributor Author

sostock commented May 12, 2021

We could use indexing for AbstractArrays to get the old performance back and only iterate over the collection as a fallback for non-AbstractArray iterators. Then it will at least work for generic non-stateful iterators, despite being slower in those cases.

I think, in typical applications the actual computation (i.e., the loop body) will be more expensive than the iteration itself, so iterating over the collection nthreads times does not seem like a bad tradeoff to support arbitrary (non-stateful) iterators.

@tkf
Copy link
Member

tkf commented May 12, 2021

While I understand the desire to make @threads more usable, I strongly suggest avoiding this approach. This Iterators.drop-based approach incurs O(length(input) * nthreads()) overhead which is not negligible for a large class of collections including Iterators.map, Iterators.filter, strings, hash tables, trees, lists, and so on.

SplittingBase takes a simple approach but it turned out to be very effective. Although I still want to explore better APIs for handling important non-standard cases, I believe something like this combined with loop composition based on higher-order function is a much better approach for providing extensible parallel loop infrastructure.

@Seelengrab
Copy link
Contributor

Seelengrab commented May 13, 2021

I think a good solution would be to have the @threads macro support work stealing or lazy iteration for non-indexables. It does support switching scheduling strategy since 1.5, but only :static scheduling is available right now. That would also prevent redundant consumption of the given iterable (which may have side effects anyway). This sadly would make it much more complicated :/

@tkf
Copy link
Member

tkf commented May 13, 2021

For the class of parallel code that include for loops, it's already possible to implement even continuation stealing (the "better" strategy of work-stealing) in Julia. It also already supports generic collections that implement SplittablesBase interface.

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.

firstindex(enumerate(...)) not defined
5 participants