dash icon indicating copy to clipboard operation
dash copied to clipboard

Issues with Thread-Safety in DASH

Open rkowalewski opened this issue 6 years ago • 1 comments

The implementation of dash::sort now heavily relies on threading and asynchronous tasks, especially in the end while we merge all our sorted sequences. In order to test that we had to enable thread support on our CI. Some tests in the ThreadsafeTest.* suite fail now which absolutely makes sense.

We have to push concurrency in DASH to the next level and make threading a first-class citizen in our algorithms. In dash::sort we try to evaluate some ideas how to that and eventually we will come with a better abstraction, we'll see. However, this does not imply that we have to make our containers thread-safe which is costly and also not easy to implement. There are libraries which do very good job there (e.g. Intel TBB) and transferring these concepts to DASH is not easy, especially with our low man power.

To simplify future development we have to agree on what is thread-safe and what not. DART is better with respect to documentation, etc. and most of it is thread-safe which is good and we appreciate that in our sorting algortihm. In DASH, threading is still a pain somehow because we have these #ifdefs and some OpenMP pragmas. That is not a solution especially if we support TBB (through Intel's ParallelStl) as well.

Coming back to our ThreadsafetyTest.*. At least two tests allocate a dash::Array concurrently in a parallel OpenMP region. This is something which I do not really want to support because it requires to rewrite many thing in DASH which makes containers and especially pattern stuff more complex and it will certainly degrade performance. And actually I do not see any advantage in that. We should focus on NUMA effects (see #615) and parallelism is relevant in algorithms. If one needs thread-safe containers then the user should rely on a different library (e.g. Intel TBB) for now.

What's your opinion?

rkowalewski avatar Dec 16 '18 12:12 rkowalewski

Some tests in the ThreadsafeTest.* suite fail now which absolutely makes sense.

Why does it make sense? It used to work in the past...

However, this does not imply that we have to make our containers thread-safe which is costly and also not easy to implement. There are libraries which do very good job there (e.g. Intel TBB) and transferring these concepts to DASH is not easy, especially with our low man power.

Do you mean thread-safety in the sense of thread-safe conflicting accesses (e.g., lock-free data structures) or thread-safe non-conflicting accesses? The latter won't need much work imo and should be mostly supported (I use multi-threaded access on containers in my work).

In DASH, threading is still a pain somehow because we have these #ifdefs and some OpenMP pragmas. That is not a solution especially if we support TBB (through Intel's ParallelStl) as well.

Using OpenMP in DASH has bitten in the past as well, yes. It should only be enabled if the user explicitly requests it, i.e., through an execution policy. The should be required to enable the use of TBB and later on DART tasks.

Coming back to our ThreadsafetyTest.*. At least two tests allocate a dash::Array concurrently in a parallel OpenMP region. This is something which I do not really want to support because it requires to rewrite many thing in DASH which makes containers and especially pattern stuff more complex and it will certainly degrade performance.

Why is that a problem? As long as the containers are allocated on distinct teams it should fine. One use-case used to be the DASH algorithms (some of which used to allocate temporary global memory). I don't see why we should not support conflict-free thread-parallel operations in DASH (conflict-free being the absence of conflicts from the user's point of view, e.g., collective operations on the same team or writing to the same memory locations).

The thread-safety tests succeeded on my machine the last time I ran them. I will look into that sometime next week (sorry, swamped with other things atm).

devreal avatar Dec 16 '18 18:12 devreal