<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, 23 Mar 2020 at 17:52, <a href="mailto:hzhang@mcs.anl.gov">hzhang@mcs.anl.gov</a> <<a href="mailto:hzhang@mcs.anl.gov">hzhang@mcs.anl.gov</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr">Lisandro:<br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div>* 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.<br></div></div></blockquote><div>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. </div></div></div></blockquote><div><br></div><div>OK, lets see... Suppose you do</div><div><br></div><div>MatCreate(&A);</div><div>// put stuf in A;</div><div><div><br></div><div>MatCreate(&B);</div><div>// put stuf in B;</div><div><br></div><div>MatProductCreate(A,B,NULL,&D);<br></div><div><br></div><div><div>MatDestory(&A); // A is gone </div><div>MatDestory(&B); // B is gone <br></div><div>MatProductSymbolic(D); // ->> SEGFAULT, internal A and B stored in D are dangling pointers to deallocated stuff!<br></div><div><br></div><div>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. </div><div>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.<br></div><div><br></div><div>BTW, the product context should require some handling at MatDestroy(&D) time; a call to MatProductReset() should be enough. </div><div><br></div><div><br></div><div></div></div><div></div></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div><br></div><div>* 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. <br></div></div></blockquote><div>Again, giving an example would help me understand. If you just want the  symbolic product, you can call</div><div>MatProductCreate()</div><div>MatProductSetType()</div><div>MatProductSetFromOptions()</div><div>MatProductSymbolic().</div><div>This is equivalent to previous MatMatMultSymbolic(), and is used in some routines of PETSc.</div></div></div></blockquote><div><br></div><div>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:</div><div><br></div><div><div>MatCreate(&A);</div><div>// put stuf in A;</div><div><div><br></div><div>MatCreate(&B);</div><div>// put stuf in B;</div><div><br></div><div>MatProductCreate(A,B,NULL,&D);<br></div><div><br></div><div><div>MatDestory(&A); // I'll not use A any more, so destroy</div><div>MatDestory(&B); // I'll not use B any more, so destroy<br></div><div>MatProductSymbolic(D);<br></div><div><div><br></div><div></div></div><div>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.</div><div>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.</div><div></div></div></div></div><div><br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div></div><div><br></div><div>* 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. </div></div></blockquote><div> MatMatMultSymbolic/Numeric() are not recommended for users, and few developers ever used them.</div></div></div></blockquote><div><br></div><div>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.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div>  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. </div><div><br></div></div></div></blockquote><div><br></div><div>OK, you have a good point here, the effort is not worth it. Forget about this one.</div><div><br></div></div>-- <br><div dir="ltr" class="gmail_signature"><div dir="ltr"><div>Lisandro Dalcin<br>============<br>Research Scientist<br>Extreme Computing Research Center (ECRC)<br>King Abdullah University of Science and Technology (KAUST)<br><a href="http://ecrc.kaust.edu.sa/" target="_blank">http://ecrc.kaust.edu.sa/</a><br></div></div></div></div>