-
Notifications
You must be signed in to change notification settings - Fork 14
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
Unclear/misleading README.md #124
Comments
It might be that they haven't been re-run since before the
Certainly! "3." is also definitely a major pain point.
It does not. Static scheduling is much easier to implement. ;) |
I know, but it that's not clear from the README :)
Makes sense, I will rerun the benchmarks when I update the README.
Yes, a work stealing scheduler would be nice. But I agree, it would be nice if we could just have one smart scheduler instead of multiple (FWIW, Dagger.jl also implements a scheduler etc.) |
I'm rewriting the README.md and have a question about the following example julia> let
@batch threadlocal=rand(10:99) for i in 0:9
println("index $i, thread $(Threads.threadid()), local storage $threadlocal")
threadlocal += 1
end
println(threadlocal)
end
index 8, thread 1, local storage 30
index 3, thread 3, local storage 49
index 9, thread 1, local storage 31
index 6, thread 4, local storage 33
index 0, thread 2, local storage 14
index 4, thread 3, local storage 50
index 7, thread 4, local storage 34
index 1, thread 2, local storage 15
index 5, thread 3, local storage 51
index 2, thread 2, local storage 16
Any[32, 17, 52, 35] The storage doesn't seem to be thread-local: Julia thread 1 appears twice, once with local storage 30, another time with local storage 31. It also makes sense to me that it isn't thread-local. I would expect it to be "Polyester task"-local. Is that's what's happening here? If that's the case, I think BTW, is there a function like |
Eh, I suspect it's a bit broken. Would be great if I (or someone else) can get around to reimplementing.
I'm not sure what the best API to actually expose to users is, either, e.g., we could go with something like letting users provide a function |
@carstenbauer, I was just trying to make sense of that |
Hi @chriselrod,
I think that the current README.md doesn't do a particularly good job at explaining how Polyester.jl works with respect to three (crucial) points:
I think that sentences like the following are insufficient and potentially misleading:
It suggests that Polyester potentially changes the number of threads (1) and also pins them to CPU-cores/threads (2). Of course, neither is the case. To be fair, that Polyester doesn't do (2) is mentioned in the README.md but only in a comment in a code example, which can easily be overlooked. The fact that Polyester just uses the already existing Julia threads (1) is also mentioned in the README (at a more prominent place) but is potentially misleading in the context of everything else that follows.
As for point (3) above, the README only compares to
@threads
which (as of now) defaults to:dynamic
scheduling. This seems somewhat strange/misleading because (IIUC)@batch
creates sticky tasks and and schedules the workload statically. Hence, it should really be compared to@threads :static
. Of course, one can also compare to@threads :dynamic
but the conceptual differences should be highlighted then IMHO.Overall, IIUC, Polyester really just creates, manages, and re-uses a bunch of sticky tasks and just runs them on existing Julia threads (similar to
@threads :static
). However, IMHO, this isn't clear from the README.md which doesn't contain the words "sticky", "task(s)" or, most importantly, "static" at all.If you're open to it, I might make a suggestion for an improved README.md in a PR later this week. Let me know what you think!
(BTW, apart from the
stride
option, Polyester.jl doesn't implement any form of load-balancing, right?)The text was updated successfully, but these errors were encountered: