Re: [petsc-dev] Why do we use void* instead of PetscObject in PETSc?

[Smith, Barry F. via petsc-dev]

> On Mar 4, 2019, at 5:54 AM, Dave May <dave.mayhe...@gmail.com> wrote:
> 
> Hi Barry,
> 
> 
> On Mon, 4 Mar 2019 at 03:26, Smith, Barry F. <bsm...@mcs.anl.gov> wrote:
> 
>    There is a lot of merit for this alternative approach. 
> 
>    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. 
> 
> Are you referring to this issue?

   No. I am not sure the suggestion I was referring to has popped up in any 
textual form yet.

   Barry

> 
> https://bitbucket.org/petsc/petsc/issues/245/developer-api-for-replacing-petsc-objects
> 
> I didn't find anything else but probably I looked in the wrong place.
> 
>  
> 
>     There will be a bit of an issue with dealing with the Fortran code but 
> presumably it will be manageable.
> 
>     Barry
> 
>     So my question is, what would be the major win to devoting the resources 
> to make this change now versus putting it off indefinitely?
> 
> 
> 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.
> 
> 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.
> 
> 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).
> 
> [option 1]
> * Keep void* in all the PETSc APIs
> * All PETSc objects currently storing, managing void* data are changed to 
> internally store a PetscObject.
> 
> 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.
> 
> 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"
> 
> 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:
> 
> typedef enum { PETSCHEADER_NULL_PTR = 0 , PETSCHEADER_INVALID_PTR, 
> PETSCHEADER_OBJECT_FREED, PETSCHEADER_INVALID_TYPE } PetscHeaderErrorType;
> 
> const char *PetscHeaderErrorTypeNames[] = { "null pointer", "invalid 
> pointer", "object already freed", "invalid (unknown) PETSc type" };
> 
> PetscErrorCode PetscHasValidHeader(void *h,PetscBool 
> *valid_header,PetscHeaderErrorType *err)
> {
>   *valid_header = PETSC_TRUE;
>   if (!h) { /* "Null Object: Parameter # %d" */
>     *valid_header = PETSC_FALSE;
>     if (err) *err = PETSCHEADER_NULL_PTR;
>     PetscFunctionReturn(0);
>   }
>   if (!PetscCheckPointer(h,PETSC_OBJECT)) { /* "Invalid Pointer to Object: 
> Parameter # %d" */
>     *valid_header = PETSC_FALSE;
>     if (err) *err = PETSCHEADER_INVALID_PTR;
>     PetscFunctionReturn(0);
>   }
>   if (((PetscObject)(h))->classid == PETSCFREEDHEADER){ /* "Object already 
> free: Parameter # %d" */
>      *valid_header = PETSC_FALSE;
>     if (err) *err = PETSCHEADER_OBJECT_FREED;
>   } else if (((PetscObject)(h))->classid < PETSC_SMALLEST_CLASSID || 
> ((PetscObject)(h))->classid > PETSC_LARGEST_CLASSID) { /* "Invalid type of 
> object: Parameter # %d" */
>     *valid_header = PETSC_FALSE;
>     if (err) *err = PETSCHEADER_INVALID_TYPE;
>   }
>   PetscFunctionReturn(0);
> }
> 
> [option 2]
> * Keep void* in all the PETSc APIs
> * Introduce the method above into PETSc.
> * 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. 
> 
> 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"
> 
> 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().
> 
> 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.
> 
> 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.
> 
> 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.
> 
> 
> 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.
> 
> What do you think Barry?
> 
> 
> Cheers
> Dave
> 
>  
> 
> 
> > On Mar 2, 2019, at 10:19 AM, Dave May via petsc-dev <petsc-dev@mcs.anl.gov> 
> > wrote:
> > 
> > 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*.
> > 
> > Some obvious benefits would be:
> > [1] Type checking!
> > [2] User contexts could be readily shared using PetscObjects reference 
> > counter (PetscObjectReferenece()).
> > [3] User contexts would have a communicator.
> > [4] User contexts could have textual names associated with them 
> > (PetscObjectSetName()).
> > [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).
> > 
> > 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.
> > 
> > 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?
> > 
> > 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?). 
> > 
> > 
> > Thanks
> > Dave
> > 
> > 
> > 
>