Created attachment 159224 [details] [review]
Step 1 of refactoring _wrap_g_function_info_invoke

Original patch by David Malcom <dmalcolm@redhat.com>

This patch bitrots *REALLY* fast.

Created attachment 159225 [details] [review]
Split invoke into 3 functions

This version replaces g_newa with g_new0 which is a horrendous change.
David Malcom got this to work whilst keeping g_newa around, hopefully he'll
have some comments on how to keep the faster allocators around.

Unfortunately I seem to have blown away my working copy.  I'll try to have another look, but am out of time tonight.


To get it working with g_newa (i.e. stack-base allocations), I grepped for all references to g_newa, and found that they occur in two groups.  I split the initialization phase into three parts, like so, so that all calls to g_newa are in the core function, so that they live for the lifetime of the whole invocation function:

_wrap_g_info_function_invoke(foo)
{
   struct invocation_state s;

   if (_setup_part_one(&s, foo)) {
       goto out;
   }

   /* first group of g_newa calls */
   s.something = g_newa();
   s.something_else = g_newa();

   if (_setup_part_two(&s, foo)) {
       goto out;
   }

   /* second group of g_newa calls */
   s.anotherthing = g_newa();

   if (_setup_part_three(&s, foo)) {
       goto out;
   }

   if (_perform_invocation(&s, foo)) {
       goto out;
   }

   if (_process_return_value(&s, foo)) {
       goto out;
   }

   if (_cleanup(&s, foo)) {
       goto out;
   }

out:
   
   /* etc */
   

}

Typo; those
  if (_bar()) {
     goto out;
  } 
should all read:
  if (!_bar()) {
     goto out;
  }

Probably need the memset at the top for sanity's sake, as well.

There are some places in the patch where a comment ending in an "s" followed by a  period got changed to "s->"  e.g.:
   /* Other types don't have fields-> */

Probably should search and replace
  s-> */
with
  s. */

to fix these back.

Created attachment 159299 [details] [review]
Split invoke into 3 functions

function into 3 steps.  We could/should rename _stepX to something more descriptive
perhaps?

I really would like people to comment on this ASAP as it bit rots really fast.

Review of attachment 159224 [details] [review]:

Apart from two naming nitpicks, I think this is good to go, provided that the future patches that depend on it are also approved (will look now to the next one).

::: gi/pygi-info.c
@@ +487,3 @@
 }
 
+struct invocation_state

Types use camel case

@@ +530,3 @@
+                              PyObject     *py_args)
+{
+    struct invocation_state s;

Would be too inconvenient to use more letters for that variable? Such as 'state' or 'context'.

Review of attachment 159299 [details] [review]:

I'm quite concerned about splitting that func in seemingly arbitrary parts called *_step1, *_step2, etc. I feel strongly that the structure of the code should follow what it does. I'm going to explore alternatives.

Any thoughts tomeu?  Simon?

(In reply to comment #9)
> Any thoughts tomeu?  Simon?

Still working on a refactoring that splits the function in meaningful units. Will post soon.

Created attachment 159579 [details] [review]
One more step at refactoring _wrap_g_function_info_invoke

Reduces the main invoke function to:

static PyObject *
_wrap_g_function_info_invoke (PyGIBaseInfo *self, PyObject *py_args)
{
    struct invocation_state state;

    _initialize_invocation_state (&state, self->info, py_args);

    if (!_prepare_invocation_state (&state, self->info, py_args)) {
        _free_invocation_state (&state);
        return NULL;
    }

    if (!_invoke_function (&state, self->info, py_args)) {
        _free_invocation_state (&state);
        return NULL;
    }

    if (!_process_invocation_state (&state, self->info, py_args)) {
        _free_invocation_state (&state);
        return NULL;
    }

    return state.return_value;
}

We can further clean up the mess by separating the invoke code to a pygi-invoke.c file
and by further refactoring the lower level functions.

Replaces g_newa calls with g_slice_alloc0, which I think is a good thing to
do because we were noticeably reducing the stack space available to the called C code.
If we later realize this has a noticeably impact on performance, we will be able to
optimize this with greater knowledge of cause.

What valgrind says about the new memory behavior:

Before:

==7981== HEAP SUMMARY:
==7981==     in use at exit: 1,473,367 bytes in 14,503 blocks
==7981==   total heap usage: 167,137 allocs, 152,634 frees, 12,864,525 bytes allocated
==7981== 
==7981== LEAK SUMMARY:
==7981==    definitely lost: 13,219 bytes in 323 blocks
==7981==    indirectly lost: 3,272 bytes in 63 blocks
==7981==      possibly lost: 1,385,584 bytes in 13,435 blocks
==7981==    still reachable: 71,292 bytes in 682 blocks
==7981==         suppressed: 0 bytes in 0 blocks


After:

==8061== HEAP SUMMARY:
==8061==     in use at exit: 1,462,009 bytes in 14,491 blocks
==8061==   total heap usage: 78,184 allocs, 63,693 frees, 8,586,075 bytes allocated
==8061== 
==8061== LEAK SUMMARY:
==8061==    definitely lost: 13,219 bytes in 323 blocks
==8061==    indirectly lost: 3,272 bytes in 63 blocks
==8061==      possibly lost: 1,374,444 bytes in 13,429 blocks
==8061==    still reachable: 71,074 bytes in 676 blocks
==8061==         suppressed: 0 bytes in 0 blocks

Attachment 159224 [details] pushed as 7fc5528 - Step 1 of refactoring _wrap_g_function_info_invoke
Attachment 159579 [details] pushed as 64fa8f9 - One more step at refactoring _wrap_g_function_info_invoke