eliminating bse_procedure_execvl (Re: Optimizations)

[Tim Janik <timj gtk org>]

On Tue, 3 Feb 2004, Stefan Westerfeld wrote:

> * No patches: performance of the unmodified build.
>
> stefan@luna1:~/src/beast-performance-eval/beast/tests$ perftest
> 0.980416 seconds for 10000 invocations => 10199.751787 invocations/second, 0.000098 sec per invocation


> * Patch #6: Remove the need for bse_procedure_execvl, by calling
>   bse_procedure_marshal directly. It is possible to remove bse_procedure_execvl
>   entierly now.
>
> Index: bseglue.c
> ===================================================================
> RCS file: /cvs/gnome/beast/bse/bseglue.c,v
> retrieving revision 1.22
> diff -u -p -r1.22 bseglue.c
> --- bseglue.c	27 Jan 2004 15:17:32 -0000	1.22
> +++ bseglue.c	3 Feb 2004 00:11:16 -0000
> @@ -682,8 +682,9 @@ bglue_exec_proc (SfiGlueContext *context
>    if (BSE_TYPE_IS_PROCEDURE (ptype) && G_TYPE_IS_DERIVED (ptype))
>      {
>        BseProcedureClass *proc = g_type_class_ref (ptype);
> -      GValue *ovalues = g_new0 (GValue, proc->n_out_pspecs);
> -      GSList *ilist = NULL, *olist = NULL, *clearlist = NULL;
> +      GValue *ivalues = g_newa (GValue, proc->n_in_pspecs + proc->n_out_pspecs);
> +      GValue *ovalues = ivalues + proc->n_in_pspecs;
> +      GSList *element = NULL, *clearlist = NULL;
>        guint i, sl = sfi_seq_length (params);
>        BseErrorType error;
>
> @@ -694,7 +695,7 @@ bglue_exec_proc (SfiGlueContext *context
>  	    {
>  	      GValue *sfivalue = sfi_seq_get (params, i);
>  	      GValue *bsevalue = bglue_value_from_serializable (sfivalue, pspec);
> -	      ilist = g_slist_prepend (ilist, bsevalue ? bsevalue : sfivalue);
> +	      memcpy (ivalues + i, bsevalue ? bsevalue : sfivalue, sizeof (GValue));

here, you're relocating a malloc'ed value ontto the stack.

>  	      if (bsevalue)
>  		clearlist = g_slist_prepend (clearlist, bsevalue);
>  	    }
> @@ -703,23 +704,21 @@ bglue_exec_proc (SfiGlueContext *context
>  	      GValue *value = sfi_value_empty ();
>  	      g_value_init (value, G_PARAM_SPEC_VALUE_TYPE (pspec));
>  	      g_param_value_set_default (pspec, value);
> -	      ilist = g_slist_prepend (ilist, value);
> +	      memcpy (ivalues + i, value, sizeof (GValue));

same here.

>  	      clearlist = g_slist_prepend (clearlist, value);
>  	    }
>  	}
>        for (i = 0; i < proc->n_out_pspecs; i++)
>  	{
> +	  ovalues[i].g_type = 0;
>  	  g_value_init (ovalues + i, G_PARAM_SPEC_VALUE_TYPE (proc->out_pspecs[i]));
> -	  olist = g_slist_prepend (olist, ovalues + i);
>  	}
>
> -      ilist = g_slist_reverse (ilist);
> -      olist = g_slist_reverse (olist);
> -      error = bse_procedure_execvl (proc, ilist, olist, bglue_marshal_proc, NULL);
> -      g_slist_free (ilist);
> -      g_slist_free (olist);
> -      for (ilist = clearlist; ilist; ilist = ilist->next)
> -	sfi_value_free (ilist->data);
> +      error = bse_procedure_marshal (BSE_PROCEDURE_TYPE (proc), ivalues, ovalues,
> +	                             bglue_marshal_proc, NULL);

here, the values on the stack are validated and the procedure is called.
both, the validation process and the procedure may change the contents
of your values on the stack.

> +
> +      for (element = clearlist; element; element = element->next)
> +	sfi_value_free (element->data);

here, you free the malloc'ed values, without re-relocating the modified
value contents from the stack back into the malloc'ed space. this is going
to segfault and leak sooner or later.

>        g_slist_free (clearlist);
>
>        if (error)
> @@ -733,7 +732,6 @@ bglue_exec_proc (SfiGlueContext *context
>  	retval = bglue_value_to_serializable (ovalues + 0);
>        for (i = 0; i < proc->n_out_pspecs; i++)
>          g_value_unset (ovalues + i);
> -      g_free (ovalues);
>        g_type_class_unref (proc);
>      }
>    else
>
> stefan@luna1:~/src/beast-performance-eval/beast/tests$ perftest
> 0.875381 seconds for 10000 invocations => 11423.596657 invocations/second, 0.000088 sec per invocation

so this patch doesn't work, it needs severe fixing ;)

>
>    Cu... Stefan

---
ciaoTJ