Re: [petsc-dev] Why do we use void* instead of PetscObject in PETSc?
Dave May writes:
> On Mon, 4 Mar 2019 at 21:30, Jed Brown wrote:
>
>> If reusing, would the interface require
>> casting the function pointers to void(*)(void) instead of what is
>> currently type-safe for most function pointers?
>
>
> No. I don't see that casting a function pointer would be required.
> We would have
>
> SNESSetJacobian(SNES snes,Mat Amat,Mat Pmat,PetscErrorCode
> (*J)(SNES,Vec,Mat,Mat,PetscObject),PetscObject ctx)
Oh, the PetscObject would only contain the context, not the function
pointer (at least not publicly). What does user code look like?
PetscErrorCode f(..., PetscObject ctx) {
User user;
ierr = PetscContainerGetPointer((PetscContainer)ctx,);CHKERRQ(ierr);
...
}
Re: [petsc-dev] Why do we use void* instead of PetscObject in PETSc?
Dave May via petsc-dev writes: > 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! How is passing a void* to PetscObject/PetscContainer different from passing it to SNES? Also, would you have different types for every callback or would you reuse? If reusing, would the interface require casting the function pointers to void(*)(void) instead of what is currently type-safe for most function pointers? This feels like less rather than more type safety. Agree on other points.
Re: [petsc-dev] Why do we use void* instead of PetscObject in PETSc?
I've been working on a nonlinear map object: it would wrap
up the function/derivative/second-derivative callbacks
for SNES/Tao/maybe TS in an object.
It doesn't do what Dave is asking: at the moment, the user still
passes a (void *) as ctx to that object. Rather, I'm interested in
use cases where there is more structure to the nonlinear map than is
expressed in the callbacks (such as for operator split methods).
Toby
On Mon, Mar 04, 2019 at 08:55:42PM +, Smith, Barry F. via petsc-dev wrote:
>
>
> > On Mar 4, 2019, at 5:54 AM, Dave May wrote:
> >
> > Hi Barry,
> >
> >
> > On Mon, 4 Mar 2019 at 03:26, Smith, Barry F. 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 >
Re: [petsc-dev] Why do we use void* instead of PetscObject in PETSc?
> On Mar 4, 2019, at 5:54 AM, Dave May wrote:
>
> Hi Barry,
>
>
> On Mon, 4 Mar 2019 at 03:26, Smith, Barry F. 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
Re: [petsc-dev] Why do we use void* instead of PetscObject in PETSc?
Something related are functions like https://www.mcs.anl.gov/petsc/petsc-current/docs/manualpages/SNES/SNESSetConvergenceTest.html https://www.mcs.anl.gov/petsc/petsc-current/docs/manualpages/KSP/KSPMonitorSet.html which even have 3 parameters - the "evaluation" function, the context, and destroying function. I have never liked this design. I think they are pretending to be "simple" for users instead of "bothering" them with having to implement a type of SNESConvergenceTest/KSPMonitor (imaginary classes at the moment). But at the end one spends much more time by finding a bug such as passing ctx instead of or something like that. And the API is uglier, error-prone, harder to remember, etc. It splits e.g. the notion of "convergence test" into 3 complicated arguments. Compare SNESSetConvergenceTest(SNES snes,PetscErrorCode (*SNESConvergenceTestFunction)(SNES,PetscInt,PetscReal,PetscReal,PetscReal,SNESConvergedReason*,void*),void *cctx,PetscErrorCode (*destroy)(void*)) vs SNESSetConvergenceTest(SNES snes, SNESConvergenceTest test) I think if E in PETSc means extensible, we should encourage users to create their own types or classes. If that is documented and there are some examples, people will just copy-paste and alter them as before but will have less problems and nicer code eventually. And they would be better prepared to implement e.g. a whole new PC. Thanks Vaclav > On 2 Mar 2019, at 17:19, Dave May via petsc-dev 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 > > >
Re: [petsc-dev] Why do we use void* instead of PetscObject in PETSc?
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. 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? > On Mar 2, 2019, at 10:19 AM, Dave May via petsc-dev > 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 > > >
