carstenbauer / ThreadPinning.jl

Readily pin Julia threads to CPU-threads
https://carstenbauer.github.io/ThreadPinning.jl/
MIT License
106 stars 7 forks source link

Fix indexing bug with pinthreads() #17

Closed JamesWrigley closed 1 year ago

JamesWrigley commented 2 years ago

When the :random strategy is selected pinthreads() will default to ignoring hyperthreads, but in this case the number of CPU IDs returned will be less than the total number of threads available, and taking a view of the returned CPU IDs will fail since the end index is out of bounds. There are probably other situations that would trigger this.

carstenbauer commented 2 years ago

Thanks for the PR! However, I don't think we should just use min(length(cpuids), nthreads) here because this will subtly only pin a subset of the Julia threads. I think for the case length(cpuids) < nthreads we should throw an explicit error indicating that there aren't enough cores for the number of Julia threads (and perhaps also suggest that, if HT is available, hyperthreads=true could be a solution). This way, the user can be certain that all threads are properly pinned if the function doesn't error. What do you think?

JamesWrigley commented 2 years ago

Thanks for the review :) I promise I haven't forgotten about this, just really busy ATM so I'll probably get back to it in a week or so.

carstenbauer commented 1 year ago

Hey @JamesWrigley, any time to finish this? :)

JamesWrigley commented 1 year ago

cough sorry :flushed: I see that you fixed the original issue in cc75d18.

Now there's only two changes:

Also noticed that openblas_pinthreads() still relies on the old _strategy_* methods, but I see that's fixed in #28.

JamesWrigley commented 1 year ago

(I'll rebase when it's ready to merge)