Open MPI logo

Open MPI Development Mailing List Archives

  |   Home   |   Support   |   FAQ   |   all Development mailing list

From: Brian Barrett (bbarrett_at_[hidden])
Date: 2007-07-26 13:20:06

Mohamad -

A couple of comments / questions:

1) Why do you need the #if OMPI_GROUP_SPARSE in communicator/comm.c?
That seems like
    part of the API that should under no conditions change based on
sparse/not sparse

2) I think it would be better to always have the flags and macros
when sparse groups are disabled. They dont' take up any space, and
mean less #ifs in the general code base

3) Instead of the GROU_GET_PROC_POINTER macro, why not just change
the behavior of the ompi_group_peer_lookup() function, so that there
is symmetry with how you get a proc from a communicator? static
inline functions (especially short ones like that) are basically
free. We'll still have to fix all the places in the code where the
macro is used or people poke directly at the group structure, but I
like static inline over macros whenever possible. So much easier t

Other than that, I think you've got my concerns pretty much addressed.


On Jul 25, 2007, at 8:45 PM, Mohamad Chaarawi wrote:

> In the current code, almost all #ifs are due to the fact that we don't
> want to add the extra memory by the sparse parameters that are
> added to
> the group structure.
> The additional parameters are 5 pointers and 3 integers.
> If nobody objects, i would actually keep those extra parameters,
> even if
> sparse groups are disabled (in the default case on configure),
> because it
> would reduce the number of #ifs in the code to only 2 (as i recall
> that i
> had it before) ..
> Thank you,
> Mohamad
> On Wed, July 25, 2007 4:23 pm, Brian Barrett wrote:
>> On Jul 25, 2007, at 3:14 PM, Jeff Squyres wrote:
>>> On Jul 25, 2007, at 5:07 PM, Brian Barrett wrote:
>>>>> It just adds a lot of #if's throughout the code. Other than that,
>>>>> there's no reason to remove it.
>>>> I agree, lots of #ifs are bad. But I guess I don't see the
>>>> problem.
>>>> The only real important thing people were directly accessing in the
>>>> ompi_group_t is the array of proc pointers. Indexing into them
>>>> could
>>>> be done with a static inline function that just has slightly
>>>> different time complexity based on compile options. Static inline
>>>> function that just does an index in the group proc pointer would
>>>> have
>>>> almost no added overhead (none if the compiler doesn't suck).
>>> Ya, that's what I proposed. :-)
>>> But I did also propose removing the extra #if's so that the sparse
>>> group code would be available and we'd add an extra "if" in the
>>> critical code path.
>>> But we can do it this way instead:
>>> Still use the MACRO to access proc_t's. In the --disable-sparse-
>>> groups scenario, have it map to[i]. In the --
>>> enable-
>>> sparse-groups scenario, have it like I listed in the original
>>> proposal:
>>> static inline ompi_proc_t lookup_group(ompi_group_t *group, int
>>> index) {
>>> if (group_is_dense(group)) {
>>> return group->procs[index];
>>> } else {
>>> return sparse_group_lookup(group, index);
>>> }
>>> }
>>> With:
>>> a) groups are always dense if --enable and an MCA parameter turns
>>> off
>>> sparse groups, or
>>> b) there's an added check in the inline function for whether the MCA
>>> parameter is on
>>> I'm personally in favor of a) because it means only one conditional
>>> in the critical path.
>> I don't really care about the sparse groups turned on case. I just
>> want minimal #ifs in the global code and to not have an if() { ... }
>> in the critical path when sparse groups are disabled :).
>> Brian
>> _______________________________________________
>> devel mailing list
>> devel_at_[hidden]
> --
> Mohamad Chaarawi
> Instructional Assistant
> Department of Computer Science University of Houston
> 4800 Calhoun, PGH Room 526 Houston, TX 77204, USA
> _______________________________________________
> devel mailing list
> devel_at_[hidden]