<div dir="ltr"><div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, 4 Mar 2019 at 11:54, Dave May <<a href="mailto:dave.mayhem23@gmail.com">dave.mayhem23@gmail.com</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"><div dir="ltr"><div dir="ltr">Hi Barry,<div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, 4 Mar 2019 at 03:26, Smith, Barry F. <<a href="mailto:bsmith@mcs.anl.gov" target="_blank">bsmith@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"><br>
   There is a lot of merit for this alternative approach. <br>
<br>
   This suggestion is also closely related to the (I think it's Toby's) proposal to replace the (function), ctx pairs  that manage call backs in PETSc with objects. <br></blockquote><div><br></div><div><div>Are you referring to this issue?</div><div><br></div><div><a href="https://bitbucket.org/petsc/petsc/issues/245/developer-api-for-replacing-petsc-objects" target="_blank">https://bitbucket.org/petsc/petsc/issues/245/developer-api-for-replacing-petsc-objects</a><br></div><div><br></div><div>I didn't find anything else but probably I looked in the wrong place.</div></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">
<br>
    There will be a bit of an issue with dealing with the Fortran code but presumably it will be manageable.<br>
<br>
    Barry<br>
<br>
    So my question is, what would be the major win to devoting the resources to make this change now versus putting it off indefinitely?<br>
