[mpi3-coll] Comments on nonblocking collectives section

[Torsten Hoefler]

Hi Jesper,
> sorry for my late input. I've had time to read the document, and have found 
> something. I refer to the version of 21st January.
Thanks for your review - I'll comment below!

> p. 49, line 26:
> "the performance of many systems..." -> 
> "the performance of many applications can be improved by overlapping communication
> and computation, and many systems enable this."
fixed

> p. 49, line 28:
> "utilize overlap -> "exploit overlap"
fixed

> p. 49, line 32:
> drop "the use of" (grammar: "the use of" is no mechanism)
nb collective communication doesn't increase performance by itself, only
the correct *use* of them leads to higher performance, cf. MPI-2.1
p47:45. I changed "the use of" to "to use" which might be better
English.

> p.49, line 35:
> "a collective communication, which..." -> "a collective communication
> operation, that..." ("that" better than "which" here?)
don't know - I'll leave it as is since Bronis didn't comment on this
before

> p.49, line 36:
> "the communication may progress" -> "the collective communication may
> then progress"
no, there is no temporal description, because we talk about initiation
*and* completion in the previous sentence. This sentence is clear(er)
without this change.

> p.49, line 38:
> "mitigate synchronizing effects" -> "mitigate synchronization effects"
no, "synchronization effects" sounds more like they're supposed to
synchronize (like barrier) to me while "synchronizing effects" sounds
more like it's meant - nasty effects that synchronize which has negative
impact to the app. That's my personal interpretation, we can discuss it.

> p. 50, line 9-11: 
> drop sentence "Collective operations...", allready said in line 4-7?
fixed, added the buffer access rule to 4-7

> p.50, after line 18, add:
> "Nonblocking collective requests are not persistent, and
> MPI_REQUEST_NULL is returned after successful completion."
I added "Nonblocking collective requests are not persistent." Returning
REQUEST_NULL is part of the semantics of Test/Wait and friends. I would
rather not scatter them throughout the document.

> p. 50, line 18: 
> I still think that MPI_Request_free should be allowed.
we had this question asked 5 times in the woring group and two times to
the whole Forum. We reached the same consensus everytime. 

> The rationale also still reads contrived.
I think the rationale is perfectly fine. Please point at particular
weaknesses or propose an alternative.

