November 2009 - petsc-dev - mailman3.cels.anl.gov

Re: ptypef.c: petscdatatypegetsize_
by Satish Balay 18 Nov '09

18 Nov '09

I have: >>>>>> void PETSC_STDCALL petscdatatypegetsize_(PetscDataType *ptype,size_t *size, int *__ierr ){ *__ierr = PetscDataTypeGetSize(*ptype,size); } <<<<<<< Perhaps you have an old sowing/bfort package? satish On Wed, 18 Nov 2009, Jed Brown wrote: > I'm not getting a correct wrapper for PetscDataTypeGetSize. > > #define PetscToPointer(a) (*(long *)(a)) > > void PETSC_STDCALL petscdatatypegetsize_(PetscDataType *ptype,size_t *size, int *__ierr ){ > *__ierr = PetscDataTypeGetSize(*ptype, > (size_t* )PetscToPointer((size) )); > } > > Note that size is being dereferenced but should not be. This causes > sys/examples/tutorials/ex5f90 to be broken. > > Jed > >

1 0

Re: Should PetscObjectName be collective?
by Barry Smith 18 Nov '09

18 Nov '09

On Nov 18, 2009, at 8:52 AM, Jed Brown wrote: > http://petsc.cs.iit.edu/petsc/petsc-dev/rev/ecbd2dcad71a > > Note that there are a couple other places where keyvals are created > (with no way to free them). Does it make sense to put those fields > into this one? > > Is there a reason that petscinitialize_() duplicates much of > PetscInitialize()? Seems like we ought to have the common stuff in a > PetscInitialize_Private(). There was a reason for this duplication many years ago. I do not know if it is still needed. There is the business of the fortran "initialization" being done twice which may be fragile. You could try merging but test it relentlessly :-) Barry > > Jed >

1 0

Re: cleaning up two very nasty coding habits
by Lisandro Dalcin 18 Nov '09

18 Nov '09

On Wed, Nov 18, 2009 at 1:21 AM, Barry Smith <bsmith(a)mcs.anl.gov> wrote: > > When starting to code PETSc 2.0 I introduced two nasty habits > > 1) since we called malloc() directly and some mallocs don't like allocating > zero length I would often allocate 1 additional space to make sure I did not > request a zero size. Giving things like > PetscMalloc((n+1)*sizeof(something),&something); This leads to confusing > code because we don't know when the 1 belongs or not. > > 2) in order to reduce the number of mallocs I would do a single malloc for > several variables. For example, > PetscMalloc((n+m+1)*sizeof(PetscScalar),&buf1); buf2 = buf1 + n; > > Later I changed PetscMalloc()/Free() to allow 0 length mallocs. I also > introduced PetscMalloc2-3-4-5() to efficiently and cleanly share one malloc. > Thus 1 and 2 became unnecessary. Since they are downright disgusting I am > trying to eliminate 2) and would like also to reduce 1) > > This fixes will likely introduce some hard to determine bugs in the near > future (or already have). Please avoid habits 1 and 2 from now on in the > PETSc code base. > I would guess that almost all the (n+1) mallocs are unnecessary, except for "I"-like arrays associated to AIJ structures... -- Lisandro Dalcín --------------- Centro Internacional de Métodos Computacionales en Ingeniería (CIMEC) Instituto de Desarrollo Tecnológico para la Industria Química (INTEC) Consejo Nacional de Investigaciones Científicas y Técnicas (CONICET) PTLC - Güemes 3450, (3000) Santa Fe, Argentina Tel/Fax: +54-(0)342-451.1594

1 0

ptypef.c: petscdatatypegetsize_
by Jed Brown 18 Nov '09

18 Nov '09

I'm not getting a correct wrapper for PetscDataTypeGetSize. #define PetscToPointer(a) (*(long *)(a)) void PETSC_STDCALL petscdatatypegetsize_(PetscDataType *ptype,size_t *size, int *__ierr ){ *__ierr = PetscDataTypeGetSize(*ptype, (size_t* )PetscToPointer((size) )); } Note that size is being dereferenced but should not be. This causes sys/examples/tutorials/ex5f90 to be broken. Jed

1 0

Re: Should PetscObjectName be collective?
by Jed Brown 18 Nov '09

18 Nov '09