<br></blockquote><div><br></div><div><div>For "basic" usage of SNES, or KSP with KSPSetComputeOperators() there is no obvious win I can see. Hence my proposal is disruptive for (probably) a large fraction of PETSc users.</div><div><br></div><div>However for "advanced" usage of KSP, SNES-DM, specifically FAS or anything involving DMCoarsenHookAdd() I think there is a benefit due to the improved type safety and debugability of the resulting solver. I asked myself the question I posted after having to think about how to safely define a user contexts on KSPs, DMs living on sub-communicators with the PCTelescope setup. Determining if a user context is valid for use on a sub-communicator obviously complicated by the fact we cannot extract / query a void* user context for a communicator.</div><div><br></div><div>Given that the percentage of basic usage mentioned above likely represents the largest fraction of petsc users, my proposal seems unnecessarily disruptive given the little gain it would likely yield to a typical end user. Hence I have two alternative proposals which are less invasive (for the user).</div><div><br></div><div>[option 1]</div><div>* Keep void* in all the PETSc APIs</div><div>* All PETSc objects currently storing, managing void* data are changed to internally store a PetscObject.</div><div><br></div><div>If the user context provided is not a valid PETSc object, we create a PetscContainer, shove in the user context and set the internal PetscObject to point to the container. This does make a call back in which the user data is returned somewhat clunky as you have to first query if the internal PetscObject was created internally, and if so check it is of type PetscContainer and then return the pointer (to allow for a user context which is itself a PetscContainer). Yuck.</div><div><br></div><div>Option 1 asserts the following: "the user does not have to concern themselves with any object model, but internal to the PETSc library we refuse to use void* for user contexts as they are inherently unsafe and hard to debug"</div><div><br></div><div>This would require a suitable method to query pointers for a consistent PETSc header (a function variant of PetscValidHeader() which does not throw errors) and PETSc objects. Below is one such method:</div><div><br></div><div><div>typedef enum { PETSCHEADER_NULL_PTR = 0 , PETSCHEADER_INVALID_PTR, PETSCHEADER_OBJECT_FREED, PETSCHEADER_INVALID_TYPE } PetscHeaderErrorType;</div><div><br></div><div>const char *PetscHeaderErrorTypeNames[] = { "null pointer", "invalid pointer", "object already freed", "invalid (unknown) PETSc type" };</div><div><br></div><div>PetscErrorCode PetscHasValidHeader(void *h,PetscBool *valid_header,PetscHeaderErrorType *err)</div><div>{</div><div>  *valid_header = PETSC_TRUE;</div><div>  if (!h) { /* "Null Object: Parameter # %d" */</div><div>    *valid_header = PETSC_FALSE;</div><div>    if (err) *err = PETSCHEADER_NULL_PTR;</div><div>    PetscFunctionReturn(0);</div><div>  }</div><div>  if (!PetscCheckPointer(h,PETSC_OBJECT)) { /* "Invalid Pointer to Object: Parameter # %d" */</div><div>    *valid_header = PETSC_FALSE;</div><div>    if (err) *err = PETSCHEADER_INVALID_PTR;</div><div>    PetscFunctionReturn(0);</div><div>  }</div><div>  if (((PetscObject)(h))->classid == PETSCFREEDHEADER){ /* "Object already free: Parameter # %d" */</div><div>     *valid_header = PETSC_FALSE;</div><div>    if (err) *err = PETSCHEADER_OBJECT_FREED;</div><div>  } else if (((PetscObject)(h))->classid < PETSC_SMALLEST_CLASSID || ((PetscObject)(h))->classid > PETSC_LARGEST_CLASSID) { /* "Invalid type of object: Parameter # %d" */</div><div>    *valid_header = PETSC_FALSE;</div><div>    if (err) *err = PETSCHEADER_INVALID_TYPE;</div><div>  }</div><div>  PetscFunctionReturn(0);</div><div>}</div></div></div></div></div></div></div></blockquote><div><br></div><div>Damn, this function is not completely safe in any case.</div><div>It will perform an illegal read if sizeof(user_context) < sizeof(classid)</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 dir="ltr"><div dir="ltr"><div class="gmail_quote"><div><div><br></div><div>[option 2]</div><div>* Keep void* in all the PETSc APIs</div><div>* Introduce the method above into PETSc.</div><div>* Do not change / require / enforce that any PETSc objects managing void* data change their ways to use PetscObject. Rather only those objects / implementations / methods which mess around with void* contexts, e.g. those expecting specific types, or require a valid communicator would use the method above (PetscHasValidHeader()) and take action accordingly. </div><div><br></div><div>Option 2 asserts the following: "the user does not have to concern themselves with any object model, but within specific / select PETSc objects we refuse (or cannot) accept void* for users contexts as they are inherently unsafe for usage for a specific implementation"</div><div><br></div><div>Hence "advanced" usage of some funky implementations which manages and manipulations user contexts may require the user context be of type PetscObject (or something derived from it), however such a context would still be cast to void* when calling things like SNESetComputeJacobian(). Then within funky implementation X, appropriate checking of the user context would be conducted using PetscHasValidHeader().</div><div><br></div><div>As an example KSPComputeOperators_SNES() currently does not call PetscValidHeaderSpecific(snes,SNES_CLASSID,4) (I think it should). alternatively it could call PetscHasValidHeader() on the user context. If true was returned, it could be additionally queried to see if it is a SNES object. In this example just using PetscValidHeaderSpecific() would have been sufficient.</div><div><br></div><div>In PCTelescope I could PetscHasValidHeader() to determine if the context is a PetscObject, and if true was returned, I could get it's communicator and check that it matches the sub-communicator Telescope will use internally. If the communicators don't match, I would have a clean way to reject the user context and throw an error. Currently I have to use heuristics to guess what the context should be and these do not involve communicators hence even if my heuristic pass, code may still hang if the user does the wrong thing.</div><div><br></div><div>There are likely other use-cases within preconditioned SNES and FAS (Matt? Jed?) and possibly within the adjoint and sensitivity support of TS where PetscValidHeaderSpecific() is not sufficient and code fragility could be reduced with improved type checking.</div><div><br></div><div><br></div><div>At the very least, option 2 only affects those developers who have faced the problem arising from manipulating a user defined void* context. Hence introducing PetscHasValidHeader() and using it within select implementations seems like a good (non invasive) way to proceed. If PetscHasValidHeader() was found useful, then maybe something like option 1 could be adopted in the long term.</div><div><br></div><div>What do you think Barry?</div><div><br></div><div><br></div><div>Cheers</div></div><div>Dave</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">
<br>
<br>
> On Mar 2, 2019, at 10:19 AM, Dave May via petsc-dev <<a href="mailto:petsc-dev@mcs.anl.gov" target="_blank">petsc-dev@mcs.anl.gov</a>> wrote:<br>
> <br>
> I think there would be a lot of merit (in the long run) if user contexts, such as given to KSPSetComputeOperators(), SNESSetJacobian() etc were changed to be of type PetscObject rather than void*.<br>
> <br>
> Some obvious benefits would be:<br>
> [1] Type checking!<br>
> [2] User contexts could be readily shared using PetscObjects reference counter (PetscObjectReferenece()).<br>
> [3] User contexts would have a communicator.<br>
> [4] User contexts could have textual names associated with them (PetscObjectSetName()).<br>
> [5] Internal to PETSc (e.g. within KSPComputeOperators_SNES) the header of the context could be checked to ensure the type of the PetscObject is valid (SNES in this example).<br>
> <br>
> If this change was adopted, the user could readily change their code to use a PetscContainer. Such a change avoids the user having to recreate their own object model / class to deal with simple issues associated with sharing a context, or having to manage communicators within their own contexts.<br>
> <br>
> I see the point that the context is "the users problem". Many PETSc examples simply use a pointer to typedef'd struct as the context. Users tend to copy examples, and really, a simple struct with a bunch of parameters is not a good object model to be followed. Would it be so horrible to require users to put "their problem" (e.g. context) inside a PetscObject?<br>
> <br>
> Besides the huge volume of work required to change internal PETSc code and more over, the code of all users, are there genuinely compelling reasons to keep using void* everywhere? I cannot think of one, but I would like to hear others thoughts on this. (Maybe there are concerns with Fortran compatibility?). <br>
> <br>
> <br>
> Thanks<br>
> Dave<br>
> <br>
> <br>
> <br>
<br>
</blockquote></div></div></div></div>
</blockquote></div></div></div>