If you think that it's contrived, then give us an example of how to
check the completion of an MPI_Ialltoall where the request was freed in
a portable way (yes, with all buffering TCP implementations, buffering
out-of-order UDP (or whatever) implementations, and without any memory
barrier semantics). Some things work (Bcast), but most just don't, thus
we should not encourage the use of such nonportable techniques (cf.
Erez' original ticket/discussion). 

Please bring it up again if you think it needs more discussion!

> p.50, line 20 (or elsewhere), add:
> "Completion of a particular nonblocking collective operations does
> \emph{not} imply completion of any other posted nonblocking collective
> (or send-receive) operations, whether they are posted before or after
> the operation in case."
hmm, I think this clarification is unnecessary because similar things
would apply to nonblocking point-to-point and are not mentioned there
(and the semantics are clearly defined). We have Example 5.34 to make
this clear, but I'd rather not introduce this to the introductory text.

> p.50, line 24:
> "Canceling a request" -> "Canceling a nonblocking request"
hmm, aren't all requests nonblocking? And it's clear (from the context)
that it refers to requests associated to nonblocking collectives.

> p.50, line 24-25:
> "cannot be defined clearly." -> "is not well-defined."
I think that "cannot be defined" is stronger than "is not well-defined". 

> p.50, line 33:
> "In this sense"?? What does this refer to? Drop, just say "Blocking
> and nonblocking..."
fixed

> p.50, line 35:
> "In particular," -> "This means that"
fixed

> p.50, line 44:
> "could find an equilibrium between" -> "could attempt to balance"
fixed (also added a missing comma before "and")

> p. 50, line 47-48:
> "than for point-to-point operations." ->
> "than for point-to-point operations, where for instance a blocking
> send can be matched by a nonblocking receive."
added

> p.50, line 48:
> "collective operations is required" -> "collective operation semantics
> is required"
I don't understand this proposal and I think the current text is
precise.

> p.51, line 6 (or elsewhere), add:
> "In terms of data movements each nonblocking collective operation upon
> completion has the same effect as its blocking counterpart, both for
> intracommunicators and intercommunicators. The use of the "in place"
> option is allowed exactly as described for the corresponding blocking
> collective operations. Likewise, the nonblocking collective reduction
> operations upon completion have the same effect as their blocking
> counterparts, and the same restrictions and recommendations on
> reduction orders apply."
excellent proposal! I re-phrased it slightly. The sentence about
reductions might be what Rolf wanted.

> I think this is quite important for the semantic definition of the
> nonblocking collectives.  The individual remarks for each of the
> operations could be dropped, although I think it is good to have them
> there (for people just checking the one operation they are interested
> in). However, the English is in most of the individual cases not good,
> see below.
yes, I would not drop them (they're short anyway).

> p.51, line 26-30:
> Advice reads a bit awkward, but have no suggestion.
I think it's clear.

> p.53, line 37:
> "are identical as after a call" - bad Engish, should rather be:
> "are the same as after a call".
fixed (twice)

> I would strongly suggest using exactly the same phrase for all of the
> data moving collectives! This is not a document being read as a whole,
> so we don't need to fear "repetition".
maybe, I think the wording is clear as it is and has been reviewed
multiple times. Thus, it should be fine. Feel free to propose a unified
wording that does not invalidate all previous reviews ;).

> Perhaps add:
> "The "in place" option is allowed for intracommunicators, with the same
> effect as for MPI_Gather."
we would have to add this to every collective. I think it's clear with
the paragraph we added above. 

> p, 54, line 41: Language!
hmm, sounds correct to me

> p. 55, line 35: Same
> p. 56, line 34: Same
> p. 57, line 32: Same
> p. 58, line 26: Same
> p. 59, line 32: Same
> p. 60, line 28: Same
> p. 61, line 47: Reads sort of ok here, would still be in favor of using the same phrase
> everywhere.
I'd propose to stay with the reviewed phrasing.

> p. 62, line 35-39:
> Drop advice to users, but add "The same rules and restrictions as for
> MPI_Reduce apply." (consistency is no hobgoblin here, and I think it's
> important that the nonblocking collectives "does" the same as their
> blocking counterparts)
there is no advice to users? We will discuss the changes to the advice
to implementors today. 

> p. 63, line 26: Language.
> p. 64, line 11: Sort of OK.
> p. 65, line 25: Sort of OK.
seems clear to me

> p.68, line 21: does a race condition "cause" non-determinism, or is it
> the other way round?
I think it's correct because it might match correctly, however, if the
race is done in the wrong order, then it's wrong. The casual chain is
here: programming mistake -> race condition -> nondeterminism

> p. 68, line 26:
> move "outstanding" to "even if there is an outstanding nonblocking..."
that's a matter of taste

> p. 69, after line 34:
> Would suggest an example 5.31 that shows that e.g. MPI_IAlltoall does
> not match MPI_Alltoall.
>
> "Blocking and nonblocking collective operations do not match. The
> following is illegal:"
added (this should be clear after Ex 5.29, but more examples can't harm)

> p.70, line 6:
> "the "MPI_Recv." -> "the MPI_Recv call."
fixed

> p. 70, line 42:
> "lower performance" (sounds too colloqial) -> "impact performance negatively." or
> "have a negative effect on performance."
fixed

All changes have been applied to Rev. 2. We will discuss those (and
possibly more) changes in today's telecon. THe document is available
here:
pdf:  http://www.unixer.de/sec/nbc-proposal-rev-2.pdf
diff: http://www.unixer.de/sec/nbc-proposal-rev-2.diff

I will attach it to ticket #109 after the telecon.

Best,
  Torsten

-- 
 bash$ :(){ :|:&};: --------------------- http://www.unixer.de/ -----
Torsten Hoefler       | Postdoctoral Researcher
Open Systems Lab      | Indiana University    
150 S. Woodlawn Ave.  | Bloomington, IN, 474045, USA
Lindley Hall Room 135 | +01 (812) 855-3608