[petsc-dev] Suggestions for MatProductCreate()

[Lisandro Dalcin]

On Mon, 23 Mar 2020 at 17:52, hzhang at mcs.anl.gov <hzhang at mcs.anl.gov> wrote:

> Lisandro:
>
>> * Please consider fixing MatProductCreate(A,B,C,&D) to take ownership
>> (that is, increase reference count) of the A,B, and the (optional) C
>> matrices provided as arguments. Otherwise it is way easy to get into the
>> dangling pointer trap.
>>
> Can you give me a simple example of " get into the dangling pointer trap"?
> We do not use reference count to keep track of A, B for Mat-Mat operations
> in the current and previous versions.
>

OK, lets see... Suppose you do

MatCreate(&A);
// put stuf in A;

MatCreate(&B);
// put stuf in B;

MatProductCreate(A,B,NULL,&D);

MatDestory(&A); // A is gone
MatDestory(&B); // B is gone
MatProductSymbolic(D); // ->> SEGFAULT, internal A and B stored in D are
dangling pointers to deallocated stuff!

The code above will segfault, the interal references to A and B are
destroyed, MatProductSymbolic(C) will use dangling pointers, that
is, pointers to already freed objects. The whole point of having reference
counting in PETSc is to prevent these issues.
Also note that this pattern is used almost everywhere in PETSc (if not
used, I would claim it is a bug). Look for example at PCSetOperators(), the
PC object takes ownership (which means it increments the refcount) of the
input matrices Amat,Pmat.

BTW, the product context should require some handling at MatDestroy(&D)
time; a call to MatProductReset() should be enough.



>> * A thing also missing in the new API is a way to "cleanup" the A,B,C
>> references, something MatProductReset(D) to get rid of (deallocate) the
>> internal "product" context, thus removing  from D the references to A,B,C.
>> This would be useful if you just want to compute JUST the symbolic product,
>> I'm using that in some code to compute the nonzero pattern of A^2.
>>
> Again, giving an example would help me understand. If you just want
> the  symbolic product, you can call
> MatProductCreate()
> MatProductSetType()
> MatProductSetFromOptions()
> MatProductSymbolic().
> This is equivalent to previous MatMatMultSymbolic(), and is used in some
> routines of PETSc.
>

Let me make the case for MatProductReset(). First suppose that you
implement proper ownership and reference counting as discussed above. Now
you construct the symbolic product this way:

MatCreate(&A);
// put stuf in A;

MatCreate(&B);
// put stuf in B;

MatProductCreate(A,B,NULL,&D);

MatDestory(&A); // I'll not use A any more, so destroy
MatDestory(&B); // I'll not use B any more, so destroy
MatProductSymbolic(D);

Now all is good, the code works just fine, but there is a caveat: the D
matrix still keeps a reference to A and B, thus holding quite a bit of
memory that the user does not care about, because he is only interested in
the symbolic product D afterwards.
But there is no API for the user to make D release the A,B resources hold
internally. Thus, you need to add MatProductReset(D) such that power
users/developers can optimize memory consumption, matrices are heavy
objects.



>
>> * It should be also considered to provide backward compatibility
>> PETSC_DEPRECATED calls to the previous MatMatMultSymbolic()
>> and MatMatMultNumeric(). It looks like it would be trivial to do, though I
>> may be getting it wrong because I have not looked at all the details.
>>
>  MatMatMultSymbolic/Numeric() are not recommended for users, and few
> developers ever used them.
>

Well, I was one of these geeks :-) but I admit it was a rather unusual, not
so frequent task, building the sparsity pattern of  A^2 (I remember seen
this somewhere else in PETSc, maybe in PCMG?). Anyway, using your
MatProduct stuff is quite easy, and it is definitely a nice and very
welcome addition/refactor of the previous API.


>   I only see one or two PETSc subroutines call them. I do not think we
> need provide backward compatibility PETSC_DEPRECATED calls for 6 pairs of
> such routines.
>
>
OK, you have a good point here, the effort is not worth it. Forget about
this one.

-- 
Lisandro Dalcin
============
Research Scientist
Extreme Computing Research Center (ECRC)
King Abdullah University of Science and Technology (KAUST)
http://ecrc.kaust.edu.sa/
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.mcs.anl.gov/pipermail/petsc-dev/attachments/20200324/00faa7a7/attachment.html>