[petsc-dev] threadcomm memory leak

[Barry Smith]

On Jul 16, 2012, at 9:51 PM, Jed Brown wrote:

> On Mon, Jul 16, 2012 at 6:53 PM, Barry Smith <bsmith at mcs.anl.gov> wrote:
>    Yes, that is absolutely a hack and does not belong there. But you are totally miss understanding what I am saying: that hack is NEW. For 15 years PETSc did NOT need a hack to work with MPIUNI (which has a single communicator), thus I conclude that MPUNI is fine and something is wrong with the PETSc thread comm stuff if it requires that hack. That is, why the heck does petscthreadcomm depend on MPI_COMM_SELF != MPI_COMM_WORLD while for 20 years NOTHING ELSE IN PETSc (which is a dang lot more complicated than petscthreadcomm) does not depend on MPI_COMM_SELF != MPI_COMM_WORLD???
> 
> As far as I know, the other calls to MPI_Attr_put occur the first time the comm is used with an object rather than being pre-initialized. In this case, PETSC_COMM_SELF and PETSC_COMM_WORLD are pre-endowed with the threadcomm. Should the threadcomm be placed in some global variable and obtained (and referenced) the first time it is used with a given communicator? If we store the canonical one on a comm, then it must be on COMM_WORLD and COMM_SELF.

  /* PETSC_COMM_SELF = PETSC_COMM_WORLD for MPIUNI */
#if !defined(PETSC_HAVE_MPIUNI)
  ierr = PetscCommDuplicate(PETSC_COMM_WORLD,&icomm,PETSC_NULL);CHKERRQ(ierr);
  ierr = MPI_Attr_put(icomm,Petsc_ThreadComm_keyval,(void*)tcomm);CHKERRQ(ierr);
  tcomm->refct++;               /* Share the threadcomm with PETSC_COMM_SELF */
#endif

  ierr = PetscCommDuplicate(PETSC_COMM_SELF,&icomm,PETSC_NULL);CHKERRQ(ierr);
  ierr = MPI_Attr_put(icomm,Petsc_ThreadComm_keyval,(void*)tcomm);CHKERRQ(ierr);


  I would not do it this way. Instead I would write a general routine that attached a threadcomm to a MPI_Comm; this routine would get the threadcomm_keyval and if it did NOT find it then would be put the attribute, otherwise it would know one was already there. Say it is called PetscThreadCommAttach(MPI_Comm, threadcomm); then in this routine you would just write

    PetscThreadCommAttach(PETSC_COMM_WORLD, tcomm);
    PetscThreadCommAttach(PETSC_COMM_SELF,tcomm);        /* won't attr it again for MPIUni because it is already there */


>  
> 
>    In other words fix petscthreadcomm model; don't mess with a perfectly good mpiuni.
> 
> MPI has them as separate communicators. MPIUNI breaks that model for, as far as I can tell, no good reason.

1 Philosophical reason)  The natural limitation of MPI to one process is to have a single communicator; having two different ones that are "identical" is unnatural 

2  Practical reason) MPIUNI would be more complicated to handle two sets of attributes etc. Plus if you do this you  really need to support MPI_Comm_dup as creating yet another "different" communicator making the code even more complicated since now you need to manage duping and freeing comms, becomes more like a real MPI implement. Not worth it.

3  It finds bugs in the design of new components like PetscThreadComm that make assumptions and puts an attribute without first checking if it is already there :-).

   Barry