Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

v2.x: fix MPI_Alltoallw() with zero size messages #3487

Merged
merged 5 commits into from
Aug 9, 2017
Merged

v2.x: fix MPI_Alltoallw() with zero size messages #3487

merged 5 commits into from
Aug 9, 2017

Conversation

ggouaillardet
Copy link
Contributor

@ggouaillardet ggouaillardet commented May 9, 2017 •
edited
Loading

Refs. #3480

@ggouaillardet ggouaillardet added this to the v2.1.1 milestone May 9, 2017
@ggouaillardet ggouaillardet requested a review from bosilca May 9, 2017 01:44
@ggouaillardet ggouaillardet changed the title v2.x: fixes MPI_Alltoallw() with zero size messages v2.x: fix MPI_Alltoallw() with zero size messages May 9, 2017
@hppritcha hppritcha modified the milestones: v2.1.2, v2.1.1 May 9, 2017
@jsquyres
Copy link
Member

jsquyres commented May 9, 2017

Per 2017-05-09 webex, move this to v2.1.2.

@hppritcha
Copy link
Member

@bosilca could you look at this? thanks.

@bosilca
Copy link
Member

bosilca commented Jun 19, 2017

@hppritcha I did a while back. I left a comment that was never addressed.

@jsquyres
Copy link
Member

