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

[Dave May]

On Mon, 4 Mar 2019 at 11:54, Dave May <dave.mayhem23 at gmail.com> wrote:

> Hi Barry,
>
>
> On Mon, 4 Mar 2019 at 03:26, Smith, Barry F. <bsmith at 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?
>
>
> 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);
> }
>

Damn, this function is not completely safe in any case.
It will perform an illegal read if sizeof(user_context) < sizeof(classid)



>
> [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 at 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
>> >
>> >
>> >
>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.mcs.anl.gov/pipermail/petsc-dev/attachments/20190304/f093ca9f/attachment.html>