On Thu, July 26, 2007 12:20 pm, Brian Barrett wrote:
> 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
>
I don't, there was one #if that i just removed..
but we do need to check in some cases like in ompi_comm_get_rprocs that we
are not using the direct access to the pointers list. like for example:
if(OMPI_GROUP_IS_DENSE(local_comm->c_local_group)) {
rc = ompi_proc_pack(local_comm->c_local_group->grp_proc_pointers,
local_size, sbuf);
}
/* get the proc list for the sparse implementations */
else {
proc_list = (ompi_proc_t **) calloc
(local_comm->c_local_group->grp_proc_count,
sizeof (ompi_proc_t *));
for(i=0 ; i<local_comm->c_local_group->grp_proc_count ; i++)
proc_list[i] =
GROUP_GET_PROC_POINTER(local_comm->c_local_group,i);
rc = ompi_proc_pack (proc_list, local_size, sbuf);
}
here if sparse groups are disabled, we don't really want to allocate and
set this list of pointers that already exists (not to waste more memory
and time)..
> 2) I think it would be better to always have the flags and macros
> available (like OMPI_GROUP_SPORADIC and OMPI_GROUP_IS_SPORADIC) even
> when sparse groups are disabled. They dont' take up any space, and
> mean less #ifs in the general code base
>
That's what i actually was proposing.. keep the flags (there are no
macros, just the GROUP_GET_PROC_POINTER) and the sparse parameters in the
group strucutre, and this will mean, only 1 maybe 2 #ifs..
> 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
> debug.
Actually i never knew till this morning that this function was in the
code.. I have an inline function ompi_group_lookup (which does the same
thing), that actually checks if the group is dense or not and act
accordingly.. but to use the inline function instead of the macro, means
again that we need to compile in all the sparse parameters/code, which im
for..
>
> Other than that, I think you've got my concerns pretty much addressed.
>
> Brian
>
> 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 comm.group.proc[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
>>> de...@open-mpi.org
>>> http://www.open-mpi.org/mailman/listinfo.cgi/devel
>>>
>>
>>
>> --
>> Mohamad Chaarawi
>> Instructional Assistant http://www.cs.uh.edu/~mschaara
>> Department of Computer Science University of Houston
>> 4800 Calhoun, PGH Room 526 Houston, TX 77204, USA
>>
>> _______________________________________________
>> devel mailing list
>> de...@open-mpi.org
>> http://www.open-mpi.org/mailman/listinfo.cgi/devel
>
> _______________________________________________
> devel mailing list
> de...@open-mpi.org
> http://www.open-mpi.org/mailman/listinfo.cgi/devel
>
--
Mohamad Chaarawi
Instructional Assistant http://www.cs.uh.edu/~mschaara
Department of Computer Science University of Houston
4800 Calhoun, PGH Room 526 Houston, TX 77204, USA