@bosilca Do you know where that comment is? I notice that the v2.0.x version of this PR (#3486) was already merged ☹️

@bosilca
Copy link
Member

bosilca commented Jun 19, 2017

I can see it right here in the "Files changed" tab. It states "Why is this test not done before allocating the NBC handle ?"

@jsquyres
Copy link
Member

Er... that's weird. I don't see your comment at all! 😲

bosilca
bosilca reviewed Jun 19, 2017
View reviewed changes
ompi/mca/coll/libnbc/nbc_ialltoallv.c Outdated
@@ -74,6 +74,11 @@ int ompi_coll_libnbc_ialltoallv(const void* sendbuf, const int *sendcounts, cons
}
}
span = opal_datatype_span(&recvtype->super, count, &gap);
if (OPAL_UNLIKELY(0 == span)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this test not done before allocating the NBC handle ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i did it (among some othe revamp) in #3719

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, but #3719 is for master, and I am not sure it will be back-ported to the 2.x. My comment doesn't point to a real issue, mostly to a missed optimization opportunity. Once you add 68ac950 I will review it again.

@bosilca
Copy link
Member

bosilca commented Jun 19, 2017

Ha. If you don't submit the review the comments inside are not visible.

ggouaillardet added a commit to ggouaillardet/ompi that referenced this pull request Jun 20, 2017
@ggouaillardet
coll/libnbc: revisit NBC_Handle usage
9ba85b8 
make NBC_Handle (almost) an internal structure created
by NBC_Schedule_request()
use a local variable instead of what was previously handle->tmpbuf

Refs open-mpi#3487

Signed-off-by: Gilles Gouaillardet <[email protected]>
@ggouaillardet ggouaillardet mentioned this pull request Jun 20, 2017
Merged
@ggouaillardet
Copy link
Contributor Author

note 68ac950 should be added to this PR and PR'ed on the other branches. will do sometimes this week

@ggouaillardet ggouaillardet force-pushed the topic/v2.x/a2aw_zeros branch from 797c907 to b0001e9 Compare June 29, 2017 02:29
@hppritcha
Copy link
Member

@ggouaillardet please squash this down to one commit for PRs to other releases.

@ggouaillardet
Copy link
Contributor Author

@hppritcha this PR is for the v2.x branch and contains 4 commits that were cherry-picked or back-ported from master. are you sure you want me to squash them into one commit ?

@jsquyres
Copy link
Member

jsquyres commented Aug 3, 2017

@bosilca It's not clear from above -- did you review/approve this PR?

@jsquyres
Copy link
Member

jsquyres commented Aug 3, 2017

For documentation:

@bosilca
Copy link
Member

bosilca commented Aug 3, 2017

It is OK but suboptimal (missing #3719).

@jsquyres
Copy link
Member

jsquyres commented Aug 7, 2017

@ggouaillardet @bosilca #3719 is pretty giant. Is this PR correct and an improvement over what we had before? Specifically: do we need #3719 on v2.x?

@bosilca
Copy link
Member

bosilca commented Aug 8, 2017

This PR fixes an existing issue with v2.0. #3719 brings improvements in performance and code readability, but it is not necessary for correctness. We should also keep in mind that without #3719 the v2.x code will diverge from master making all future maintenance more complicated.

@ggouaillardet
coll/libnbc: fix a2aw_sched_linear() with zero size datatype or zero …
c1094d6 
…count

Signed-off-by: Gilles Gouaillardet <[email protected]>

(cherry picked from commit open-mpi/ompi@1294954)
@ggouaillardet
coll/libnbc: optimize zero size ialltoall{v,w} with MPI_IN_PLACE
96808ae 
and incidentally avoids malloc(0)

Thanks Lisandro Dalcin for the report

Fixes open-mpi#2945

Signed-off-by: Gilles Gouaillardet <[email protected]>

(cherry picked from commit open-mpi/ompi@e70a30c)
@ggouaillardet
coll/base: fix zero size datatype handling in mca_coll_base_alltoallv…
0fbebee 
…_intra_basic_inplace()

Signed-off-by: Gilles Gouaillardet <[email protected]>

(back-ported from commit open-mpi/ompi@68ac950)
@ggouaillardet
coll/base: optimize handling of zero-byte datatypes in mca_coll_base_…
30c3b35 
…alltoallv_intra_basic_inplace()

Signed-off-by: Gilles Gouaillardet <[email protected]>

(back-ported from commit open-mpi/ompi@d1c5955)
ggouaillardet added a commit to ggouaillardet/ompi that referenced this pull request Aug 8, 2017
@ggouaillardet
coll/libnbc: revisit NBC_Handle usage
b01e4d6 
make NBC_Handle (almost) an internal structure created
by NBC_Schedule_request()
use a local variable instead of what was previously handle->tmpbuf

Refs open-mpi#3487

Signed-off-by: Gilles Gouaillardet <[email protected]>
(cherry picked from commit 9ba85b8)
@ggouaillardet ggouaillardet mentioned this pull request Aug 8, 2017
Merged
@ggouaillardet
coll/libnbc: revisit NBC_Handle usage
608726a 
make NBC_Handle (almost) an internal structure created
by NBC_Schedule_request()
use a local variable instead of what was previously handle->tmpbuf

Refs open-mpi#3487

Signed-off-by: Gilles Gouaillardet <[email protected]>

(back-ported from commit open-mpi/ompi@9ba85b8)
@ggouaillardet ggouaillardet force-pushed the topic/v2.x/a2aw_zeros branch from 9424bb2 to 608726a Compare August 8, 2017 01:01
@ggouaillardet
Copy link
Contributor Author

@bosilca, i added #3817 to this PR
(and as you already noticed, i backported #3817 to v3.0.x)

bosilca
bosilca approved these changes Aug 8, 2017
View reviewed changes
Copy link
Member

@bosilca bosilca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ggouaillardet the PR looks good.

@hppritcha hppritcha merged commit d43a118 into open-mpi:v2.x Aug 9, 2017
ggouaillardet added a commit to ggouaillardet/ompi that referenced this pull request Sep 20, 2017
@ggouaillardet
coll/libnbc: revisit NBC_Handle usage
cc8de6e 
make NBC_Handle (almost) an internal structure created
by NBC_Schedule_request()
use a local variable instead of what was previously handle->tmpbuf

Refs open-mpi#3487

Signed-off-by: Gilles Gouaillardet <[email protected]>

(back-ported from commit open-mpi/ompi@9ba85b8)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bosilca bosilca bosilca approved these changes

Assignees

@bosilca bosilca

Labels
bug RM approved Target: v2.x
Projects
None yet
Milestone
v2.1.2
Development

Successfully merging this pull request may close these issues.
4 participants
@ggouaillardet @jsquyres @hppritcha @bosilca