[MOAB-dev] parallel read bug, interface awkwardness

[Jed Brown]

I have identified a MOAB parallel read bug.  It appears that if all
nodes of an edge or face are shared, MOAB sometimes assumes that the
edge/face is also shared.  This is incorrect since the face might
actually be a boundary face.  I think that if

0 --- 1 --- 2
|  A  |  B  |
3 --- 4 --- 5
|  C  |  D  |
6 --- 7 --- 8
|  E  |  F  |
9 --- 10 -- 11

appears on a boundary where proc 0 owns all faces except face D which is
owned by proc 1.  Then proc 0 will own all edges except 5-8, but proc 1
will think that 5-8 and face D are also owned by proc 0.  The attached
tarball demonstrates the issue.  If you unpack and run `make break' you
will see that the shared sets don't agree.  Run `make repartition' to
produce a new partition (likely different because mbzoltan is
nondeterministic).  I have run `make repartition break' a few times and
there are always sets which don't agree.


I (valgrind) found a trivial memory leak in ReadParallel (ownership of
the communicator).  The attached patch provides a naive fix.


The rest of this email relates to what seems like iMesh interface
awkwardness.  You might notice code like the following in pread.c

  char pstatus,*silly=&pstatus;
  int one=1,one1;
  iMesh_getEntSetData(mesh,set,pstatusTag,&silly,&one,&one1,&err);ICHK(mesh,err);
  /* handle set according to pstatus, (silly,one,one1 never used again) */

I realize it's sometimes convenient to let the implementation allocate
memory, but needing to have a pointer to the memory as well as variables
holding the amount of allocated space and the used space causes some
awkwardness.  I think it would be much cleaner to have a non-allocating
and (optionally) allocating interface rather than a single and array
interface.  Also, it could support different data types in much the same
way that MPI does.  Consider (it's radical, I know!)

  iMesh_getTagData(iMesh_Instance mesh, iBase_TagHandle tag,
                   void *handles, int handles_count,
                   void *data, int data_count, int data_type, int *err);

where my formerly awkward call would now look like

  iMesh_getTagData(mesh,pstatusTag,&set,1,&pstatus,1,iBase_CHAR,&err);

and this single function would subsume all existing
getXxxData()/getXxxArrData()/getEntSetXxxData() functions and could
easily be extended to support user-defined data types so that structured
tag types could be used in a heterogeneous environment.  If user-defined
types such as MPI_Type_indexed were used, it would be possible to unpack
tag data directly into the user's data structure.

I'm tempted to implement iMesh_getTagData() in terms of the existing
functions just because it would be much cleaner to use in my code.


For the current (allocating) interface, it would be nice if the user
could specify an alternate malloc.


I'm not sure exactly what the interface intends, but there should be a
way to ignore in_entity_set in iMesh_getAllVtxCoords() and
iMesh_getAdjEntities().  It seems from the interface spec that if
entity_set_handle=0 then it should be okay to pass in_entity_set=NULL,
in_entity_set_alloc=NULL, in_entity_set_size=NULL but this is not okay
in the MOAB implementation.

Jed
-------------- next part --------------
A non-text attachment was scrubbed...
Name: moab-pread-bug.tar.gz
Type: application/octet-stream
Size: 12356 bytes
Desc: not available
URL: <https://lists.mcs.anl.gov/mailman/private/moab-dev/attachments/20080922/24c79826/attachment.obj>
-------------- next part --------------
Index: parallel/ReadParallel.cpp
===================================================================
--- parallel/ReadParallel.cpp	(revision 2068)
+++ parallel/ReadParallel.cpp	(working copy)
@@ -40,14 +40,22 @@
       
 ReadParallel::ReadParallel(MBInterface* impl, 
                            MBParallelComm *pc) 
-        : mbImpl(impl), myPcomm(pc) 
+        : mbImpl(impl), myPcomm(pc), ownPcomm(false)
 {
   if (!myPcomm) {
     myPcomm = MBParallelComm::get_pcomm(mbImpl, 0);
-    if (NULL == myPcomm) myPcomm = new MBParallelComm(mbImpl);
+    if (NULL == myPcomm) {
+      myPcomm = new MBParallelComm(mbImpl);
+      ownPcomm = true;
+    }
   }
 }
 
+ReadParallel::~ReadParallel()
+{
+  if (ownPcomm) delete myPcomm;
+}
+
 MBErrorCode ReadParallel::load_file(const char **file_names,
                                     const int num_files,
                                     MBEntityHandle& file_set,
Index: parallel/ReadParallel.hpp
===================================================================
--- parallel/ReadParallel.hpp	(revision 2068)
+++ parallel/ReadParallel.hpp	(working copy)
@@ -35,7 +35,7 @@
   ReadParallel(MBInterface* impl = NULL, MBParallelComm *pc = NULL);
 
    //! Destructor
-  virtual ~ReadParallel() {}
+  virtual ~ReadParallel();
 
   static const char *parallelOptsNames[];
   
@@ -76,6 +76,7 @@
 
     // each reader can keep track of its own pcomm
   MBParallelComm *myPcomm;
+  bool ownPcomm;
 };
 
 inline MBErrorCode ReadParallel::load_file(const char *file_name,
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: not available
URL: <https://lists.mcs.anl.gov/mailman/private/moab-dev/attachments/20080922/24c79826/attachment.pgp>