<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, 24 Mar 2020 at 05:02, <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>Matt and Lisandro,<br></div><div>Let me explain a little more about new API for mat-mat operations, which include MatMaMult (AB), MatPtAP (PtAP), MatMatMatMult (ABC), with a total 6 matproduct types.</div><div><br></div><div>1) MatProduct is logically similar to MatFactor, not a single Mat. Calling</div><div>MatProductCreate(A,B,NULL,&D);</div><div>MatProductSetType(D, MATPRODUCT_AB);</div><div><div>MatDestory(&A); // A is gone </div><div>MatDestory(&B); // B is gone <br></div><div>MatProductSymbolic(D);</div></div> is equivalent to calling MatMatMult(null, null, ..., &D) or MatGetFactor(null, ...,&F). <div>If you do this, you'll get segfault (I'll add additional error message for this use case).</div><div><br></div></div></blockquote><div><br></div><div>No, it is not really equivalent to MatMatMult(null, null). Because if you pass NULL to MatMatMult you can check for them. However, in the new MatProduct API, if you desdtroy A,B and next call MatProductSymbolic(D) as in my example, the internal pointers to A and B are not NULL, they are "dangling" pointers, holding the same addresses at MatProductCreate() time.</div><div><br></div><div>This is way I said that this API is prone to dangling pointer issues if ownership is not properly handled.</div><div><br></div><div><br></div><div>All I'm asking is you to implement things this way:</div><div><br></div><div>static PetscErrorCode MatProductCreate_Private(Mat A,Mat B,Mat C,Mat D)<br>{<br>  ...<br>  PetscObjectReference(A); // take ownership<br>  PetscObjectReference(B); // take ownership<br>  PetscObjectReference(C); // take ownership<br>  product->A        = A;<br>  product->B        = B;<br>  product->C        = C;<br>}<br><br>PetscErrorCode MatProductReplaceMats(Mat A,Mat B,Mat C,Mat D)<br>{<br>  ...<br>  if (A) {<br>    if (!product->Areplaced) {<br>      PetscObjectReference(A); // take ownership of input first<br>      MatDestroy(&product->A); // release old reference<br>      product->A = A;<br>    } else ...<br>  }<br><br>  ...<br>}<br><br>PetscErrorCode MatDestroy(Mat *A)<br>{<br> ...<br>    if ((*A)->product) {<br>    Mat_Product  *product = (*A)->product;<br>    MatDestroy(&product->A); // release reference<br>    MatDestroy(&product->B); // release reference<br>    MatDestroy(&product->C); // release reference<br>    MatDestroy(&product->Dwork);<br>    PetscFree(product);<br>  }<br>...<br>}<br></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></div><div>2) New API is an incremental improvement over the old API, e.g., </div><div>MatMatMultSymbolic(A,B, fill,&C); MatMatMultNumeric(A,B,C)</div><div>are replaced by</div><div>MatProductCreate(A,B,NULL,&D);</div><div>MatProductSetType(D, MATPRODUCT_AB);<br></div><div> MatProductSetFill(D,fill);<br> MatProductSetFromOptions(D);<br></div><div>MatProductSymbolic(D):<br></div><div>MatProductNumeric(D);<br></div><div><br></div><div>There are at least two reasons we decided to introduce new API.</div><div>a) there are multiple implementations (algorithms) for some product types and matrix types, e.g., A*B  and PtAP for mpiaij_mpiaij matrices. Developers are adding more and more special implementations, e.g, for gpu etc. Previously, the options of algorithms are selected in MatMatMultSymbolic_xxx(...,&D) with random option names. However, some options need to be selected before D is setup, and attached to D. For this reason, we introduced new API, and add MatProductSetFromOptions(D) to handle all the options. Note: the options can only be determined at the level of producttype and mattype are known, thus the options are enabled in MatProductSetFromOptions_mattype_producttype().</div></div></blockquote><div><br></div><div>As I said in previous emails, I'm not complaining about the API, I said it was a welcome adition. All I'm asking is:</div><div><br></div><div>1)  Fix the handling of reference counting. This is an implementation detail, not really an API change.</div><div><br></div><div>2) Add a simple new routine to the API, MatProductReset(D)  [or maybe name it MatProductClear(D) ], that should throw away all of the D->product stuff to release memory related to the product algorithm implementation. After calling MatProductReset(D), you cannot call anymore MatProductNumeric().</div><div><br></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>b) Reuse of MatProduct D = A*B means same A and B with updated values. However, some users change A or B, some use different  D without knowing original D might contain internal data structure that is associated with A and B. The new API forces reuse same A, B, unless user calls MatProductReplaceMats(A,B,C,D).</div></div></blockquote><div><br></div><div>All that is good, no objections. But please consider adding MatProductReset(D) with the behavior I described above. </div><div>Of course, this is only needed if you implement the changes to reference counting that I requested at the beginning.</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><br></div><div>3) For MatProduct, I consider</div><div>MatProductSetFromOptions() is logically equivalent to MatSetFromOptions(), <br></div><div>MatProductSymbolic() is logically equivalent to MatSetUp(),</div><div>and</div><div>MatProductNumeric() assembles the matproduct  D repeatedly when A or B are updated.</div><div><br></div></div></blockquote><div><br></div><div>OK, so... your point it that what looks like "setup" code in MatProductSetFromOptions() is just some minimal stuff required to actually dispatch the SetFromOptions_typeA_typeB routine, right? In that case, you are right, no changes/refactors needed here.</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>