On Mon, 23 Mar 2020 at 17:52, hzh...@mcs.anl.gov <hzh...@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/