Improve request/progress performance #1810
Conversation
The OPAL_ENABLE_MULTI_THREADS macro is always defined as 1. This was causing us to always use the multi-thread path for synchronization objects. The code has been updated to use the opal_using_threads() function. When MPI_THREAD_MULTIPLE support is disabled at build time (2.x only) this function is a macro evaluating to false so the compiler will optimize out the MT-path in this case. The OPAL_ATOMIC_ADD_32 macro has been removed and replaced by the existing OPAL_THREAD_ADD32 macro. Signed-off-by: Nathan Hjelm <hjelmn@lanl.gov>
This commit adds another check to the low-priority callback conditional that short-circuits the atomic-add if there are no low-priority callbacks. This should improve performance in the common case. Signed-off-by: Nathan Hjelm <hjelmn@lanl.gov>
| } else { | ||
| OPAL_ATOMIC_CMPSET_32(&(sync->count), 0, 0); | ||
| sync->status = -1; | ||
| /* this is an error path so just use the atomic */ |
There was a problem hiding this comment.
Want to point out this error path was wrong. OPAL_ATOMIC_CMPSET_32(&(sync->count), 0, 0); does nothing.
|
👍 |
| (sync)->next = NULL; \ | ||
| (sync)->prev = NULL; \ | ||
| (sync)->status = 0; \ | ||
| if (opal_using_threads()) { \ |
There was a problem hiding this comment.
Since PTHREAD_COND_INIT and PTHREAD_MUTEX_INIT already check for opal_using_threads(), this branch might be redundant ?
There was a problem hiding this comment.
Indeed. I didn't think to check what those macro did. Will clean it up in a bit.
There was a problem hiding this comment.
Unless you want to do it. I can add your commit to the 2.x PR.
There was a problem hiding this comment.
Didn't see your comment until later, timezone issues. I see that you already have a PR to fix it - #1811
Thanks 👍
|
@hjelmn Apart from the previous comment it looks good 👍 |
3 participants
This fixes a couple of performance regressions and is targeted to 2.0.0.
@nysal, @thananon Let me know if there is anything else that should be fixed. The RC needs to go out ASAP.