[petsc-dev] Remove calls to XXXReset() in XXXDestroy()

[Lisandro Dalcin]

On 12 May 2011 23:55, Barry Smith <bsmith at mcs.anl.gov> wrote:
>
> On May 12, 2011, at 3:47 PM, Lisandro Dalcin wrote:
>
>> I would like to remove the calls to XXXReset() in XXXDestroy(),
>> probably refactoring the code to reuse a XXXReset_Private(). My
>> intention is to have the two things separated, object destruction is a
>> bit delicate and requires some care in handling Python subtypes. I
>> really prefer subtype implementors not to rely on ops->reset() being
>> called before ops->destroy(). Any oposition?
>
>    Yes. You don't provide an explanation about why you want to do this, besides "object destruction is delicate". Please formulate a complete explanation why this change would be needed and what the alternatives are.

When you call XXXDestroy(), then eventually the reference count drops
to zero, and continues to be zero while XXXDestroy() is executing. In
petsc4py, I've decided long ago to make each Python object to own a
ref to the PETSc handle. Then, in order to let users implement a "def
destroy(self, mat/pc): ..." for MAT/PC/PYTHON, I need to create a new
Python object owning a reference to the PETSc Mat/Vec, and then pass
the object to the call. This causes double calls to XXXDestroy() while
the first XXXDestroy() is being executing. Then I have to artificially
increment PETSc refcounts, call Python method destroy(), then decref,
etc.  And now, I have to do the same refcounting hacking for Reset().
I had the same issue with MatPreallocated() hack at MatDestroy(), that
I could fortunately fix in a recent commit. Add to all this that I
already had a lot of trouble to figure out how to trick Python's
garbage collector to not cause segfaults when reference cycles are
created at the Python level.

I'll say it again: object destruction is delicate, really delicate,
and even more delicated when you add Python to the equation. And this
stuff is not so easy to explain, though I tried. Perhaps you do not
agree how much delicate is because PETSc just do not care about error
recovery.

>
> Just because it makes your life a little easier without context why the change is needed isn't a reason to change it.
>

However, I think you added these calls to XXXReset() just to make YOUR
life a little easier, saving code duplication :-). Additionally, don't
you agree that Reset() and Destroy() have different intentions? Even
if Python issues were not on the table, why it is good to mix these
calls? Could you elaborate?

Finally, grant me a comment: How many times I asked PETSc to change
things just to make my life easier? I try hard to core PETSc ignorant
about the existence of Python wrappers. I even added Python support
using dynamic loading in order to remove any compile/link time
dependecy on Python!!  IOW, when I ask to change something because of
Python issues, that is because I'm really needing them.

Anyway, I have things mostly working on Python. If you decide this
thing is not going to change, I'l have to live with that. But please,
in the future try to not add any other call that dispatching
ops->something() during the execution of XXXDestroy().

>
>> In some previous cleanup
>> commits I took care of preparing the code to make this change, so I
>> expect this one to be easy.
>>
>> BTW, I think we should review the way we handle referrence counting
>> regarding DM's and the global&local vector cache. I had to spend some
>> time to figure out that dm->ops->get[global/local]vector() should
>> forcibly compose the DM in the vector, if not, bad things occur at
>> DMDestroy().
>
>   Did you need to change something recently with this? It was working fine so I'd like to know what was changed and why.
>

Yes. Some time ago Matt added the IGA stuff from Nathan Collier at
KAUST. Unfortunately, at the time that happened, I could not review
the status of the code being pushed. DMIGA is a kind of wrapper around
a DMDA, for the case of implementing get/global/local/vector*() all
you need to ask a DMDA for the vector. BUT if you forget to compose
the DMIGA in the new Vec, things get broken. Matt did not noticed this
issue when he pushed, and it tooks me some time to figure out what's
going on. IMHO, when that things happens (the implementor didn't
noticed, the committer didn't noticed, and a 7-years experience
user/developer like me have trouble to figure out), then there is
something that smells bad.

And yes, this was working fine, but perhaps just for accident. Any DM
subtypes implementing GetGlobal/LocalVector should be reviewed for
this issue. Unfortunately, I didn't have the time when I fixed DMIGA.

>> Cyclic references do not play well in our refcounted
>> model, and we cannot implement a garbage collector that works in
>> parallel :-(
>
>   yes, we all understand this.
>

However, we have been using PetscObjectRegisterDestroyAll() for ages
to collect cached garbage, and such feature is really dangerous if you
ever register an object created on a subcommunicator. You see, we all
think that handling destruction is a piece of cake, but that's not the
case. I figured out this issue just a few hours ago.

-- 
Lisandro Dalcin
---------------
CIMEC (INTEC/CONICET-UNL)
Predio CONICET-Santa Fe
Colectora RN 168 Km 472, Paraje El Pozo
3000 Santa Fe, Argentina
Tel: +54-342-4511594 (ext 1011)
Tel/Fax: +54-342-4511169