So I have been working thought the code so I could determine the required vfuncs and the overall flow of the classes. For now I have only split up the current cache and invoke mechanism and Callable only does the minimum.

Is there a specific way that you would want this done before I start working on this? My current formulation uses GObject classes, however I've noticed that there aren't any in the pygobject code.

The following is a class diagram for the various cache classes and what the various methods and vfuncs do.

PyGICallableCache : GInitable {
    new(callable_info) -> sets callable_info
    bool init() -> runs generate_args_cache()

    bool generate_args_cache(arg_index) ->
        generates the rest of the args cache (+ return value)
}

PyGIFunctionCache : PyGICallableCache {
    constructed() -> setup invoker

    new(callable_info) ->
        create correct class instance (except
        PyGICallbackCache, has specialized new())

    PyObject *invoke(State, py_args, py_kwargs) ->
        marshal in args
        call State.function_ptr
        handle error
        extract return value
        marshal out args
        cleanup args from python
        cleanup args to python

    PyObject *invoke(py_args, py_kwargs) ->
        create State
        call invoke() vfunc with State
        cleanup State
}

PyGICCallbackCache : PyGIFunctionCache {
    override new(..., function_ptr) -> set State.function_ptr
    override invoke(..., user_data) -> set State.user_data
}

PyGIConstructorCache : PyGIFunctionCache {
    override invoke() -> remove class arg
}

PyGIFunctionWithInstanceInitCache : PyGIFunctionCache {
    override generate_args_cache() -> add instance arg

    override n_args -> defaults to 1 (instead of 0)
}

PyGIMethodCache : PyGIFunctionWithInstanceCache {
}

PyGIVFuncCache : PyGIFunctionWithInstanceCache {
    override invoke() ->
        set State.function_ptr
        remove instance arg
}