http://petsc.cs.iit.edu/petsc/petsc-dev/rev/ecbd2dcad71a Note that there are a couple other places where keyvals are created (with no way to free them). Does it make sense to put those fields into this one? Is there a reason that petscinitialize_() duplicates much of PetscInitialize()? Seems like we ought to have the common stuff in a PetscInitialize_Private(). Jed

1 0

cleaning up two very nasty coding habits
by Barry Smith 17 Nov '09

17 Nov '09

When starting to code PETSc 2.0 I introduced two nasty habits 1) since we called malloc() directly and some mallocs don't like allocating zero length I would often allocate 1 additional space to make sure I did not request a zero size. Giving things like PetscMalloc((n+1)*sizeof(something),&something); This leads to confusing code because we don't know when the 1 belongs or not. 2) in order to reduce the number of mallocs I would do a single malloc for several variables. For example, PetscMalloc((n+m +1)*sizeof(PetscScalar),&buf1); buf2 = buf1 + n; Later I changed PetscMalloc()/Free() to allow 0 length mallocs. I also introduced PetscMalloc2-3-4-5() to efficiently and cleanly share one malloc. Thus 1 and 2 became unnecessary. Since they are downright disgusting I am trying to eliminate 2) and would like also to reduce 1) This fixes will likely introduce some hard to determine bugs in the near future (or already have). Please avoid habits 1 and 2 from now on in the PETSc code base. Thanks Barry

1 0

Re: Should PetscObjectName be collective?
by Barry Smith 17 Nov '09

17 Nov '09

On Nov 17, 2009, at 6:01 PM, Jed Brown wrote: > Barry Smith wrote: >> >> On Nov 17, 2009, at 5:08 PM, Jed Brown wrote: > > Great. May as well make this a struct instead of an array > > typedef struct {PetscMPIInt tag,namecount,refcount;} > PetscCommAttribute; Yes, this is ok. Note that tag needs to be a PetscMPIInt but the other beasts could be anything you want. PetscInt, unsigned int etc. Barry > > > > Jed >

1 0

Re: Should PetscObjectName be collective?
by Jed Brown 17 Nov '09

17 Nov '09

