Re: [mpi3-coll] New nonblocking collective intro text
Thanks for your review, Christian. I personally had trouble parsing the whole picture in the original text. I know it's a fairly big change, but it's mostly just reordering of the existing content. I felt it was important to do since this introduces new readers to the NBC concept. -Adam Christian Siebert wrote:
Dear Adam and all,
I very much appreciate many of your textual improvements. However, it is very time consuming for us as reviewers to keep track of such substantial changes. It would have been much easier, if you had contributed your changes in an earlier stage. Anyway, I waded through the changes to revision 3 and will comment only on the things that I don't like (consider everything else as excellent):
I second Torsten's opinion on p. 49, l. 35-37: This reformulation introduces three times the term "complete". Besides this drawback, I also don't see any benefit from this reformulation compared to the previous version.
It's a bit repetitive, I know. To make a direct parallel, I copied the same sentence structure used in the nonblocking point-to-point section (see the 2.1 document, page 47, lines 46-48). We could drop the "but does not complete it" clause to shorten this some (saves one "complete"): "A nonblocking start call initiates a collective operation, which must be completed in a separate completion call."
p. 50, l. 1-2: Similar problem (i.e. 2x"complete"). May I propose: "The start call returns a request handle, which is eventually passed to a completion call."
I'd be ok with dropping a "complete" here, as well, although I prefer "which must be passed" over "which is eventually passed": "The start call returns a request handle, which must be passed to a completion call."
p. 50, l. 8: The part "unless otherwise specified in, or implied by, the description of the operation" has been added. Seem ok to me at first glance. However, we might re-check if this is really necessary and drop it otherwise.
I added the "implied by" to address cases like MPI_Alltoall, where there is no specified synchronization as in MPI_Barrier, but clearly a process can't complete until it has received data from every other process and thus knows all other processes have started the call. I have no objections to dropping one of these two phrases. Going with just "implied by" leads to a cyclic-sounding statement. "Completion does not imply that other processes have completed or even started the operation unless otherwise implied by the description of the operation." We could just go with "specified in": "Completion does not imply that other processes have completed or even started the operation unless otherwise specified in the description of the operation."
p. 50, l. 16: This looks quite redundant to me. I'd propose sth. like the following restatement: "Upon completion of a nonblocking collective operation, the MPI_ERROR field...".
Yes, that's fine.
p. 50, l. 17: The added postfix "to indicate any errors" seems wrong to me because MPI_SUCCESS is also a valid entry for the MPI_ERROR field. I'd propose to simply omit this postfix. However, I'm not 100% sure because the section about the MPI Status object looks quite confusing in this respect. Btw. there exists also an empty status (which sets all 3 fields) - so we might need to reconsider our decision about setting only the error field for nonblocking collective operations.
Yes, I agree, but I'll leave this alone as it is a bigger discussion for the group.
p. 50, l. 29: "nonblocking collective communications" => "nonblocking collective operations"
Sure.
p. 50, l. 44: You changed "running time" to "minimal time to completion" - this is _not_ the same thing! I'd strongly prefer to stay with the previous terminology.
I meant to go with whatever we had after the telecon here. This statement is only trying to make a point to the reader that there are reasonably two different algorithms possible here. It does not demand what an MPI implemenation must do. I'm happy with either wording.
p. 62, l. 36/37: now we have two consecutive "the"s
Yep, nice catch.
Many thanks!
Christian
participants (1)
-
Adam Moody