use sfi_categorize_type (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 #5: Categorizing and using switch is more efficient than looking up
>   each value type seperately. There are one or two more cases in the code
>   where this could be done (value_as_num and value_as_real in sfiprimitives.c).
>
>   Should I also remove the g_return_if_fails, since the values are already
>   categorized?

no, look at the code again. the checks are performed on the destination value,
after the source value has been identified. this is to make sure the value
types match for the copy.

> Index: sfivalues.c
> ===================================================================
> RCS file: /cvs/gnome/beast/sfi/sfivalues.c,v
> retrieving revision 1.14
> diff -u -p -r1.14 sfivalues.c
> --- sfivalues.c	27 Jan 2004 15:17:09 -0000	1.14
> +++ sfivalues.c	2 Feb 2004 20:57:09 -0000
> @@ -362,39 +362,49 @@ void
>  sfi_value_copy_deep (const GValue *src_value,
>  		     GValue       *dest_value)
>  {
> +  SfiSCategory scat;
> +
>    g_return_if_fail (G_IS_VALUE (src_value));
>    g_return_if_fail (G_IS_VALUE (dest_value));
> -
> -  if (SFI_VALUE_HOLDS_SEQ (src_value))
> +
> +  scat = sfi_categorize_type (G_VALUE_TYPE (src_value)) & SFI_SCAT_TYPE_MASK;
> +  switch (scat)
>      {
> -      SfiSeq *seq;
> -      g_return_if_fail (SFI_VALUE_HOLDS_SEQ (dest_value));
> -      seq = sfi_value_get_seq (src_value);
> -      sfi_value_take_seq (dest_value, seq ? sfi_seq_copy_deep (seq) : NULL);
> -    }
> -  else if (SFI_VALUE_HOLDS_REC (src_value))
> -    {
> -      SfiRec *rec;
> -      g_return_if_fail (SFI_VALUE_HOLDS_REC (dest_value));
> -      rec = sfi_value_get_rec (src_value);
> -      sfi_value_take_rec (dest_value, rec ? sfi_rec_copy_deep (rec) : NULL);
> -    }
> -  else if (SFI_VALUE_HOLDS_BBLOCK (src_value))
> -    {
> -      SfiBBlock *bblock;
> -      g_return_if_fail (SFI_VALUE_HOLDS_BBLOCK (dest_value));
> -      bblock = sfi_value_get_bblock (src_value);
> -      sfi_value_take_bblock (dest_value, bblock ? sfi_bblock_copy_deep (bblock) : NULL);
> -    }
> -  else if (SFI_VALUE_HOLDS_FBLOCK (src_value))
> -    {
> -      SfiFBlock *fblock;
> -      g_return_if_fail (SFI_VALUE_HOLDS_FBLOCK (dest_value));
> -      fblock = sfi_value_get_fblock (src_value);
> -      sfi_value_take_fblock (dest_value, fblock ? sfi_fblock_copy_deep (fblock) : NULL);
> +      case SFI_SCAT_SEQ:
> +	{
> +	  SfiSeq *seq;
> +	  g_return_if_fail (SFI_VALUE_HOLDS_SEQ (dest_value));
> +	  seq = sfi_value_get_seq (src_value);
> +	  sfi_value_take_seq (dest_value, seq ? sfi_seq_copy_deep (seq) : NULL);
> +	}
> +	break;
> +      case SFI_SCAT_REC:
> +	{
> +	  SfiRec *rec;
> +	  g_return_if_fail (SFI_VALUE_HOLDS_REC (dest_value));
> +	  rec = sfi_value_get_rec (src_value);
> +	  sfi_value_take_rec (dest_value, rec ? sfi_rec_copy_deep (rec) : NULL);
> +	}
> +	break;
> +      case SFI_SCAT_BBLOCK:
> +	{
> +	  SfiBBlock *bblock;
> +	  g_return_if_fail (SFI_VALUE_HOLDS_BBLOCK (dest_value));
> +	  bblock = sfi_value_get_bblock (src_value);
> +	  sfi_value_take_bblock (dest_value, bblock ? sfi_bblock_copy_deep (bblock) : NULL);
> +	}
> +	break;
> +      case SFI_SCAT_FBLOCK:
> +	{
> +	  SfiFBlock *fblock;
> +	  g_return_if_fail (SFI_VALUE_HOLDS_FBLOCK (dest_value));
> +	  fblock = sfi_value_get_fblock (src_value);
> +	  sfi_value_take_fblock (dest_value, fblock ? sfi_fblock_copy_deep (fblock) : NULL);
> +	}
> +	break;
> +      default:
> +	g_value_copy (src_value, dest_value);
>      }
> -  else
> -    g_value_copy (src_value, dest_value);
>  }
>
> stefan@luna1:~/src/beast-performance-eval/beast/tests$ perftest
> 0.887342 seconds for 10000 invocations => 11269.611276 invocations/second, 0.000089 sec per invocation

looks good, please apply with appropriate ChangeLog entry.

> Any comments? I'd like to put the changes #1 - #6 into the CVS.

yeah, i notice that your sec/per invocation seem to monotonically
shrink. did you do *incremental* profiling across applying
the patches? (individual emails with individual before/after
stats would have been more informative or at leas less dubious).

>
>    Cu... Stefan

---
ciaoTJ