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

[Dave May]

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);
}

[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/f460c9fb/attachment-0001.html>