Re: [Boost-mpi] Thread API

Subject: Re: [Boost-mpi] Thread API
From: Hal Finkel (hfinkel_at_[hidden])
Date: 2012-11-28 11:21:32

----- Original Message -----
> From: "Matthias Troyer" <troyer_at_[hidden]>
> To: "Discussion of Boost.MPI development" <boost-mpi_at_[hidden]>
> Sent: Wednesday, November 28, 2012 9:41:43 AM
> Subject: Re: [Boost-mpi] Thread API
> 
> 
> On Nov 28, 2012, at 10:21 AM, Hal Finkel <hfinkel_at_[hidden]> wrote:
> 
> > ----- Original Message -----
> >> From: "Matthias Troyer" <troyer_at_[hidden]>
> >> To: "alain miniussi" <alain.miniussi_at_oca.eu>, "Discussion of
> >> Boost.MPI development" <boost-mpi_at_[hidden]>
> >> Sent: Wednesday, November 28, 2012 8:25:18 AM
> >> Subject: Re: [Boost-mpi] Thread API
> >> 
> >> Hi Alain,
> >> 
> >> 
> >> On Nov 27, 2012, at 5:11 PM, Alain O Miniussi
> >> <alain.miniussi_at_oca.eu>
> >> wrote:
> >> 
> >>> Hi,
> >>> 
> >>> I have a tentative patch that is supposed to  provide binding to
> >>> the MPI
> >>> thread API. What is the best way to have it reviewed ?
> >> 
> >> Thank you for your work. I have a few stylistic comments - shall I
> >> send them to you off-list, or shall we discuss them on our new
> >> mailing list?
> > 
> > If you don't mind, I'd prefer on-list reviews (even for stylistic
> > matters). That way we can all benefit from your advice.
> > 
> > I also have a few comments:
> > 
> > +enum level {
> > +  /** Only one thread will execute.
> > +   */
> > +  SINGLE     = MPI_THREAD_SINGLE,
> > +  /** Only main thread will do MPI calls.
> > 
> > I think that we should use lower-case enum constant names (single
> > instead of SINGLE).
> 
> Thank you, that was one of the points I also wanted to raise.
> 
> Another comment was that I would prefer threading::multiple over
> mt::multiple - would you mind changing the namespace?

I also like this better.

> 
> 
> > 
> > +  mt::level mt_level = mt::MULTIPLE;
> > +  boost::mpi::environment env(argc, argv, mt_level);
> > +  mt::level provided = env.thread_level();
> > +  std::cout << "Asked:" << mt_level << ", provided: " << provided
> > << '\n';
> > +  BOOST_CHECK((provided >= mt::SINGLE && provided <=
> > mt::MULTIPLE));
> > 
> > I have a lot of code that looks like this ;) -- but I think that it
> > could be greatly simplified. How about if the environment
> > constructor throws an exception if the provided level is less than
> > the requested level. This way, in the common case where the
> > program has no fall-back provisions, we don't accidentally
> > continue under an invalid environment (many programs don't
> > explicitly check the provided level -- and this obviously leads to
> > odd behavior and crashes).
> 
> A very good point. One might think about a constructor that has the
> more relaxed semantics but we might want to specify this explicitly,
> like on
> 
> boost::mpi::environment env(argc, argv,
> threading::attempt(threading::multiple));

That seems reasonable.

> 
> or similar.
> 
> I have another small worry. We have two similar constructors now:
> 
>    environment(int& argc, char** &argv, bool abort_on_exception =
>    true);
>    environment(int& argc, char** &argv, mt::level mt_level,  bool
>    abort_on_exception = true);
> 
> Should we worry about legacy codes that passes an int as third
> argument? We could avoid that by making the threading level a class
> instead of an enum, or a strongly typed enum in C++11.

I'm not terribly worried about that; having the enum constants interchangeable with the MPI constants seems convenient.

 -Hal

> 
> Matthias
> 
> 
> > Thanks for working on this!
> > 
> > -Hal
> > 
> >> 
> >>> It might be a ticket
> >>> buthttps://svn.boost.org/trac/boost/report/15the
> >>> referenced maintainer is Doug
> >>> (https://svn.boost.org/trac/boost/report/15)
> >>> and, probably since I have no userid in the tracker, I won't be
> >>> able to
> >>> change it.
> >> 
> >> I'll try to find out how to change it. Doug does not have time
> >> anymore.
> >> 
> >>> Just in case, there is a patch in attachment (against rev 81596
> >>> of
> >>> trunk).
> >> 
> >> 
> >> Thank you!
> >> 
> >> Matthias
> >> 
> >> _______________________________________________
> >> Boost-mpi mailing list
> >> Boost-mpi_at_[hidden]
> >> http://lists.boost.org/mailman/listinfo.cgi/boost-mpi
> >> 
> > 
> > --
> > Hal Finkel
> > Postdoctoral Appointee
> > Leadership Computing Facility
> > Argonne National Laboratory
> > _______________________________________________
> > Boost-mpi mailing list
> > Boost-mpi_at_[hidden]
> > http://lists.boost.org/mailman/listinfo.cgi/boost-mpi
> > 
> 
> _______________________________________________
> Boost-mpi mailing list
> Boost-mpi_at_[hidden]
> http://lists.boost.org/mailman/listinfo.cgi/boost-mpi
> 

-- 
Hal Finkel
Postdoctoral Appointee
Leadership Computing Facility
Argonne National Laboratory