Barry Smith wrote: > > On Nov 17, 2009, at 5:08 PM, Jed Brown wrote: >> Browsing tagm.c, >> MPI_Keyval_free is never called for the other attributes. This explains >> some valgrind warnings under Open MPI but not under MPICH (presumably >> MPICH frees zombie keyvals in MPI_Finalize() but OMPI does not). > > We should refactor the code to move the body of the code block below > into PetscIntialize() > if (Petsc_Tag_keyval == MPI_KEYVAL_INVALID) { > ierr = > MPI_Keyval_create(MPI_NULL_COPY_FN,Petsc_DelTag,&Petsc_Tag_keyval,(void*)0);CHKERRQ(ierr); > > ierr = > MPI_Keyval_create(MPI_NULL_COPY_FN,Petsc_DelComm,&Petsc_InnerComm_keyval,(void*)0);CHKERRQ(ierr); > > ierr = > MPI_Keyval_create(MPI_NULL_COPY_FN,Petsc_DelComm,&Petsc_OuterComm_keyval,(void*)0);CHKERRQ(ierr); > > } > and put a corresponding set of frees() into PetscFinalize(). Yup. > No, we don't want to share across MPI_Comm_dup(). They are different > communicators so should have their own name counters. Note that it would > be rare that there are MPI_Comm_dup()s lying around anyways unless the > user explicitly calls it. Thanks. > I am not sure what reference counting you are talking about but PETSc > currently reference counts how many objects reference a communicator > attribute (this is tagvalp[1]). When it gets down to zero it deletes > the attribute and the inner communicator. This is the reference counting I was referring to, it would be duplicated if we weren't reusing the attribute (which we couldn't do if they were to have different sharing semantics). > To get a counter for object names you can increase the size of > tagvalp[2] to tagvalp[3], then each time you get a name crank the value > up by 1, much like PetscCommGetNewTag() No need to add new attributes. Great. May as well make this a struct instead of an array typedef struct {PetscMPIInt tag,namecount,refcount;} PetscCommAttribute; Jed

1 0

Re: Should PetscObjectName be collective?
by Barry Smith 17 Nov '09

17 Nov '09

On Nov 17, 2009, at 5:08 PM, Jed Brown wrote: > Lisandro Dalcin wrote: >> On Mon, Nov 16, 2009 at 4:04 PM, Barry Smith <bsmith(a)mcs.anl.gov> >> wrote: >>> Ok, we could just put the "counter" into the MPI attribute and >>> then just >>> have uniqueness of the names within each communicator. This would >>> solve >>> Jed's problem cleanly. >>> >> >> I agree. +1 on this. > > If I understand correctly, every communicator would start at 0 with no > attempt to distinguish communicators. PetscObjectName would add an > attribute to each communicator it sees. Nope, don't want to do it this way, it increases the complexity. See below. > Browsing tagm.c, > MPI_Keyval_free is never called for the other attributes. This > explains > some valgrind warnings under Open MPI but not under MPICH (presumably > MPICH frees zombie keyvals in MPI_Finalize() but OMPI does not). We should refactor the code to move the body of the code block below into PetscIntialize() if (Petsc_Tag_keyval == MPI_KEYVAL_INVALID) { ierr = MPI_Keyval_create(MPI_NULL_COPY_FN,Petsc_DelTag,&Petsc_Tag_keyval, (void*)0);CHKERRQ(ierr); ierr = MPI_Keyval_create (MPI_NULL_COPY_FN,Petsc_DelComm,&Petsc_InnerComm_keyval, (void*)0);CHKERRQ(ierr); ierr = MPI_Keyval_create (MPI_NULL_COPY_FN,Petsc_DelComm,&Petsc_OuterComm_keyval, (void*)0);CHKERRQ(ierr); } and put a corresponding set of frees() into PetscFinalize(). > I see > that Petsc_InnerComm_keyval and Petsc_OuterComm_keyval just uses the > pointer to hold the value, since the counter changes we need to > actually > malloc. > > Do we want the counter to hold a reference count (and be shared across > MPI_Comm_dup)? No, we don't want to share across MPI_Comm_dup(). They are different communicators so should have their own name counters. Note that it would be rare that there are MPI_Comm_dup()s lying around anyways unless the user explicitly calls it. > This is slightly tricky because it's not normally > obvious to a user when a duplication occurs, but the numbering would > be > different if the comm was duped before or after the attribute was set > (unpredictable if it's set in PetscObjectName()). OTOH, > PetscCheckSameComm will say duped communicators are equivalent so we > really have to share the counter. I'm talking myself into starting a > reference-counted counter the first time PETSc touches a communicator. I am not sure what reference counting you are talking about but PETSc currently reference counts how many objects reference a communicator attribute (this is tagvalp[1]). When it gets down to zero it deletes the attribute and the inner communicator. To get a counter for object names you can increase the size of tagvalp[2] to tagvalp[3], then each time you get a name crank the value up by 1, much like PetscCommGetNewTag() No need to add new attributes. Barry > > > Jed >

1 0

Re: Should PetscObjectName be collective?
by Jed Brown 17 Nov '09

17 Nov '09

Lisandro Dalcin wrote: > On Mon, Nov 16, 2009 at 4:04 PM, Barry Smith <bsmith(a)mcs.anl.gov> wrote: >> Ok, we could just put the "counter" into the MPI attribute and then just >> have uniqueness of the names within each communicator. This would solve >> Jed's problem cleanly. >> > > I agree. +1 on this. If I understand correctly, every communicator would start at 0 with no attempt to distinguish communicators. PetscObjectName would add an attribute to each communicator it sees. Browsing tagm.c, MPI_Keyval_free is never called for the other attributes. This explains some valgrind warnings under Open MPI but not under MPICH (presumably MPICH frees zombie keyvals in MPI_Finalize() but OMPI does not). I see that Petsc_InnerComm_keyval and Petsc_OuterComm_keyval just uses the pointer to hold the value, since the counter changes we need to actually malloc. Do we want the counter to hold a reference count (and be shared across MPI_Comm_dup)? This is slightly tricky because it's not normally obvious to a user when a duplication occurs, but the numbering would be different if the comm was duped before or after the attribute was set (unpredictable if it's set in PetscObjectName()). OTOH, PetscCheckSameComm will say duped communicators are equivalent so we really have to share the counter. I'm talking myself into starting a reference-counted counter the first time PETSc touches a communicator. Jed

1 0