[lucy-dev] XSubs for overridden methods

[Nick Wellnhofer <we...@aevum.de>]

Lucifers,

CFCPerlClass_method_bindings in CFCPerlClass.c currently generates XSubs for 
overridden methods. The comment states:

     /* Create the binding, add it to the array.
      *
      * Also create an XSub binding for each override.  Each of these
      * directly calls the implementing function, rather than invokes the
      * method on the object using vtable method dispatch.  Doing things
      * this way allows SUPER:: invocations from Perl-space to work
      * properly.
      */

While this is true, I fail to see a practical use case. IMO, the only place 
where calling a supermethod makes sense is in the method implementation of a 
subclass. Everywhere else the result is mostly unpredictable. Even if you know 
exactly where methods are overridden, this could change in a later version. 
Apart from that, why would you ever want to invoke a super method?

In a method of a Perl subclass, a SUPER:: invocation (of the same method) 
would still work if we only created a single XSub with virtual dispatch. So 
what about removing XSubs for overridden methods?

Nick

Re: [lucy-dev] XSubs for overridden methods

[Nick Wellnhofer <we...@aevum.de>]

> That happens because the method pointer stored in the vtable is a callback
> which calls the XSUB which calls the callback which calls the XSUB which calls
> the callback…

Thanks again for the explanation. I think this is third time you explained this to me, but I keep forgetting about it. I’ll add a more detailed explanation to the code comment.

> PS: While researching this post and preparing an example using SUPER::DESTROY,
>    I discovered that the implementation for DESTROY is a little wacky right
>    now.  When a Perl subclass implements DESTROY, the method pointer for
>    Destroy in the subclass vtable doesn't get overridden (possibly because
>    "destroy" doesn't match "DESTROY" after
>    https://github.com/apache/lucy-clownfish/commit/64cf00e28a2 ) but stuff
>    still works because under the Perl bindings cfish_dec_refcount calls
>    SvREFCNT_dec if there's a cached host object.

The commit is what made overriding aliases methods from Perl work. But if you were testing with the latest code from the master branch, I just removed the alias for `Destroy` and added a custom `DESTROY` XSUB to fix the global destruction issue:

    https://github.com/apache/lucy-clownfish/commit/4aea9977c3

It’s tricky to make overriding custom XSUBs from Perl work. The custom implementation would have to use static dispatch and be implemented for every subclass which overrides the method because of the issue mentioned above. This isn't really needed for Destroy but it might be useful in other cases (also for default interface methods).

The key idea is to reuse the XSUB of a novel method for all overridden methods, but without dynamic dispatch. If you have a look at how the ALIAS and INTERFACE keywords work in XS, you’ll see that there’s a slot in CV that can be used to store additional information for XSUBs. We could use it to store a pointer to the subclass:

    // Single XSUB for a method (novel in MyClass)
    XS_INTERNAL(XS_MyClass_method) {
        ...
        // Get (sub)class with CvXSUBANY.
        klass = (Class*)CvXSUBANY(cv).any_ptr;
        // Static dispatch.
        method = CFISH_METHOD_PTR(klass, MyClass_Method);
        ...
    }

    // Register XSUB for novel methods.
    CV *cv = newXS(“MyClass::method", XS_MyClass_method, filename);
    CvXSUBANY(cv).any_ptr = MYCLASS;

    // Reuse XSUB for overridden method.
    CV *cv = newXS(“MySubclass::method", XS_MyClass_method, filename);
    CvXSUBANY(cv).any_ptr = MYSUBCLASS;

    // Or, for methods from a different parcel.
    CV *parent_cv = get_cv(“MyClass::method”, 0);
    CV *cv = newXS(“MySubclass::method", CvXSUB(parent_cv), filename);
    CvXSUBANY(cv).any_ptr = MYSUBCLASS;

Then we’ll only need a way to tell CFC that a there’s a custom XSUB for a method (which can also have an alias) and to suppress the autogenerated XSUB. Custom XSUBs have to use the same static dispatch mechanism, of course. 

Nick

Re: [lucy-dev] XSubs for overridden methods

[Marvin Humphrey <ma...@rectangular.com>]

On Thu, Feb 23, 2017 at 9:57 AM, Nick Wellnhofer <we...@aevum.de> wrote:

> CFCPerlClass_method_bindings in CFCPerlClass.c currently generates XSubs for
> overridden methods. The comment states:
>
>     /* Create the binding, add it to the array.
>      *
>      * Also create an XSub binding for each override.  Each of these
>      * directly calls the implementing function, rather than invokes the
>      * method on the object using vtable method dispatch.  Doing things
>      * this way allows SUPER:: invocations from Perl-space to work
>      * properly.
>      */
>
> While this is true, I fail to see a practical use case. IMO, the only place
> where calling a supermethod makes sense is in the method implementation of a
> subclass.

Calling the supermethod from the method implementation of a subclass is
exactly the intended use case.

There's a test in t/binding/019-obj.t to guarantee that it works.

    package SonOfTestObj;
    use base qw( TestObj );
    {
        sub to_string {
            my $self = shift;
            return "STRING: " . $self->SUPER::to_string;
        }
    }

    ...

    $object = SonOfTestObj->new;
    like( $object->to_string, qr/STRING:.*?SonOfTestObj/,
        "overridden XS bindings can be called via SUPER" );

If I make a change like the following in the XS binding, switching from a
hard-coded function to a vtable method invocation...

-    method = CFISH_METHOD_PTR(CFISH_OBJ, CFISH_Obj_To_String);
-    retval = method(arg_self);
+    retval = CFISH_Obj_To_String(arg_self);

... the test will segfault after Perl tells us "Deep recursion on subroutine
"SonOfTestObj::to_string" at t/binding/019-obj.t line 32."

That happens because the method pointer stored in the vtable is a callback
which calls the XSUB which calls the callback which calls the XSUB which calls
the callback...

PS: While researching this post and preparing an example using SUPER::DESTROY,
    I discovered that the implementation for DESTROY is a little wacky right
    now.  When a Perl subclass implements DESTROY, the method pointer for
    Destroy in the subclass vtable doesn't get overridden (possibly because
    "destroy" doesn't match "DESTROY" after
    https://github.com/apache/lucy-clownfish/commit/64cf00e28a2 ) but stuff
    still works because under the Perl bindings cfish_dec_refcount calls
    SvREFCNT_dec if there's a cached host object.

Marvin Humphrey