(In reply to comment #1)
> So I have been working thought the code so I could determine the required
> vfuncs and the overall flow of the classes. For now I have only split up the
> current cache and invoke mechanism and Callable only does the minimum.

I'm happy someone else is interested in fixing this!

> Is there a specific way that you would want this done before I start working on
> this? My current formulation uses GObject classes, however I've noticed that
> there aren't any in the pygobject code.

To start, I think it would be worthwhile to setup some C unittesting. It seems like it would be useful to test the mechanics of the caches from C with minimal complication. I started a C unittest patch some time ago which ended up not being used, but it could be a starting point at least for the infrastructure:
https://bugzilla.gnome.org/show_bug.cgi?id=693405#c61

I'm not sure we need to use GObject for the classes because I don't think we need its features. The caches are fairly simple and their memory is directly managed by a Python callable object and not shared. I actually think C++ classes would be a better tool for this particular job, but C with manually setup vfuncs/casting also works. Of course, if you show me code that uses GObject that is clearly beneficial/cleaner then I can be swayed.

>     new(callable_info) ->
>         create correct class instance (except
>         PyGICallbackCache, has specialized new())

It is unclear if we will need virtual constructors. Some of the call sites for creating the caches already have concrete knowledge (or could be made so). So they can just directly call things like pygi_callback_cache_new(...). But there is some mess WRT functions, constructors, and methods. We basically already know the callable type in _function/callable_info_call and related methods, but it might take some cleanup to make that clearer, basically try to avoid multiple dispatchers based on GICallableInfo:
https://git.gnome.org/browse/pygobject/tree/gi/pygi-info.c?id=3.13.3#n533

I've worked through some of this before but never got around to coming up with something I was happy with. I'll try to dig up those patches this weekend for reference.

> PyGIFunctionWithInstanceInitCache : PyGIFunctionCache {
>     override generate_args_cache() -> add instance arg
> 
>     override n_args -> defaults to 1 (instead of 0)

I wonder if n_args could be a virtual getter which returns (pygi_function_cache_get_n_args_real() + 1) ?

The rest of your design looks good.

(In reply to comment #2)
> (In reply to comment #1)
> > So I have been working thought the code so I could determine the required
> > vfuncs and the overall flow of the classes. For now I have only split up the
> > current cache and invoke mechanism and Callable only does the minimum.
> 
> I'm happy someone else is interested in fixing this!
> 
> > Is there a specific way that you would want this done before I start working on
> > this? My current formulation uses GObject classes, however I've noticed that
> > there aren't any in the pygobject code.
> 
> To start, I think it would be worthwhile to setup some C unittesting. It seems
> like it would be useful to test the mechanics of the caches from C with minimal
> complication. I started a C unittest patch some time ago which ended up not
> being used, but it could be a starting point at least for the infrastructure:
> https://bugzilla.gnome.org/show_bug.cgi?id=693405#c61
> 

I am all for adding unittests and preventing any breaks or other issues. I will look at the unittests that you started and try to get them integrated into the test suite.

> I'm not sure we need to use GObject for the classes because I don't think we
> need its features. The caches are fairly simple and their memory is directly
> managed by a Python callable object and not shared. I actually think C++
> classes would be a better tool for this particular job, but C with manually
> setup vfuncs/casting also works. Of course, if you show me code that uses
> GObject that is clearly beneficial/cleaner then I can be swayed.
> 

Using C++ classes would be much easier and I highly doubt any GObject features are needed to get it working. Although I guess this would then require either moving the code to C++ or writing some simple C wrapper code around the new C++ classes. How would you prefer this be done?

> >     new(callable_info) ->
> >         create correct class instance (except
> >         PyGICallbackCache, has specialized new())
> 
> It is unclear if we will need virtual constructors. Some of the call sites for
> creating the caches already have concrete knowledge (or could be made so). So
> they can just directly call things like pygi_callback_cache_new(...). But there
> is some mess WRT functions, constructors, and methods. We basically already
> know the callable type in _function/callable_info_call and related methods, but
> it might take some cleanup to make that clearer, basically try to avoid
> multiple dispatchers based on GICallableInfo:
> https://git.gnome.org/browse/pygobject/tree/gi/pygi-info.c?id=3.13.3#n533
> 

The various different constructors was based on doing this using GObject classes to avoid having callers need to do some work. But it can easily be moved there. From what I have read and mapped out in the code it should all be fairly easy to break down once the various vfuncs are determined.

> I've worked through some of this before but never got around to coming up with
> something I was happy with. I'll try to dig up those patches this weekend for
> reference.
> 

It would be great to avoid starting over from scratch and see what design you have envisioned.

> > PyGIFunctionWithInstanceInitCache : PyGIFunctionCache {
> >     override generate_args_cache() -> add instance arg
> > 
> >     override n_args -> defaults to 1 (instead of 0)
> 
> I wonder if n_args could be a virtual getter which returns
> (pygi_function_cache_get_n_args_real() + 1) ?
> 
> The rest of your design looks good.

The number of args are needed when creating the arg cache, so in the CallableCache I was simply going to do: n_args += get_n_args(). This way FunctionWithInstance could simply set n_args to 1 early and it would magically work.

Created attachment 281828 [details] [review]
Initial attempt refactoring CallableCache into multiple classes

This was a very early attempt at playing with the ideas here.
Note this is very broken but is being added for historical/reference
reasons.

Review of attachment 281828 [details] [review]:

Comments for Garrett:
The overarching goal of this patch was to add concrete variations of the tp_descr_get (__get__) and tp_call (__call__) to PyGIFunctionInfo_Type and PyGIVFuncInfo_Type. This could be taken further by adding new PyGIConstructorInfo_Type and PyGIMethodInfo_Type which are created by checking flags within _pygi_info_new [1]. I think idea was to dispatch the creation of concrete types as early as possible so it doesn't need to be done dynamically each call (checking GICallableInfo flags if it is a constructor for example). This was such early prototyping it is unclear if the approach is worthwhile, feel free to throw the idea away if it doesn't make sense in practice.

[1] https://git.gnome.org/browse/pygobject/tree/gi/pygi-info.c?id=3.13.3#n417

::: gi/pygi-cache.h
@@ +208,3 @@
+    const gchar *name;
+
+    GICallableInfo *info;

Stashing the info on the cache is a bit ugly and I would like to avoid it. However, it might be necessary at least initially for vfuncs, but we might be able to store the vfunc function pointer in the cache instead.

A background goal is that the cache should be independent of GI (only uses GI info as a starting point). This would allow building caches from other information (non-introspected signals for instance).

@@ +241,3 @@
+    GIFunctionInvoker invoker;
+
+    /* Number of out args passed to g_function_info_invoke.

I think the idea of this was sub-classes can implement it and handle the instance argument pulled from args[0] where needed.

@@ +265,3 @@
+typedef struct PyGIFunctionCache PyGICCCallbackCache;
+
+

These were just temporary typedefs which could be replaced with new types where needed.

(In reply to comment #3)
> (In reply to comment #2)
> > To start, I think it would be worthwhile to setup some C unittesting. It seems
> > like it would be useful to test the mechanics of the caches from C with minimal
> > complication. I started a C unittest patch some time ago which ended up not
> > being used, but it could be a starting point at least for the infrastructure:
> > https://bugzilla.gnome.org/show_bug.cgi?id=693405#c61
> > 
> 
> I am all for adding unittests and preventing any breaks or other issues. I will
> look at the unittests that you started and try to get them integrated into the
> test suite.

One approach here would be to build caches manually instead of from GI info, set the function pointer to a C function internal to the test and try calling invoke on it with Python arguments to test the mechanics. But it may also be a bit redundant, we have a lot of Python tests which will crash and burn if anything is wrong in these parts of the code. So I wouldn't sweat it too much unless it makes your life easier in terms of isolating the problems.

> Using C++ classes would be much easier and I highly doubt any GObject features
> are needed to get it working. Although I guess this would then require either
> moving the code to C++ or writing some simple C wrapper code around the new C++
> classes. How would you prefer this be done?

Putting it that way, C++ might not be worth it as could be pretty invasive and has the potential to side track the effort. Basically we end up with the convenience of:


class PyGICallableCache {
public:
    virtual void invoke(...) = 0;
}

class PyGIMethodCache : public PyGICallableCache {
    void invoke (...);
}


vs.

typedef struct {
    void (*invoke) (...);
} PyGICallableCache;

typedef PyGICallableCache PyGIMethodCache;
...
void method_cache_init (PyGIMethodCache *cache) {
    ((PyGICallableCache *)cache)->invoke = method_cache_invoke_real;
}


The latter is uglier to me but the former might require a lot of changes to the code base as a whole.

Created attachment 281964 [details] [review]
Split the Cache v1

It seems that for the most part if the caches aren't setup correctly everything will just "crash and burn." So I've just been going over the testsuite and trying to get everything working.

The attached patch is my work in progress on this, seems everything but functions and method caches work. I've been trying to go through the code and figure out what is wrong but haven't been able to pinpoint it.

Seems the overall issue is that the cache->arg_name_list doesn't have the instance param because it's meta_type is CHILD. But I believe I avoided changing any of that logic.

Any pointers on where to look would be great, thanks!

Created attachment 281977 [details] [review]
Split the Cache v2

This one seems to be working for all of the tests (cairo and other are disabled atm).

Review of attachment 281977 [details] [review]:

Looks like a good start. I have some minor style related comments along with some larger questions. I like how you are setting up the new/free/init/deinit technique, I think it would be nice to follow the pattern more strictly even if it is a bit tedious. So for instance allocator functions should be limited to allocating the memory and calling init which chains up to base class inits. Similarly, dealloc functions are limited to calling deinit which are chains up base class deinits and then deallocs the memory.

::: gi/Makefile.am
@@ -94,3 @@
 	pygi-signal-closure.h \
 	pygobject-external.h \
-	pygi-invoke.c \

Not sure why this was removed.

::: gi/pygi-cache.c
@@ +456,3 @@
+_callable_cache_generate_args_cache_real (PyGICallableCache *callable_cache,
+                                          GICallableInfo *callable_info,
+                                          gssize arg_index)

Slightly unfortunate to have arg_index as an argument. Perhaps this function (and the generate_args_cache vfuncs generally) can be rolled into the various xx_cache_init() functions and use cache->args_offset as the starting index? If this doesn't make sense then we need some commentary explaining why generate_args_cache needs to be a vfunc, some type of ordering problem perhaps?

@@ +688,2 @@
     } else if (type == GI_INFO_TYPE_VFUNC) {
         cache->function_type = PYGI_FUNCTION_TYPE_VFUNC;

Seems like these should be rolled into the sub-class init functions. So vfunc_cache_init() sets function_type then calls callable_cache_init() after it setup vfunc specifics.

@@ +716,3 @@
+
+static PyObject *
+_function_cache_invoke_real (PyGIFunctionCache *function_cache,

Avoid underscore prefixes on new functions: "function_cache_invoke_real" is fine for static and prefix anything with "pygi_" if it goes in a header.

@@ +889,3 @@
+    /* TODO: We don't use the class parameter sent in by  the structure
+     * so we remove it from the py_args tuple but we can keep it
+    function_cache = g_slice_new0 (PyGIFunctionCache);

I don't think the latter part of this comment makes sense anymore, we've been using actual gobject constructors for longer than I've been hacking on this stuff.

@@ +1003,3 @@
+    method_cache = g_slice_new0 (PyGIMethodCache);
+    function_cache = (PyGIFunctionCache *) method_cache;
+        return;

Since these casts are only use once, we probably don't need the variables and can cast where they are used instead.

@@ +1078,3 @@
+
+    /* This will be set in _vfunc_cache_invoke_real() */
+        PyErr_Clear ();

I don't understand why this is being set to pygi_vfunc_cache_new.

@@ +1080,3 @@
+    function_cache->invoker.native_address = pygi_vfunc_cache_new;
+
+        PyErr_Format (PyExc_TypeError,

Slightly more tedious, but I think adding this (and the info setup below) to a vfunc_cache_init() function which chains up function_with_instance_cache_init() would be better.

@@ +1101,3 @@
+    _function_cache_deinit ((PyGIFunctionCache *) vfunc_cache);
+
+    Py_DECREF (py_args);

Similarly, add this to a new vfunc_cache_deinit() function which chains up to function_with_instance_cache_deinit.

::: gi/pygi-cache.h
@@ +167,3 @@
     const gchar *name;
 
+    PyGIDirection direction;

Add a comment noting what direction means in the context of a callable cache. Since it's different than marshaling direction, consider using a new enum, something like:

PyGICallingContext:
    PYGI_IS_CALLED_FROM_C
    PYGI_IS_CALLED_FROM_PYTHON

@@ +272,3 @@
 
+PyGIFunctionCache *
+pygi_function_cache_new (GICallableInfo *info);

Attempt alignment with prior "(" symbols when feasible.

::: gi/pygi-ccallback.c
@@ +38,3 @@
     }
 
+    result = pygi_ccallback_cache_invoke( self->cache,

I realize the style in pygobject is a mess, but I've been slowly trying to change things over to having a space prior to open parens and none after with multi-line arguments left aligned:

    result = pygi_ccallback_cache_invoke (self->cache,
                                          args,

::: gi/pygi-closure.c
@@ +929,2 @@
     if (callable_cache != NULL)
+        child_offset = callable_cache->args_offset;

We might be able to break this out into a separate patch before the main one. Even though it will add assignment code to _args_cache_generate which would be subsequently withdrawn, it makes reviewing and bisecting easier.

::: gi/pygi-info.c
@@ +185,3 @@
+
+            if (flags & GI_FUNCTION_IS_CONSTRUCTOR)
+            GIFunctionInfoFlags flags;

Can this be setup so CallableCache has a "free" virtual method? Then all that would be needed is: self->cache->free (self->cache)

::: gi/pygi-invoke.c
@@ +676,3 @@
+            flags = g_function_info_get_flags ( (GIFunctionInfo *)self->info);
+
+            if (flags & GI_FUNCTION_IS_CONSTRUCTOR)

I prefer 1TBS even for one liners:

if (flags & GI_FUNCTION_IS_CONSTRUCTOR) {
    self->cache = pygi_constructor_cache_new (self->info);
} else if (flags & GI_FUNCTION_IS_METHOD) {
    ...
}

Although I realize this might have been copied from elsewhere.

I'm still working on fixing a few of the other issues and completing the split. Current callable_cache->function_type is still used for marshaling out args although that just seems to be a fast fail if the constructor returns NULL. Can that just be moved to constructor_cache_invoke_real() or do you still want a fast failure. Currently none of the other caches need special marshal_{in,out}() vfuncs.


(In reply to comment #9)
> Review of attachment 281977 [details] [review]:
> 
> Looks like a good start. I have some minor style related comments along with
> some larger questions. I like how you are setting up the new/free/init/deinit
> technique, I think it would be nice to follow the pattern more strictly even if
> it is a bit tedious. So for instance allocator functions should be limited to
> allocating the memory and calling init which chains up to base class inits.
> Similarly, dealloc functions are limited to calling deinit which are chains up
> base class deinits and then deallocs the memory.
> 
> ::: gi/Makefile.am
> @@ -94,3 @@
>      pygi-signal-closure.h \
>      pygobject-external.h \
> -    pygi-invoke.c \
> 
> Not sure why this was removed.
> 

At the moment I am #include'ing it into pygi-cache.c because I need to use the static functions in it. I plan to change them around a bit and make them non-static once I can get the basic split working.

> ::: gi/pygi-cache.c
> @@ +456,3 @@
> +_callable_cache_generate_args_cache_real (PyGICallableCache *callable_cache,
> +                                          GICallableInfo *callable_info,
> +                                          gssize arg_index)
> 
> Slightly unfortunate to have arg_index as an argument. Perhaps this function
> (and the generate_args_cache vfuncs generally) can be rolled into the various
> xx_cache_init() functions and use cache->args_offset as the starting index? If
> this doesn't make sense then we need some commentary explaining why
> generate_args_cache needs to be a vfunc, some type of ordering problem perhaps?
> 

We could remove the arg_index arg and instead set the initial value to callable_cache->args_offset. However, callable_cache->args_cache is created in init() so we would require all caches to include that bit of code, add to the arg cache and then chain up init(). I will be adding comments, this is just an initial implementation to get things working.

> @@ +688,2 @@
>      } else if (type == GI_INFO_TYPE_VFUNC) {
>          cache->function_type = PYGI_FUNCTION_TYPE_VFUNC;
> 
> Seems like these should be rolled into the sub-class init functions. So
> vfunc_cache_init() sets function_type then calls callable_cache_init() after it
> setup vfunc specifics.
> 

This will be removed soon as it is only still there for backwards compatibility and for initial testing.

> @@ +716,3 @@
> +
> +static PyObject *
> +_function_cache_invoke_real (PyGIFunctionCache *function_cache,
> 
> Avoid underscore prefixes on new functions: "function_cache_invoke_real" is
> fine for static and prefix anything with "pygi_" if it goes in a header.
> 

I'm just following the style for the rest of pygi-cache.c and pygi-invoke.c, should I still change this or keep the same coding style as the rest of the files?

> @@ +889,3 @@
> +    /* TODO: We don't use the class parameter sent in by  the structure
> +     * so we remove it from the py_args tuple but we can keep it
> +    function_cache = g_slice_new0 (PyGIFunctionCache);
> 
> I don't think the latter part of this comment makes sense anymore, we've been
> using actual gobject constructors for longer than I've been hacking on this
> stuff.
> 

Removed.

> @@ +1003,3 @@
> +    method_cache = g_slice_new0 (PyGIMethodCache);
> +    function_cache = (PyGIFunctionCache *) method_cache;
> +        return;
> 
> Since these casts are only use once, we probably don't need the variables and
> can cast where they are used instead.
> 

Removed the PyGIFunctionCache cast. Removing the PyGIFunctionWithInstanceCache cast would have a 100+ column line, still remove it?

> @@ +1078,3 @@
> +
> +    /* This will be set in _vfunc_cache_invoke_real() */
> +        PyErr_Clear ();
> 
> I don't understand why this is being set to pygi_vfunc_cache_new.
> 

I will add a comment to it, _function_cache_invoke_real() checks the native_address to create the invoker, if it isn't set to non-NULL then things go wrong. It can be any value, any preference? 0xdeedbeef?

> @@ +1080,3 @@
> +    function_cache->invoker.native_address = pygi_vfunc_cache_new;
> +
> +        PyErr_Format (PyExc_TypeError,
> 
> Slightly more tedious, but I think adding this (and the info setup below) to a
> vfunc_cache_init() function which chains up function_with_instance_cache_init()
> would be better.
> 

That would imply also doing the same for all leaf caches: PyGICCallbackCache, PyGIConstructorCache, PyGIMethodCache and PyGIVFuncCache. Only the leaf caches without any inheritance are missing (de)init().

> @@ +1101,3 @@
> +    _function_cache_deinit ((PyGIFunctionCache *) vfunc_cache);
> +
> +    Py_DECREF (py_args);
> 
> Similarly, add this to a new vfunc_cache_deinit() function which chains up to
> function_with_instance_cache_deinit.
> 

See above.

> ::: gi/pygi-cache.h
> @@ +167,3 @@
>      const gchar *name;
> 
> +    PyGIDirection direction;
> 
> Add a comment noting what direction means in the context of a callable cache.
> Since it's different than marshaling direction, consider using a new enum,
> something like:
> 
> PyGICallingContext:
>     PYGI_IS_CALLED_FROM_C
>     PYGI_IS_CALLED_FROM_PYTHON
> 

Will add the enum.

> @@ +272,3 @@
> 
> +PyGIFunctionCache *
> +pygi_function_cache_new (GICallableInfo *info);
> 
> Attempt alignment with prior "(" symbols when feasible.
> 

Lined up all CallableCache and subclass functions.

> ::: gi/pygi-ccallback.c
> @@ +38,3 @@
>      }
> 
> +    result = pygi_ccallback_cache_invoke( self->cache,
> 
> I realize the style in pygobject is a mess, but I've been slowly trying to
> change things over to having a space prior to open parens and none after with
> multi-line arguments left aligned:
> 
>     result = pygi_ccallback_cache_invoke (self->cache,
>                                           args,
> 

Done, is there an example on how I should be formatting all new code. I've been attempting to follow what is already there but, as you said, it is a mess.

Should I format things according to the glib style format, but with different brace positions and 4 spaces for indentation?

> ::: gi/pygi-closure.c
> @@ +929,2 @@
>      if (callable_cache != NULL)
> +        child_offset = callable_cache->args_offset;
> 
> We might be able to break this out into a separate patch before the main one.
> Even though it will add assignment code to _args_cache_generate which would be
> subsequently withdrawn, it makes reviewing and bisecting easier.
> 

I could, although there are other places that do the same check and benefit from callable_cache->args_offset. 

> ::: gi/pygi-info.c
> @@ +185,3 @@
> +
> +            if (flags & GI_FUNCTION_IS_CONSTRUCTOR)
> +            GIFunctionInfoFlags flags;
> 
> Can this be setup so CallableCache has a "free" virtual method? Then all that
> would be needed is: self->cache->free (self->cache)
> 

Yes I think that would be very helpful and might actually be a good case for having deinit() functions. If I also moved from g_slice_new0() to g_malloc0() then I could have pygi_callable_cache_free() just call the deinit() vfunc and then free the cache with g_free() (need to correct size for g_slice_free()).

> ::: gi/pygi-invoke.c
> @@ +676,3 @@
> +            flags = g_function_info_get_flags ( (GIFunctionInfo *)self->info);
> +
> +            if (flags & GI_FUNCTION_IS_CONSTRUCTOR)
> 
> I prefer 1TBS even for one liners:
> 
> if (flags & GI_FUNCTION_IS_CONSTRUCTOR) {
>     self->cache = pygi_constructor_cache_new (self->info);
> } else if (flags & GI_FUNCTION_IS_METHOD) {
>     ...
> }
> 
> Although I realize this might have been copied from elsewhere.

Fixed.

OK, I have added a deinit() vfunc and then xxx_deinit_real() checks if callable_cache->deinit == xxx_deinit_real and if it does it g_slice_free()s the cache.

This might be better off as just passing a boolean free argument to the deinit, chain up to other with False and free the cache if True.

Created attachment 282096 [details] [review]
Split the Cache v3

So I have now taken another run through the patch, things left:

- Should deinit() have a free arg or continue to do vfunc comparison?

- Should the vfuncs have a typedef? PyGIMarshal{From,To,Cleanup}PyFunc exist for the arg cache marshaling.

- Need to rename or move some fields from PyGICallableCache into PyGIFunctionCache, if nothing else the comments need to be updated.


(Probably forgetting things...)


(In reply to comment #10)
> I'm still working on fixing a few of the other issues and completing the split.
> Current callable_cache->function_type is still used for marshaling out args
> although that just seems to be a fast fail if the constructor returns NULL. Can
> that just be moved to constructor_cache_invoke_real() or do you still want a
> fast failure. Currently none of the other caches need special
> marshal_{in,out}() vfuncs.
> 
> 

Moved to _constructor_cache_invoke_real()

> (In reply to comment #9)
> > Review of attachment 281977 [details] [review] [details]:
> > 
> > Looks like a good start. I have some minor style related comments along with
> > some larger questions. I like how you are setting up the new/free/init/deinit
> > technique, I think it would be nice to follow the pattern more strictly even if
> > it is a bit tedious. So for instance allocator functions should be limited to
> > allocating the memory and calling init which chains up to base class inits.
> > Similarly, dealloc functions are limited to calling deinit which are chains up
> > base class deinits and then deallocs the memory.
> > 
> > ::: gi/Makefile.am
> > @@ -94,3 @@
> >      pygi-signal-closure.h \
> >      pygobject-external.h \
> > -    pygi-invoke.c \
> > 
> > Not sure why this was removed.
> > 
> 
> At the moment I am #include'ing it into pygi-cache.c because I need to use the
> static functions in it. I plan to change them around a bit and make them
> non-static once I can get the basic split working.
> 

Done.

> > ::: gi/pygi-cache.c
> > @@ +456,3 @@
> > +_callable_cache_generate_args_cache_real (PyGICallableCache *callable_cache,
> > +                                          GICallableInfo *callable_info,
> > +                                          gssize arg_index)
> > 
> > Slightly unfortunate to have arg_index as an argument. Perhaps this function
> > (and the generate_args_cache vfuncs generally) can be rolled into the various
> > xx_cache_init() functions and use cache->args_offset as the starting index? If
> > this doesn't make sense then we need some commentary explaining why
> > generate_args_cache needs to be a vfunc, some type of ordering problem perhaps?
> > 
> 
> We could remove the arg_index arg and instead set the initial value to
> callable_cache->args_offset. However, callable_cache->args_cache is created in
> init() so we would require all caches to include that bit of code, add to the
> arg cache and then chain up init(). I will be adding comments, this is just an
> initial implementation to get things working.
> 

Removed arg_index arg and set it initially to callable_cache->args_offset.

> > @@ +688,2 @@
> >      } else if (type == GI_INFO_TYPE_VFUNC) {
> >          cache->function_type = PYGI_FUNCTION_TYPE_VFUNC;
> > 
> > Seems like these should be rolled into the sub-class init functions. So
> > vfunc_cache_init() sets function_type then calls callable_cache_init() after it
> > setup vfunc specifics.
> > 
> 
> This will be removed soon as it is only still there for backwards compatibility
> and for initial testing.
> 

Removed.

> > @@ +1078,3 @@
> > +
> > +    /* This will be set in _vfunc_cache_invoke_real() */
> > +        PyErr_Clear ();
> > 
> > I don't understand why this is being set to pygi_vfunc_cache_new.
> > 
> 
> I will add a comment to it, _function_cache_invoke_real() checks the
> native_address to create the invoker, if it isn't set to non-NULL then things
> go wrong. It can be any value, any preference? 0xdeedbeef?
> 

Added a comment and initially set it to 0xdeadbeef.


> > @@ +1101,3 @@
> > +    _function_cache_deinit ((PyGIFunctionCache *) vfunc_cache);
> > +
> > +    Py_DECREF (py_args);
> > 
> > Similarly, add this to a new vfunc_cache_deinit() function which chains up to
> > function_with_instance_cache_deinit.
> > 
> 
> See above.
> 

Added as a vfunc and called by pygi_callable_cache_free().

> > ::: gi/pygi-cache.h
> > @@ +167,3 @@
> >      const gchar *name;
> > 
> > +    PyGIDirection direction;
> > 
> > Add a comment noting what direction means in the context of a callable cache.
> > Since it's different than marshaling direction, consider using a new enum,
> > something like:
> > 
> > PyGICallingContext:
> >     PYGI_IS_CALLED_FROM_C
> >     PYGI_IS_CALLED_FROM_PYTHON
> > 
> 
> Will add the enum.
> 

Added, but don't really know what I should put for a comments as PyGIDirection's comment seems to be very similar.

> > ::: gi/pygi-info.c
> > @@ +185,3 @@
> > +
> > +            if (flags & GI_FUNCTION_IS_CONSTRUCTOR)
> > +            GIFunctionInfoFlags flags;
> > 
> > Can this be setup so CallableCache has a "free" virtual method? Then all that
> > would be needed is: self->cache->free (self->cache)
> > 
> 
> Yes I think that would be very helpful and might actually be a good case for
> having deinit() functions. If I also moved from g_slice_new0() to g_malloc0()
> then I could have pygi_callable_cache_free() just call the deinit() vfunc and
> then free the cache with g_free() (need to correct size for g_slice_free()).
> 

Added a deinit() vfunc which is called by pygi_callable_cache_free() and g_slice_free() is only called when ->deinit() is the current deinit() function running.

(In reply to comment #10)
> I'm still working on fixing a few of the other issues and completing the split.
> Current callable_cache->function_type is still used for marshaling out args
> although that just seems to be a fast fail if the constructor returns NULL. Can
> that just be moved to constructor_cache_invoke_real() or do you still want a
> fast failure. Currently none of the other caches need special
> marshal_{in,out}() vfuncs.

I'm not sure what code this is referring to specifically, but we probably don't need to optimize for failures especially if it involves extra storage.

> > @@ +716,3 @@
> > +
> > +static PyObject *
> > +_function_cache_invoke_real (PyGIFunctionCache *function_cache,
> > 
> > Avoid underscore prefixes on new functions: "function_cache_invoke_real" is
> > fine for static and prefix anything with "pygi_" if it goes in a header.
> > 
> 
> I'm just following the style for the rest of pygi-cache.c and pygi-invoke.c,
> should I still change this or keep the same coding style as the rest of the
> files?

Either way is would be fine. At some point I'd like to go through the code base and remove all the underscore prefixes.

> > @@ +1003,3 @@
> > +    method_cache = g_slice_new0 (PyGIMethodCache);
> > +    function_cache = (PyGIFunctionCache *) method_cache;
> > +        return;
> > 
> > Since these casts are only use once, we probably don't need the variables and
> > can cast where they are used instead.
> > 
> 
> Removed the PyGIFunctionCache cast. Removing the PyGIFunctionWithInstanceCache
> cast would have a 100+ column line, still remove it?

Good point, yeah keep the lines to 100 or less columns.

> > @@ +1080,3 @@
> > +    function_cache->invoker.native_address = pygi_vfunc_cache_new;
> > +
> > +        PyErr_Format (PyExc_TypeError,
> > 
> > Slightly more tedious, but I think adding this (and the info setup below) to a
> > vfunc_cache_init() function which chains up function_with_instance_cache_init()
> > would be better.
> > 
> 
> That would imply also doing the same for all leaf caches: PyGICCallbackCache,
> PyGIConstructorCache, PyGIMethodCache and PyGIVFuncCache. Only the leaf caches
> without any inheritance are missing (de)init().

Ok, probably overkill then.

> > ::: gi/pygi-ccallback.c
> > @@ +38,3 @@
> >      }
> > 
> > +    result = pygi_ccallback_cache_invoke( self->cache,
> > 
> > I realize the style in pygobject is a mess, but I've been slowly trying to
> > change things over to having a space prior to open parens and none after with
> > multi-line arguments left aligned:
> > 
> >     result = pygi_ccallback_cache_invoke (self->cache,
> >                                           args,
> > 
> 
> Done, is there an example on how I should be formatting all new code. I've been
> attempting to follow what is already there but, as you said, it is a mess.
> 
> Should I format things according to the glib style format, but with different
> brace positions and 4 spaces for indentation?

Yep, that sounds right.

> > ::: gi/pygi-closure.c
> > @@ +929,2 @@
> >      if (callable_cache != NULL)
> > +        child_offset = callable_cache->args_offset;
> > 
> > We might be able to break this out into a separate patch before the main one.
> > Even though it will add assignment code to _args_cache_generate which would be
> > subsequently withdrawn, it makes reviewing and bisecting easier.
> > 
> 
> I could, although there are other places that do the same check and benefit
> from callable_cache->args_offset. 

Break the other places out into the same patch as well. In general if there are changes that make sense independently from the main patch, we can clean them up as individual patches and commit them sooner.

> > ::: gi/pygi-info.c
> > @@ +185,3 @@
> > +
> > +            if (flags & GI_FUNCTION_IS_CONSTRUCTOR)
> > +            GIFunctionInfoFlags flags;
> > 
> > Can this be setup so CallableCache has a "free" virtual method? Then all that
> > would be needed is: self->cache->free (self->cache)
> > 
> 
> Yes I think that would be very helpful and might actually be a good case for
> having deinit() functions. If I also moved from g_slice_new0() to g_malloc0()
> then I could have pygi_callable_cache_free() just call the deinit() vfunc and
> then free the cache with g_free() (need to correct size for g_slice_free()).

I think moving to malloc would be fine. So the idea would be the deinit() vfuncs would never free the main cache struct but pygi_callable_cache_free() would? Something like:

void pygi_callable_cache_free (CallableCache *cache) {
    cache->deinit (cache);
    g_free (cache);
}

(In reply to comment #13)
> (In reply to comment #10)
> > I'm still working on fixing a few of the other issues and completing the split.
> > Current callable_cache->function_type is still used for marshaling out args
> > although that just seems to be a fast fail if the constructor returns NULL. Can
> > that just be moved to constructor_cache_invoke_real() or do you still want a
> > fast failure. Currently none of the other caches need special
> > marshal_{in,out}() vfuncs.
> 
> I'm not sure what code this is referring to specifically, but we probably don't
> need to optimize for failures especially if it involves extra storage.
> 

Now it is done in _constructor_cache_invoke_real() after chaining up.


> > > ::: gi/pygi-closure.c
> > > @@ +929,2 @@
> > >      if (callable_cache != NULL)
> > > +        child_offset = callable_cache->args_offset;
> > > 
> > > We might be able to break this out into a separate patch before the main one.
> > > Even though it will add assignment code to _args_cache_generate which would be
> > > subsequently withdrawn, it makes reviewing and bisecting easier.
> > > 
> > 
> > I could, although there are other places that do the same check and benefit
> > from callable_cache->args_offset. 
> 
> Break the other places out into the same patch as well. In general if there are
> changes that make sense independently from the main patch, we can clean them up
> as individual patches and commit them sooner.
> 

OK, I will split up and attach.

> > > ::: gi/pygi-info.c
> > > @@ +185,3 @@
> > > +
> > > +            if (flags & GI_FUNCTION_IS_CONSTRUCTOR)
> > > +            GIFunctionInfoFlags flags;
> > > 
> > > Can this be setup so CallableCache has a "free" virtual method? Then all that
> > > would be needed is: self->cache->free (self->cache)
> > > 
> > 
> > Yes I think that would be very helpful and might actually be a good case for
> > having deinit() functions. If I also moved from g_slice_new0() to g_malloc0()
> > then I could have pygi_callable_cache_free() just call the deinit() vfunc and
> > then free the cache with g_free() (need to correct size for g_slice_free()).
> 
> I think moving to malloc would be fine. So the idea would be the deinit()
> vfuncs would never free the main cache struct but pygi_callable_cache_free()
> would? Something like:
> 
> void pygi_callable_cache_free (CallableCache *cache) {
>     cache->deinit (cache);
>     g_free (cache);
> }


Yeah it could either do that or check which vfunc is set or pass a free boolean. Currently I have it setup to check the vfunc and apparently g_slice_*() might actually be a bit faster for small frequent allocations (but who really knows when compared to tcmalloc/...).

Review of attachment 282096 [details] [review]:

::: gi/pygi-cache.c
@@ +646,1 @@
+    if (cache->deinit == _callable_cache_deinit_real)

It would be nice to get rid of this with the malloc idea.

@@ +685,3 @@
+
+    if (!cache->generate_args_cache (cache, callable_info)) {
+        cache->args_cache = g_ptr_array_new_full (n_args, (GDestroyNotify) pygi_arg_cache_free);

Possibly leave deinit calls out of failures within init implementations and instead leave it up to the allocator to call deinit/free?

@@ +975,3 @@
+
+    if (!_function_with_instance_cache_init (fwi_cache, info)) {
+

Given the above comments, this (and the rest of the allocators) would then become:

if (!_function_with_instance_cache_init (fwi_cache, info)) {
   g_callable_cache_free (PyGIMethodCache, method_cache);
}

(In reply to comment #14)
> (In reply to comment #13)
> > I think moving to malloc would be fine. So the idea would be the deinit()
> > vfuncs would never free the main cache struct but pygi_callable_cache_free()
> > would? Something like:
> > 
> > void pygi_callable_cache_free (CallableCache *cache) {
> >     cache->deinit (cache);
> >     g_free (cache);
> > }
> 
> 
> Yeah it could either do that or check which vfunc is set or pass a free
> boolean. Currently I have it setup to check the vfunc and apparently
> g_slice_*() might actually be a bit faster for small frequent allocations (but
> who really knows when compared to tcmalloc/...).

It probably makes sense to move to malloc to express the code structure cleanly at this point. We can always change this later by any number of techniques if we find there is actually a performance difference.

(In reply to comment #15)
> Review of attachment 282096 [details] [review]:
> 
> ::: gi/pygi-cache.c
> @@ +646,1 @@
> +    if (cache->deinit == _callable_cache_deinit_real)
> 
> It would be nice to get rid of this with the malloc idea.
> 
> @@ +685,3 @@
> +
> +    if (!cache->generate_args_cache (cache, callable_info)) {
> +        cache->args_cache = g_ptr_array_new_full (n_args, (GDestroyNotify)
> pygi_arg_cache_free);
> 
> Possibly leave deinit calls out of failures within init implementations and
> instead leave it up to the allocator to call deinit/free?
> 
> @@ +975,3 @@
> +
> +    if (!_function_with_instance_cache_init (fwi_cache, info)) {
> +
> 
> Given the above comments, this (and the rest of the allocators) would then
> become:
> 
> if (!_function_with_instance_cache_init (fwi_cache, info)) {
>    g_callable_cache_free (PyGIMethodCache, method_cache);
> }

For caches like PyGIFunctionCache this would be an issue as there is no easy way to determine that creating the invoker failed. Considering the other possible ordering issues I think it would probably be easier to just continue calling deinit() and returning False in init() functions.


(In reply to comment #16)
> (In reply to comment #14)
> > (In reply to comment #13)
> > > I think moving to malloc would be fine. So the idea would be the deinit()
> > > vfuncs would never free the main cache struct but pygi_callable_cache_free()
> > > would? Something like:
> > > 
> > > void pygi_callable_cache_free (CallableCache *cache) {
> > >     cache->deinit (cache);
> > >     g_free (cache);
> > > }
> > 
> > 
> > Yeah it could either do that or check which vfunc is set or pass a free
> > boolean. Currently I have it setup to check the vfunc and apparently
> > g_slice_*() might actually be a bit faster for small frequent allocations (but
> > who really knows when compared to tcmalloc/...).
> 
> It probably makes sense to move to malloc to express the code structure cleanly
> at this point. We can always change this later by any number of techniques if
> we find there is actually a performance difference.

Sounds great, I will modify the patch with respect to these changes.

Created attachment 282145 [details] [review]
Added args_offset to the cache instead of checking the function type

Created attachment 282150 [details] [review]
Split the callable cache into the different types v4

This is in preparation for adding closure caches.

So I've been looking at converting the closure code to using the caches. It seems that the new pygi_invoke_*() functions invoke state needs might not match up with what is in PyGIInvokeState. Should I instead have two state structs PyGI{C,Py}InvokeState? Or just one where the args have different meanings depending on the function being called.

It also seems that many of the pygi_invoke_*() functions will need to be renamed for clarity and correct mapping.

For example:
  pygi_invoke_marshal_{in,out}_args() -> pygi_invoke_marshal_to_py_{in,out}_args()

  pygi_invoke_callable() -> pygi_invoke_c_callable()

Also the naming on the new pygi_invoke_*() functions for calling from C:
  pygi_invoke_marshal_from_py_{in,out}_args()
  pygi_invoke_py_callable()


These names are based on the current names for marshaling and cleanup. If these renames are OK I will rename them in a separate patch.


It might make more sense to move the contents of _function_cache_invoke_real() into pygi_invoke_callable() and keep the rest of it private. This would also allow us to remove the invoke() vfunc and simply pass the user_data for pygi_ccallback_cache_invoke() to pygi_invoke_callable(). It also might make sense to split things up into pygi-invoke-{c,py}.* to avoid boat and confusion. Bloat is also starting to hit pygi-cache.c which is now 1000+ lines, yet the first 600 lines are just arg cache stuff.

Looking forward to input and guidance on how to proceed!

Review of attachment 282145 [details] [review]:

Looks good.

(In reply to comment #20)
> So I've been looking at converting the closure code to using the caches. It
> seems that the new pygi_invoke_*() functions invoke state needs might not match
> up with what is in PyGIInvokeState. Should I instead have two state structs
> PyGI{C,Py}InvokeState? Or just one where the args have different meanings
> depending on the function being called.

Off hand I can't really answer that as the answer depends on a bit of complexity. Some of the individual argument marshallers are dependent on the state for getting things like the length argument for arrays (See [1]). As side note, for vfuncs and callbacks (C calling a Python function) we don't need to skip length argument marshaling as is done here [2], we basically have to match the behavior of this bug [3] otherwise we break API.

Even more complex than array length is callback destroy notify and user data [4] and [5]. While we don't need to handle this case for calling Python from C, it is just an example of the argument dependency that will be part of the challenge when having multiple state structs.

I guess one approach would be to abstract accessing places where the state need to diverge (it would be good to hear specifics on that).

> It also seems that many of the pygi_invoke_*() functions will need to be
> renamed for clarity and correct mapping.
> 
> For example:
>   pygi_invoke_marshal_{in,out}_args() ->
> pygi_invoke_marshal_to_py_{in,out}_args()
> 
>   pygi_invoke_callable() -> pygi_invoke_c_callable()
> 
> Also the naming on the new pygi_invoke_*() functions for calling from C:
>   pygi_invoke_marshal_from_py_{in,out}_args()
>   pygi_invoke_py_callable()
> 
> 
> These names are based on the current names for marshaling and cleanup. If these
> renames are OK I will rename them in a separate patch.

The renaming seems fine. If you haven't already looked at it, study pygi-closure.c because this is what will be either modified to use the caches or replaced... basically _pygi_closure_handle is the ffi C callback which wraps our Python vfuncs and callbacks. This is essentially where pygi_invoke_marshal_to_py_{in,out} would be called, these correlate roughly to _pygi_closure_convert_arguments and _pygi_closure_set_out_arguments. I guess the caches for this case would then be stored on the PyGIClosure struct.

Side note for down the rode, using PyGIClosure to store single vfunc caches will be fine but callbacks will probably be redundant. So connecting the different callbacks to different instances using the same connect function would create a new cache for each connection (e.g. button.connect()). We will probably want to share those caches at some point in a hash keyed by something like "<repo>.<class>.<method>".

> It might make more sense to move the contents of _function_cache_invoke_real()
> into pygi_invoke_callable() and keep the rest of it private. This would also
> allow us to remove the invoke() vfunc and simply pass the user_data for
> pygi_ccallback_cache_invoke() to pygi_invoke_callable().

This sounds fine.

> It also might make
> sense to split things up into pygi-invoke-{c,py}.* to avoid boat and confusion.
> Bloat is also starting to hit pygi-cache.c which is now 1000+ lines, yet the
> first 600 lines are just arg cache stuff.

Makes sense for it to go in a new file. Also consider hacking the new caches into pygi-closure.c piecemeal. For example, _pygi_closure_convert_arguments could start to use parts of the argument cache instead of calling GI functions like g_arg_info_get_direction() and g_arg_info_get_ownership_transfer() but don't yet replace the argument marshalling call to _pygi_argument_to_object(). Things like this could stand alone as an individual patch which help tell a "refactoring story". 

[1] https://git.gnome.org/browse/pygobject/tree/gi/pygi-array.c?id=3.13.3#n516
[2] https://git.gnome.org/browse/pygobject/tree/gi/pygi-invoke.c?id=3.13.3#n485
[3] https://bugzilla.gnome.org/show_bug.cgi?id=652115#c7
[4] https://git.gnome.org/browse/pygobject/tree/gi/pygi-closure.c?id=3.13.3#n754
[5] https://git.gnome.org/browse/pygobject/tree/gi/pygi-closure.c?id=3.13.3#n807

(In reply to comment #22)
> Even more complex than array length is callback destroy notify and user data
> [4] and [5]. While we don't need to handle this case for calling Python from C,
> it is just an example of the argument dependency that will be part of the
> challenge when having multiple state structs.

Turns out this isn't completely correct. While we don't handle to-python marshalling of arguments which are C callbacks [1] in the context of Python calling a C function, it seems like we do for vfuncs which receive input arguments which are C callbacks [2]. So I think [1] will need to be implemented using [2] as an example at some point.

[1] https://git.gnome.org/browse/pygobject/tree/gi/pygi-closure.c?id=3.13.3#n844
[2] https://git.gnome.org/browse/pygobject/tree/gi/pygi-closure.c?id=3.13.3#n422

Created attachment 282277 [details] [review]
Split the callable cache into the different types  v5

This is in preparation for adding closure caches.

Now only one new function in pygi-invoke.c is made public and it does the work that _function_cache_invoke() used to.

Created attachment 282293 [details] [review]
Use the caches for closures, but not for marshalling the arguments

There are still a few GITypeInfo calls but they all use type infos from the arg cache. I could move these, but there are only a few left....

Created attachment 282445 [details] [review]
Use the caches for closures, but not for marshaling the arguments v2

All GITypeInfo calls have now been removed.

Created attachment 282484 [details] [review]
Use the caches for marshaling the arguments in closures

So this works, except for a number of tests for floating refs. Seems to be related to https://git.gnome.org/browse/pygobject/tree/gi/pygi-argument.c#n1423


I've started to look into the issue but don't really know how this can be fixed without breaking a load of other tests. Any suggestions would be highly appreciated!


One potential issue with this patch is that it changes callbacks that used to have the array length in them, i.e.:
-        def callback(one, one_length, two, two_length):
+        def callback(one, two):
             TestCallbacks.callargs.append((one, two))
             return len(TestCallbacks.callargs)


So if backwards compatibility is required for this I will need to modify the arg caches generated to get this work. One idea is to require that the array cache actually creates a "real" cache for the length arg (with to/from_py set) and then set the meta type to PARENT in closure_cache_new() so it is included in the args for closures only.

Review of attachment 282484 [details] [review]:

::: gi/pygi-closure.c
@@ +42,2 @@
 static void
+_pygi_closure_assign_pyobj_to_retval (gpointer retval,

I think this probably be split off into a smaller patch which is limited to changing return_arg as an out argument.

@@ +124,2 @@
 static void
+_pygi_closure_assign_pyobj_to_out_argument (gpointer out_arg,

Same thing here.

@@ -435,3 @@
-                                                              arg_cache->type_info, &free_array);
-
-                if (arg_tag == GI_TYPE_TAG_ARRAY)

Very nice seeing this removed!

@@ -860,3 @@
 
-static void
-_arg_cache_from_py_interface_callback_setup (PyGIArgCache *arg_cache,

Is there any particular reason this was rolled into pygi_arg_callback_setup_from_info() ? I think it's fine, but if it is just general cleanup or works in isolation from the rest of the changes, consider moving it into a new patch.

::: gi/pygi-error.c
@@ +267,3 @@
     if (direction & PYGI_DIRECTION_TO_PYTHON) {
         arg_cache->to_py_marshaller = _pygi_marshal_to_py_gerror;
+        arg_cache->meta_type = PYGI_META_ARG_TYPE_PARENT;

This seems right since we have "throws" for the case where we want to convert GError's to Python exceptions. Couldn't this also go in a new patch though?

::: gi/pygi-marshal-cleanup.c
@@ +199,3 @@
 void
 pygi_marshal_cleanup_args_to_py_parameter_fail (PyGIInvokeState   *state,
+                                                PyGICallableCache *cache,

We can save unrelated formatting patches for another time.

::: tests/test_gi.py
@@ +2098,3 @@
             self.n_variants = None
 
+        def do_test_variant_array_in(self, variants):

Yep, we do need to keep this compatible :(
See comment #22 and https://bugzilla.gnome.org/show_bug.cgi?id=652115#c7

::: tests/test_object_marshaling.py
@@ +116,3 @@
         self.assertTrue(vfuncs_ref() is None)
 
+#    def test_vfunc_return_object_transfer_none(self):

I know commenting this out is temporary but it may be easier to use @unittest.skip

I'll make the changes and splits to this patch and attach tomorrow, any comments on the other patches?


(In reply to comment #28)
> Review of attachment 282484 [details] [review]:
> 
> ::: gi/pygi-closure.c
> @@ +42,2 @@
>  static void
> +_pygi_closure_assign_pyobj_to_retval (gpointer retval,
> 
> I think this probably be split off into a smaller patch which is limited to
> changing return_arg as an out argument.
> 
> @@ +124,2 @@
>  static void
> +_pygi_closure_assign_pyobj_to_out_argument (gpointer out_arg,
> 
> Same thing here.
> 

Will do.


> @@ -860,3 @@
> 
> -static void
> -_arg_cache_from_py_interface_callback_setup (PyGIArgCache *arg_cache,
> 
> Is there any particular reason this was rolled into
> pygi_arg_callback_setup_from_info() ? I think it's fine, but if it is just
> general cleanup or works in isolation from the rest of the changes, consider
> moving it into a new patch.
> 

All but settings the marshaling functions was also needed by to_py, no need to duplicated it.

> ::: gi/pygi-error.c
> @@ +267,3 @@
>      if (direction & PYGI_DIRECTION_TO_PYTHON) {
>          arg_cache->to_py_marshaller = _pygi_marshal_to_py_gerror;
> +        arg_cache->meta_type = PYGI_META_ARG_TYPE_PARENT;
> 
> This seems right since we have "throws" for the case where we want to convert
> GError's to Python exceptions. Couldn't this also go in a new patch though?
> 

Yeah it can and I'll attach it as another patch. Parts of this patch are just to get input and make sure I'm actually going in the correct direction.

> ::: gi/pygi-marshal-cleanup.c
> @@ +199,3 @@
>  void
>  pygi_marshal_cleanup_args_to_py_parameter_fail (PyGIInvokeState   *state,
> +                                                PyGICallableCache *cache,
> 
> We can save unrelated formatting patches for another time.
> 

Yup, that just happened to sneak in...

> ::: tests/test_gi.py
> @@ +2098,3 @@
>              self.n_variants = None
> 
> +        def do_test_variant_array_in(self, variants):
> 
> Yep, we do need to keep this compatible :(
> See comment #22 and https://bugzilla.gnome.org/show_bug.cgi?id=652115#c7
> 

I figured it would. One idea I had is to make array's create length caches that have {to,from}_py set and then in pygi_closure_cache_new() it can simply set all length arg caches so they have a meta type of PYGI_META_ARG_TYPE_PARENT and thus wont be skipped.

> ::: tests/test_object_marshaling.py
> @@ +116,3 @@
>          self.assertTrue(vfuncs_ref() is None)
> 
> +#    def test_vfunc_return_object_transfer_none(self):
> 
> I know commenting this out is temporary but it may be easier to use
> @unittest.skip

Not with a comment plugin, select bad function and then Ctrl+M, Done!


Thanks for the review!

Review of attachment 282277 [details] [review]:

LGTM. nice work! I think it would be helpful to add a bit more text to the commit message explaining what was done here.

Review of attachment 282445 [details] [review]:

Nice work. Please add a commit log giving an overview of what was done.

::: gi/pygi-closure.c
@@ +412,2 @@
                     if (destroy_notify_arg != -1)
+                        destroy_notify = (GDestroyNotify) g_args[destroy_notify_arg].v_pointer;

Nice catch, looks like this was a bug? probably makes sense to break it out into a new patch.

Created attachment 282571 [details] [review]
Split the callable cache into the different types v6

Instead of doing different things based on the various function types
this adds vfuncs for generate_args_cache() and invoke() which are then
specialized for the various function types. Also add a calling context
to the callable cache which is then used to determine the direction
when generating the arg caches.

This is in preparation for adding closure caches.

Created attachment 282572 [details] [review]
Correctly set the destroy notify for callbacks in closures

Created attachment 282573 [details] [review]
Use the caches for closures, but not yet for marshaling the arguments

Instead of using the various GI functions we use the data from the caches.
This also fixes generating an arg cache for a closure as it was missing some
data or simply setting incorrect data. Also, always included the GITypeInfo
until the closures no longer need it for marshaling the arguments.

Created attachment 282574 [details] [review]
Pass the GIArgument to the closure assign functions

This will be required once we use the caches for marshaling.

Created attachment 282576 [details] [review]
Set the correct meta type for GErrors when marshaling to Python

Otherwise we do not pass the GError into python callbacks and
we also want to convert these into Python Exceptions.

Created attachment 282589 [details] [review]
Use the caches for marshaling the arguments in closures

Changes to object marshaling when the calling context is from
C was required to correctly keep the correct floating status.

The array cache has been modified to set to/from_py_marshaller
for the length arg cache. This is required for closures which include
the length arg for backwards compatibility. The closure cache takes
care to change the length arg cache's meta type so it gets marshalled
for closures.

Review of attachment 282572 [details] [review]:

Looks good.

Review of attachment 282574 [details] [review]:

LGTM

Review of attachment 282576 [details] [review]:

Makes sense.

Review of attachment 282571 [details] [review]:

Looks good.

::: gi/pygi-ccallback.c
@@ +81,3 @@
+
+    if (self->cache != NULL) {
+        pygi_callable_cache_free ( (PyGICallableCache *)self->cache);

Nice, this fixes a leak which I believe was showing up as indirectly lost in valgrind.

==16265== 2,672 (128 direct, 2,544 indirect) bytes in 2 blocks are definitely lost in loss record 6,611 of 6,909
==16265==    at 0x4C2745D: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==16265==    by 0x4ED4DAC: PyObject_Malloc (in /usr/lib64/libpython3.3m.so.1.0)
==16265==    by 0x4EDFCA4: PyType_GenericAlloc (in /usr/lib64/libpython3.3m.so.1.0)
==16265==    by 0x11FE9F1B: _pygi_ccallback_new (pygi-ccallback.c:63)
...

Becomes:

==14958== 128 bytes in 2 blocks are definitely lost in loss record 4,666 of 6,902
==14958==    at 0x4C2745D: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==14958==    by 0x4ED4DAC: PyObject_Malloc (in /usr/lib64/libpython3.3m.so.1.0)
==14958==    by 0x4EDFCA4: PyType_GenericAlloc (in /usr/lib64/libpython3.3m.so.1.0)
==14958==    by 0x11FE9F1B: _pygi_ccallback_new (pygi-ccallback.c:63)
...


Seen with:
% make check.valgrind TEST_NAMES=test_gi.TestPythonGObject.test_callback_in_vfunc

Review of attachment 282573 [details] [review]:

LGTM.

Review of attachment 282589 [details] [review]:

Looks fine, one potential change though.

::: gi/pygi-cache.c
@@ +1062,3 @@
     }
 
+    /* For backwards compatibility closures include the array's length.

I like how this is broken out into an isolated "fixup" block. At some point we can conditionalize the block based on a configuration option as noted in bug 652115 and remove it in a major release.

::: gi/pygi-closure.c
@@ +419,3 @@
                 }
+            } else if (arg_cache->meta_type != PYGI_META_ARG_TYPE_PARENT) {
+                continue;

Nice to see all that moved into an argument marshaller.

::: gi/pygi-object.c
@@ +224,3 @@
+     * See: https://bugzilla.gnome.org/show_bug.cgi?id=693400
+     */
+     * references are added by the PyGObject wrapper and avoids the sink

I think this could be broken out into a separate function that and reused from within _pygi_argument_to_object() (so the hack isn't in two places). We can also create more than one cache adapter which is assigned during argument marshalling setup time based on the calling context (it would turn this into a one time conditional at cache setup time).

Also please make sure you add the bugzilla ticket url to the bottom of the commit messages before committing these:

https://bugzilla.gnome.org/show_bug.cgi?id=727004

Created attachment 282733 [details] [review]
Always pass along the callable cache to the arg cache constructors

This will be needed in a future patch which requires that the callable
cache is always available.

Created attachment 282734 [details] [review]
Move special handling of GObject from Python when calling from C

This will soon be used in the GObject arg cache marshaling when
the calling context is C.

Created attachment 282735 [details] [review]
Specialize GObject marshaling when called from C

These are needed otherwise the floating status of
the GObject will not be kept.

Created attachment 282736 [details] [review]
Use the caches for marshaling the arguments in closures

Changes to object marshaling when the calling context is from
C was required to correctly keep the correct floating status.

The array cache has been modified to set to/from_py_marshaller
for the length arg cache. This is required for closures which include
the length arg for backwards compatibility. The closure cache takes
care to change the length arg cache's meta type so it gets marshalled
for closures.

Review of attachment 282733 [details] [review]:

Sure

Review of attachment 282734 [details] [review]:

::: gi/pygi-argument.c
@@ +1421,3 @@
                 case GI_INFO_TYPE_INTERFACE:
                 case GI_INFO_TYPE_OBJECT:
+                    object = pygi_arg_gobject_to_py_from_c (arg, transfer);

Perhaps name it something very explicit like pygi_arg_gobject_to_py_with_floating_hack() ?

Review of attachment 282735 [details] [review]:

I'm not too keen on the various to_py_from_py, from_py_from_py naming, but I don't have a better idea and we can always change it later.

(In reply to comment #50)
> Review of attachment 282734 [details] [review]:
> 
> ::: gi/pygi-argument.c
> @@ +1421,3 @@
>                  case GI_INFO_TYPE_INTERFACE:
>                  case GI_INFO_TYPE_OBJECT:
> +                    object = pygi_arg_gobject_to_py_from_c (arg, transfer);
> 
> Perhaps name it something very explicit like
> pygi_arg_gobject_to_py_with_floating_hack() ?

I think it gives more information about when to use it than with_floating_hack() because is specifically says that it is for marshaling when called from C. Otherwise one would wonder, should we simply always use the with_floating_hack() variant or when are we supposed to use it. This would also follow the new naming conventions when a function is dependent on the calling context.

Does that make sense or should I still rename it with_floating_hack()?

(In reply to comment #51)
> Review of attachment 282735 [details] [review]:
> 
> I'm not too keen on the various to_py_from_py, from_py_from_py naming, but I
> don't have a better idea and we can always change it later.

Yeah I couldn't either, however having thought about it I think it should instead be {to,from}_py_called_from_{c,py} which would make more sense as it is dependent on the calling context. I think it would reduce the confusion and simply extend the current naming conventions used for any function which is dependent on the calling context.

Should I use the original name or this one?

Created attachment 282829 [details] [review]
tests: Add constructor with an error argument

Make sure a constructor that sets a GError works correctly.

Created attachment 282830 [details] [review]
Fix raising an error in a constructor

It should raise a Python exception instead of warnings about a constructor returning NULL.

(In reply to comment #52)
> Does that make sense or should I still rename it with_floating_hack()?

I was thinking with_floating_hack() gives a nice clue that it is not how we want to do things and should probably be removed at some point. We also have comments for this, so I don't feel too strongly either way.

> (In reply to comment #51)
> > Review of attachment 282735 [details] [review] [details]:
> > 
> > I'm not too keen on the various to_py_from_py, from_py_from_py naming, but I
> > don't have a better idea and we can always change it later.
> 
> Yeah I couldn't either, however having thought about it I think it should
> instead be {to,from}_py_called_from_{c,py} which would make more sense as it is
> dependent on the calling context. I think it would reduce the confusion and
> simply extend the current naming conventions used for any function which is
> dependent on the calling context.
> 
> Should I use the original name or this one?

Adding "called_from" definitely makes it clearer so go with that.

Review of attachment 282736 [details] [review]:

Looks good.

Review of attachment 282829 [details] [review]:

LGTM. Same comment about bugzilla ticket as last line though.

Review of attachment 282830 [details] [review]:

Please commit with the given notes.

::: tests/test_gi.py
@@ +2112,3 @@
         self.assertTrue(isinstance(object_, self.Object))
 
+    def test_object_fail(self):

Same comment about using @unittest.skipUnless(...) from patch review in bug 702508.

@@ +2116,3 @@
+            GIMarshallingTests.Object.new_fail(int_=42)
+
+        self.assertRaises(GLib.Error, failed_constructor)

Optionally change this to:

with self.assertRaises(GLib.Error):
    GIMarshallingTests.Object.new_fail(int_=42)

More specific tests and a regex variation can also be used if wanted:
https://docs.python.org/2/library/unittest.html#unittest.TestCase.assertRaises

It looks like the new test is showing some leaks:

$ make check.valgrind TEST_NAMES=test_everything.TestCallbacks.test_callback_scope_call_array_inout

==26999== 16 bytes in 1 blocks are definitely lost in loss record 979 of 8,546
==26999==    at 0x4C2745D: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==26999==    by 0x4C2932B: realloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==26999==    by 0x14D62E92: g_realloc (gmem.c:162)
==26999==    by 0x14D257DB: g_array_maybe_expand (garray.c:779)
==26999==    by 0x14D24997: g_array_sized_new (garray.c:206)
==26999==    by 0x13E89B06: _pygi_marshal_from_py_array (pygi-array.c:224)
==26999==    by 0x13E80F8F: _pygi_closure_set_out_arguments (pygi-closure.c:509)
==26999==    by 0x13E81231: _pygi_closure_handle (pygi-closure.c:585)
==26999==    by 0xE8F2B7A: ffi_closure_unix64_inner (in /usr/lib64/libffi.so.6.0.1)
==26999==    by 0xE8F2EF3: ffi_closure_unix64 (in /usr/lib64/libffi.so.6.0.1)
==26999==    by 0x1A0EABCE: regress_test_array_inout_callback (regress.c:3380)
==26999==    by 0xE8F2D8B: ffi_call_unix64 (in /usr/lib64/libffi.so.6.0.1)
==26999== 
==26999== 20 bytes in 1 blocks are definitely lost in loss record 1,026 of 8,546
==26999==    at 0x4C2745D: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==26999==    by 0x14D62D9E: g_malloc (gmem.c:97)
==26999==    by 0x14D630C6: g_malloc_n (gmem.c:338)
==26999==    by 0x1A0EAB83: regress_test_array_inout_callback (regress.c:3376)
==26999==    by 0xE8F2D8B: ffi_call_unix64 (in /usr/lib64/libffi.so.6.0.1)
==26999==    by 0xE8F26BB: ffi_call (in /usr/lib64/libffi.so.6.0.1)
==26999==    by 0x13E84DCB: pygi_invoke_c_callable (pygi-invoke.c:628)
==26999==    by 0x13E860AB: _function_cache_invoke_real (pygi-cache.c:714)
==26999==    by 0x13E862AA: pygi_function_cache_invoke (pygi-cache.c:793)
==26999==    by 0x13E84ECD: pygi_callable_info_invoke (pygi-invoke.c:671)
==26999==    by 0x13E85011: _wrap_g_callable_info_invoke (pygi-invoke.c:708)
==26999==    by 0x13E71E2A: _callable_info_call (pygi-info.c:568)

Looks like the leaks might be related to the cleanup line being commented out:
https://git.gnome.org/browse/pygobject/tree/gi/pygi-closure.c?id=d7b9ef0f#n585

However, un-commenting that causes the test to crash.

Created attachment 282918 [details] [review]
Cleanup input args when marshaling in closures

The cleanup must happen before setting the out args otherwise
the args that cleanup would free are the just set args, not the
original ones.




Ran valgrind and didn't get either of the memory leaks.

Review of attachment 282918 [details] [review]:

Perfect, thanks!

Created attachment 283424 [details] [review]
Fix invalid unref after getting callbable container

This was causing an attempted unref on an invalid object when
setting up callback caches for signals.

Created attachment 283425 [details] [review]
Add virtual "call" method to PyGICClosure

Virtualize the technique for calling the Python callable held in PyGICClosures.
This will allow future customization like instance argument swapping needed
for using PyGICClosure with signals.

Created attachment 283426 [details] [review]
Determine instance argument direction based on calling context

Use the calling context to determine proper argument marshalling direction
within _function_with_instance_cache_generate_args_cache_real().
This is preliminary work for supporting signal closures with caches.

Created attachment 283427 [details] [review]
Add instance argument to closure caches used for signals

GI signals imply an instance argument like methods and vfuncs. Add support
for this within closure cache creation and re-use method argument generation.

Created attachment 283428 [details] [review]
Add swap data to GI closures

Add swap data to PyGIClosure structure and related functions.
This is needed to support connect_object() which automatically
swaps in this object as the instance argument.

Review of attachment 283424 [details] [review]:

To clarify, g_base_info_get_container() is transfer-none, so we don't need to unref the results. It is unclear why this started showing up when implementing signal caches, but it might mean there is a leak somewhere else which is why this wasn't previously failing.

Comment on attachment 283424 [details] [review]
Fix invalid unref after getting callbable container

Committed as: https://git.gnome.org/browse/pygobject/commit/?id=0d2e797

The reason this probably doesn't fail in the normal case is function and vfunc info instances already hold a strong reference from the Python side of things. This reference can then be traced all the way up a reference hierarchy to sys.modules holding a ref to the introspection module. AFAIK Python does not do a full GC at exit. Furthermore, C extension unloading was only added in Python 3 (http://legacy.python.org/dev/peps/pep-3121/) and we don't even make use of it yet (bug 677091).

-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/pygobject/issues/69.