Skip to content

Conversation

@hjelmn
Copy link
Member

@hjelmn hjelmn commented Jun 21, 2016

This commit fixes a performance regression introduced by the request
rework. We were always using the multi-thread path because
OPAL_ENABLE_MULTI_THREADS is either not defined or always defined to 1
depending on the Open MPI version. To fix this I removed the
conditional and added a conditional on opal_using_threads(). This path
will be optimized out in 2.0.0 in a non-thread-multiple build as
opal_using_threads is #defined to false in that case.

Fixes #1806

Signed-off-by: Nathan Hjelm hjelmn@lanl.gov

This commit fixes a performance regression introduced by the request
rework. We were always using the multi-thread path because
OPAL_ENABLE_MULTI_THREADS is either not defined or always defined to 1
depending on the Open MPI version. To fix this I removed the
conditional and added a conditional on opal_using_threads(). This path
will be optimized out in 2.0.0 in a non-thread-multiple build as
opal_using_threads is #defined to false in that case.

Fixes open-mpi#1806

Signed-off-by: Nathan Hjelm <hjelmn@lanl.gov>
@hjelmn
Copy link
Member Author

hjelmn commented Jun 21, 2016

@nysal Confirmed the latency is back down to what is expected with this change.

@hjelmn hjelmn merged commit 7bd7c05 into open-mpi:master Jun 21, 2016
@thananon
Copy link
Member

thananon commented Jun 22, 2016

There's still macro left in opal/thread/wait_sync.h that could affect the performance. I can fix this.

#if OPAL_ENABLE_MULTI_THREADS

#define OPAL_ATOMIC_ADD_32(a,b)         opal_atomic_add_32(a,b)
#define OPAL_ATOMIC_SWP_PTR(a,b)        opal_atomic_swap_ptr(a,b)
#define SYNC_WAIT(sync)                 sync_wait_mt(sync)
#define PTHREAD_COND_INIT(a,b)          pthread_cond_init(a,b)
#define PTHREAD_MUTEX_INIT(a,b)         pthread_mutex_init(a,b)

#define WAIT_SYNC_RELEASE(sync)                       \
    do {                                              \
       pthread_cond_destroy(&(sync)->condition);      \
       pthread_mutex_destroy(&(sync)->lock);          \
    } while(0)

#define WAIT_SYNC_SIGNAL(sync)                        \
    do {                                              \
        pthread_mutex_lock(&(sync->lock));            \
        pthread_cond_signal(&sync->condition);        \
        pthread_mutex_unlock(&(sync->lock));          \
    } while(0)

#else

#define OPAL_ATOMIC_ADD_32(a,b)         (*(a) += (b))
#define OPAL_ATOMIC_SWP_PTR(a,b)        *(a) = (b)
#define PTHREAD_COND_INIT(a,b)
#define PTHREAD_MUTEX_INIT(a,b)
#define SYNC_WAIT(sync)                 sync_wait_st(sync)
#define WAIT_SYNC_RELEASE(sync)
#define WAIT_SYNC_SIGNAL(sync)

#endif /* OPAL_ENABLE_MULTI_THREADS */

@hjelmn
Copy link
Member Author

hjelmn commented Jun 22, 2016

Yup. Have that fixed in my local branch. Plan to push later today after testing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Single threaded latency with latest request changes

2 participants