Re: BonoboUnknown & G_SIGNAL_TYPE_STATIC_SCOPE

[Tim Janik <timj gtk org>]

On 3 Jan 2002, Michael Meeks wrote:

> On Thu, 2002-01-03 at 02:17, Tim Janik wrote:
> > > People just have to learn to always use G_SIGNAL_TYPE_STATIC_SCOPE;
> > > doing it magically for one type doesn't help.
> 
> 	So - the consensus is that this should go; so I will accept a patch set
> for libbonoboui, libbonobo, and an LXR search to add this
> G_SIGNAL_TYPE_STATIC_SCOPE - everywhere this type is currently used.
> 
> 	Otherwise it stays.

erk, and counts as definitely buggy code ;(

> 	Owen's idea for a per Box ref count is a good one; to force a static
> scope always; but the whole thing makes me so unhappy that it's just not
> true. We should go for the suffering option of typing loads and loads [
> of course, a BONOBO_TYPE_UNKNOWN_FOR_SIGNALS would perhaps be an option
> if the API wern't frozen ... ].

hm, BONOBO_TYPE_UNKNOWN_FOR_SIGNALS would be an API addition, so not that big
of an issue.

> > the signal system is a whole different (alive!) beast, the majority of
> > data (handlers, closures, emissions, invocation hints) it uses/accesses
> > and manages is modifiable and changes frequently, which is why there's
> > one global mutex protecting it all.
> 
> 	Incidentaly; could we be using g_alloca for eg. g_signal_emit_valist's
> GValue 'free_me' list ?

hm, i doubt you'll notice that, the code reads:

#define MAX_STACK_VALUES        (16)

void
g_signal_emit_valist (gpointer instance,
                      guint    signal_id,
                      GQuark   detail,
                      va_list  var_args)
{
  GValue *instance_and_params, stack_values[MAX_STACK_VALUES], *free_me = NULL;
[...]
  if (node->n_params < MAX_STACK_VALUES)
    instance_and_params = stack_values;
  else
    {
      free_me = g_new (GValue, node->n_params + 1);
      instance_and_params = free_me;
    }

so unless you have more than 16 signal arguments, you get your values on the
stack anyway.

> also since locking is somewhat expensive - would
> it not be more expeditious to copy the param types array to a g_alloca'd
> area per emission - than do a lock / unlock pair per argument per signal
> ? NB. Threads are enabled in pretty much every Gnome app due to
> gnome-vfs.

out of curiousity, do you actually have any profiling data which exposes the
signal system lock?
but yes, reviewing that code once more, the lock in
  for (i = 0; i < node->n_params; i++)
is easily moved out of the loop.

> 	Also - I wonder at that locking, is it neccesary ? can signal's
> parameter types be changed dynamicaly ? and if so - why ? what's the

nope, that's exactly why the lock can be moved out of that loop.
i went for better-be-safe-than-sorry when adding locks to gsignal.c,
so there might be other places where locks could be optimized (probably
not in signal_emit_unlocked_R() though, which i fryed my brain over for
several times ;)

> point ? you're going to get pretty stuffed unwinding a stack frame when
> the types change underneath you - I mean if someone changes the type of
> arg2 from boolean to double as you are walking an emission with
> 'boolean' then you're pretty stuffed.
> 
> 	I wouldn't mind - but I hear rumours of slowness in the signal code
> around there.

yep, but like my initial guesses and some preliminary profiling results
showed, the majority of that is currently due to g_value_{s|g}et_*() for
every argument in every marshaller and i have strong intentions to fix
that prior to 2.0 (glib-genmarshal still needs some extensions, little
fixes and some aggresive optimizations).

> 	HTH, I look forward to a patch set from someone ultra motivated,

yep, my priority queue is along
1) settling API
2) bug squashing
3) optimizations
i'm currently working on (2), so there's been very little optimization
work going on so far (basically typedef gsize GType; was pushed because
it was an API issue to some extend).

to relieve you some, the GUI part of beast is unbearably slow currently,
so i'll definitely tackle that once the worst glib/gtk bugs are addressed.

> 
> 	Regards,
> 
> 		Michael.
> 

---
ciaoTJ