Parallelism approach seems inconsistent
dievsky opened this issue · 7 comments
Some parts of bioinf-commons use the common ForkJoinPool, while other parts create a newFixedThreadPool.
For example, MoreStreams.kt uses ForkJoinPool in its chunked and forking extension methods, but ExecutorExtensions.kt methods (like await) prefer a separate pool. Span uses both approaches: FJP is used for the HMM EM expect and refill steps, and GenomeMap parallel operations (e.g. read coverage or compute peaks) work through await and use a special thread pool.
It might be better to use just one approach (common FJP, most likely). This will at least prevent an inadvertent creating of several thread pools in parallel. (And while we're at it, we might want to switch to coroutines.)
Discussion is most welcome.
Did you get any problems with this in real use cases? FJP is know to perform well when single operation is 100-150ms long.
Currently, the FJP code and the newFixedThreadPool code are separate and don't call each other. This might change in the future, though.
Are you saying that there's a reason why we use the two approaches here? One is better for the fast operations, and the other is better for the slow ones?
Yes, that's one of the reasons.
I've read some opinions on this issue, and here's what can pass for the general consensus:
"One last thing to remember is that the choosing a ForkJoinPool is only useful if the tasks create subtasks. Otherwise, it will function the same as a ThreadPoolExecutor, but with extra overhead." (https://stackify.com/java-thread-pools/)
However, expect and refill methods (which use FJP) don't seem to create any subtasks.
Update: my bad. Actually, IntRange.chunked(used in HMMIterationContext#expect and several other places) involves a ChunkSpliterator, and this approach does occasionally fork subtasks. However, this seems like a technical decision, since cutting the range into small chunks and feeding them to a fixed-thread executor all at once would likely work as efficiently.
I don't see any changes, to be honest, but we can certainly close the issue by ignoring it ("don't fix what ain't broken" and all that).
Well, the one change that I see is that we no longer use newFixedThreadPool (just one usage); instead, newWorkStealingPool is used, so all our parallelism is now work-stealing (both the common FJP and the hand-made newWorkStealingPool).
Closing as obsolete. Lately most of parallel code shares executor in ExecutorExtensions.kt