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
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
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.
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
