ompi: Avoid unnecessary PMIx lookups when adding procs (step 2). #3181
Conversation
|
As it was suggested in #3011 this PR removes some unnecessary overhead during ompi add proc. I consider this as a perf bug and we would like to have it in v2.1.1. |
ompi/proc/proc.c
Outdated
| } | ||
| } | ||
|
|
||
| /* TODO: do we need to lock here ? */ | ||
| opal_mutex_lock (&ompi_proc_lock); |
There was a problem hiding this comment.
I think you do need the lock in order to sort the proc list. However, I would suggest removing the unlock just above this loop and remaining locked until the end. The cost of unlock/relock likely outweighs any benefit as we only execute that loop if we are below the cutoff, and the cutoff defaults to zero (if that isn't true yet, then please ask that we put it on the telecon agenda).
There was a problem hiding this comment.
I believe that here the cost of the lock/unlock is negligible - no contention.
There was a problem hiding this comment.
I agree that the probability of contention is low, and shouldn't exist during initial launch (might occur later during a connect/accept operation), so I think the issue is minor. I was only pointing it out as something minor to consider. One thing you could do, though, is move the unlock to right after the if statement, and then relock before leaving that clause. This way, you'd only unlock/relock if you actually need to do so.
Again, minor point, so I'm okay if you'd rather not do it.
There was a problem hiding this comment.
Good point!
will update now.
| opal_process_name_t proc_name; | ||
| proc_name.jobid = OMPI_PROC_MY_NAME->jobid; | ||
| proc_name.vpid = i; | ||
| (void) ompi_proc_for_name (proc_name); |
There was a problem hiding this comment.
@rhc54 we need to unlock because ompi_proc_for_name is locking internally.
artpol84
force-pushed
the
add_proc_fix_2/master
branch
from
5a143ef to
9e1c0d8
Compare
Follow-up for 717f3fe. Signed-off-by: Artem Polyakov <[email protected]>
artpol84
force-pushed
the
add_proc_fix_2/master
branch
from
9e1c0d8 to
1f7a3a2
Compare
Follow-up for 717f3fe.
Signed-off-by: Artem Polyakov [email protected]