Re: [Boost-mpi] Thread API

Subject: Re: [Boost-mpi] Thread API
From: Matthias Troyer (troyer_at_[hidden])
Date: 2012-11-28 10:41:43

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?

> 
> +  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));

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.

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
>