What we might want to do is to encapsulate the operation as:

 static GType type;

 return g_type_register_once (...., &type);

Though that does involve another function call, possibly with
a lot of arguments. To make it perfectly portable, it would
also have to be more along the lines of:

 static GType type;
 static pthread_once_t once = PTHREAD_ONCE_INIT;
 return g_type_register_once (...., &type, &once);

Created attachment 21074 [details] [review]
like this ?

a new registration function shouldn't take once data, but simply be a
variant of g_type_register_static() that takes a GType*type_p as first
argument. internally, we can use expensive locking/blocking in case
*type_p==0 if type assignment occours atomic (i.e. once we have atomic
ints).

Created attachment 35045 [details] [review]
Makes GObject _get_type() functions thread-safe

In the meanwhile GOnce has been added, this patch uses it.

g_type_register_once() is too specialized, it won't help when some extra
processing (such as adding interfaces) is needed after registering a type.

Instead, I suggest to make G_DEFINE_TYPE_EXTENDED thread-safe (using a GOnce).

G_DEFINE_TYPE is just convenience functionality... it has to be based
on public functionality. Probably the right way to handle interfaces
and prerequisites is to do something along the lines of:

GType
foo_get_type
{
 static GType type;
 static pthread_once_t once = PTHREAD_ONCE_INIT;

 return g_type_register_once (real_init_func, &type, &once);
}

Tim: GOnce is meant to encapsulate system dependencies on 
memory coherency/read/write barriers, etc. To do without, we
have to do one of:

 a) Use double-checked locking, accept that it is questionable but works
    a lot of places
 b) Use double-checked locking, pretend that people will notice when
    it doesn't work and file bug reports so we can work around 
    on particular architectures.
 c) *always* lock on every call to get_type()

I don't much like any of those.

Created attachment 38486 [details] [review]
the patch

I like the approach taken by the second patch (not add new GType api, let
users use GOnce themselves). My patch extends that and changes G_DEFINE_TYPE
to also create threadsafe get_type functions

Another thing to keep in mind is that these get_type functions are not really
G_GNUC_CONST, because they access global memory. G_GNUC_PURE functions can
access global memory, but can't change it, so they're not pure either.

Calling the get_type functions from the main thread before spawning off other
threads has been a common technique for getting around threadsafety, but a smart
compiler will omit the function call completely if the return value isn't used,
which can then cause problems when threads actually call a get_type for its value.

Regarding the performance effects of CONST declarations, a hugely serious
scientifical test: A simple startup of gedit shows (startlingly) about 30000
gtk_toolbar_get_type() calls, with kcachegrind showing a 'relative cost' of the
function at 0.05. If I redefine gtk_toolbar_get_type without G_GNUC_CONST in
gtktoolbar.h, I get the same results. FWIW.

How'd you test this? gedit doesn't call any toolbar code directly, lower
libraries in the stack do that work. Did you build the whole stack with the
gtktoolbar.h change?

It may be worth proposing to the gcc people a call_once attribute or somesuch.

I ran it with valgrind-callgrind, and looked at it with kcachegrind. The calls
are all coming from inside gtk. You can run it yourself if you like: valgrind
--skin=callgrind gedit, then kcachegrind callgrind.out.PID.

Also, the number of times toolbar_get_type was called was the same on both tries
to within one call, meaning the proc was always called for for its value, and
the compiler never decided to cache the return in a register or some such.
Normal CFLAGS (-g, -O2).

I would not worry too much about optimizing get_type calls. First, they're
called most when #ifndef G_DISABLE_CHECKS, which adds more overhead because of
the runtime type checking, which is more expensive than get_type. Second the
cost is low: again with these kcachegrind pseudobenchmarks,
g_type_check_instance_is_a takes about 1.2% of startup time (180 000 calls),
while get_type functions take about 0.2% of startup time (somewhere around 100
000 calls). That's not why gedit is slow to start up ;-)

You didn't answer my question. The question was did you recompile gedit and all
the dependent libs down to gtk+ with the gtktoolbar.h change, or just gedit
itself. Cause if it's just gedit (or even just gedit and gtk+) you probably
won't get a change in the number of gtk_toolbar_get_type calls.

Only Gtk.

The get_type functions are intentionally marked as const to allow the compiler
to optimize them away. If you want to force the call, declare the variable
receiving the result as volatile.

Right. But (1) They aren't const, in the objective sense of the term, because
they call non-const functions (i.e. g_type_register_static), and (2) I don't
think it makes any difference at all speed-wise.

*** Bug 300659 has been marked as a duplicate of this bug. ***

I think that g_object_new() should be passed in the pointer to the _get_type() function and should call it 
whenever it decides, g_object_new() will do the right job.
Data types won't have to bother about this.

My latest patch doesn't add api.

*** Bug 317246 has been marked as a duplicate of this bug. ***

the last patch does not seem to fit.

1. Trial without API change
since initialization of an object looks like this :

GType
test_object_get_type (void)
{
  static GType test_object_type = 0;

  if (!test_object_type)
    {
      static const GTypeInfo test_object_info =
      {
	sizeof (TestObjectClass),
	NULL,           /* base_init */
	NULL,           /* base_finalize */
	(GClassInitFunc) test_object_class_init,
	NULL,           /* class_finalize */
	NULL,           /* class_data */
	sizeof (TestObject),
	5,              /* n_preallocs */
	(GInstanceInitFunc) test_object_init,
      };
      GInterfaceInfo iface_info = { test_object_test_iface_init, NULL, GUINT_TO_POINTER (42) };

      test_object_type = g_type_register_static (G_TYPE_OBJECT, "TestObject", &test_object_info, 0);
      g_type_add_interface_static (test_object_type, TEST_TYPE_IFACE, &iface_info);
    }

  return test_object_type;
}

(this is extracted from the example in glib)

to avoid API change to all derived gobject, we need to operate into g_type_register_static().
g_type_register_static() will have to check whether the given (GTypeInfo *) is already registered and if this is the case, g_type_register_static() will have to return immediately with the identifier of the object.
A big lock has to be placed in g_type_register_static() but since this is supposed to be called only at initialization phase, this should not be a problem.
A hash table is needed to make it correspond an identifier for a given (GTypeInfo *).

g_type_add_interface_static() will also have to check if the interface is already registered for a given (GInterfaceInfo *).

Though, it will not work for the example since the GInterfaceInfo is allocated on the stack ...
If the developer who defined the object forgot to declare static the GTypeInfo and GInterfaceInfo, this will not work.
Then, g_object_new API must change.

2. proposed API change

GType (* gobject_get_type_declarator)(void);

gpointer g_object_thread_safe_new (g_object_get_type_declarator decl, const gchar *first_property_name, ...);
(and friends of function g_object_thread_safe_new() will also have the same kind of interface)

then, a g_once() of the given gobject_get_type_declarator() will be called in g_object_thread_safe_new().

That will not break the old objects, but will introduce thread-safety for the objects that need it.

I am not sure I understand what you are saying here.

> _type_register_static() will have to check whether the given (GTypeInfo *) 
> is already registered and if this is the case, g_type_register_static() will
> have to return immediately with the identifier of the object.

The get_type functions already try to do this; although not in a 
threadsafe way. The GOnce patch will make it threadsafe and remove
the issue.

> Though, it will not work for the example since the GInterfaceInfo is allocated
> on the stack ...
> If the developer who defined the object forgot to declare static the GTypeInfo
> and GInterfaceInfo, this will not work.
> Then, g_object_new API must change.

Created attachment 65981 [details] [review]
Idempotentize type registration functions

Patch for Owen Taylor's original 2nd option

Created attachment 65982 [details]
Test program

Program to test idempotentize patch

The GOnce solution is way better than the idempotentize solution, but it doesn't help code that is already written the old way.

here's a thread from 2003 also complaining about the issue:
  http://mail.gnome.org/archives/gtk-list/2003-July/msg00033.html

*** Bug 161770 has been marked as a duplicate of this bug. ***

(In reply to comment #6)

> GType
> foo_get_type
> {
>  static GType type;
>  static pthread_once_t once = PTHREAD_ONCE_INIT;
> 
>  return g_type_register_once (real_init_func, &type, &once);
> }
> 
> Tim: GOnce is meant to encapsulate system dependencies on 
> memory coherency/read/write barriers, etc. To do without, we
> have to do one of:
> 
>  a) Use double-checked locking, accept that it is questionable but works
>     a lot of places
>  b) Use double-checked locking, pretend that people will notice when
>     it doesn't work and file bug reports so we can work around 
>     on particular architectures.
>  c) *always* lock on every call to get_type()
> 
> I don't much like any of those.

with the arrival of atomic operations, this is fortunately not true anymore.
i think we can join the type_id with the "once" field conveniently now with something like:

GType
foo_get_type
{
  static GType type_id = 0;
  if (g_once_init_enter (&type_id))
    {
      GType id = g_type_register_static (...);
      g_once_init_leave (&type_id, id);
    }
  return type_id;
}

with these prototypes (using gsize ensures pointer type width):

static inline gboolean  g_once_init_enter (volatile gsize *value_location);
void                    g_once_init_leave (volatile gsize *value_location,
                                           gsize           value);

note the following:
- the type registration code can stay *inline* and doesn't need a seperate
  real_init_func() to be implemented (one of my main concerns with the former
  GOnce suggestions)
- g_once_init_enter() can be implemented static inline which allows to quickly
  return FALSE if g_atomic_pointer_get (value_location) != 0.
- if g_atomic_pointer_get (value_location) == 0, g_once_init_enter() returns
  TRUE if called the first time, or blocks, until g_once_init_leave() was called
  for the same address value_location.
- g_once_init_leave() ensures via return_if_fail, that *value_location is still
  ==0. other than that, it assigns *value_location and causes any pending
  g_once_init_enter() calls blocking on the same address value_location to
  return.
- the only performance impact this approach has over the current single-threaded
  _get_type() functions is using g_atomic_pointer_get() instead of a normal
  static variable read for the common case.
- the heavy performance penalties (acquirering/releasing a mutex and blocking on
  a condition) are hidden inside g_once_init_enter/g_once_init_leave and are
  only triggered in the contention case, which is acceptable.

I'm about to change gstreamer code to use the g_once solution to fix the problem in our get_type fonctions...

But I'm hesitating since this seems to be still in discussion... should I wait before undertaking this "rewrite" (there's a lot of get_type fonctions) ? 

Matthias Clasen seemed to have an acceptable patch in March 2005 it's now October 2006 ...now there's a new suggestion as comment #28 wich looks good to me except that it adds API and thus it will introduce a glib version requirement.. ( the gonce patch would too but wouldn't be a "hard" requirement because the API would not change). 

In short I would favor the patch of Matthias Clasen but what I would really like is someone to decide so that the apps based on glib can finally stop crashing... ?

(In reply to comment #29)
> I'm about to change gstreamer code to use the g_once solution to fix the
> problem in our get_type fonctions...
> 
> But I'm hesitating since this seems to be still in discussion... should I wait
> before undertaking this "rewrite" (there's a lot of get_type fonctions) ? 
> 
> Matthias Clasen seemed to have an acceptable patch in March 2005 it's now
> October 2006 ...now there's a new suggestion as comment #28 wich looks good to
> me except that it adds API and thus it will introduce a glib version
> requirement.. ( the gonce patch would too but wouldn't be a "hard" requirement
> because the API would not change). 
> 
> In short I would favor the patch of Matthias Clasen but what I would really
> like is someone to decide so that the apps based on glib can finally stop
> crashing... ?

i intend to implement #28 as soon as i get the time (that might be in a couple weeks though) unless there are serious objections to this approach.
note that you can still implement your own GOnce or GMutex based get_type workarounds and don't need to be affected by the then-newly offered API if you are striving for an earlier solution.

ok :) thanks for the very quick reply!

I will implement the

static inline gboolean  gst_g_once_init_enter (volatile gsize *value_location);
void  g_once_init_leave (volatile gsize *value_location,  gsize value);

fonctions in gstreamer with the same api so that when they are in g_lib we can use them direclty...

Maybe my code would fit in glib too...

There's just 1 or 2 things I need to clarify :

when you say :

- if g_atomic_pointer_get (value_location) == 0, g_once_init_enter() returns
  TRUE if called the first time, or blocks, until g_once_init_leave() was
called

Won't we need a Gonce struct passed to the fonction to be able to set it a state and know if it was called the 1st time etc.. ? .. So I can 

so it would be more like :

static inline gboolean  g_once_init_enter (GOnce* once, volatile gsize *value_location);
void  g_once_init_leave (GOnce* once, volatile gsize *value_location,  gsize value);

And when it's not the first time, I would need a mutex or something to signal that leave is done should we create one and pass it to the fonction or any idea on how I would wait for this ? use the gonce_mutex ?

 

(In reply to comment #32)
> I will implement the
> 
> static inline gboolean  gst_g_once_init_enter (volatile gsize *value_location);
> void  g_once_init_leave (volatile gsize *value_location,  gsize value);
> 
> fonctions in gstreamer with the same api so that when they are in g_lib we can
> use them direclty...
> 
> Maybe my code would fit in glib too...

if you provided a patch for glib as well, that could speed up the implementation.

> when you say :
> 
> - if g_atomic_pointer_get (value_location) == 0, g_once_init_enter() returns
>   TRUE if called the first time, or blocks, until g_once_init_leave() was
> called
> 
> Won't we need a Gonce struct passed to the fonction to be able to set it a
> state and know if it was called the 1st time etc.. ? .. So I can 

no, the point is to get rid of that GOnce as well and we can do that, because the code portion is uniquely identified by the pointer to the static type_id.

> And when it's not the first time, I would need a mutex or something to signal
> that leave is done should we create one and pass it to the fonction or any idea
> on how I would wait for this ? use the gonce_mutex ?

well, one possible implementation can look like this (for the case where *value_location is still == 0):

g_once_init_enter gets a mutex, a condition and a global list. the list contains value_locations from _enter without _leave calls.
when _enter is called, it determines if value_location is in the list, if not, it adds it and returns. if it is present already, it waits in a loop on the condition until it's not in the list anymore then returns.
_leave then asserts value_location is in the list, makes sure *value_location==0 (via return_if_fail), removes it, assigns it signals condition and returns.

(In reply to comment #33)
> if you provided a patch for glib as well, that could speed up the
> implementation.

I will :)

> well, one possible implementation can look like this (for the case where
> *value_location is still == 0):
> 
> g_once_init_enter gets a mutex, a condition and a global list. the list
> contains value_locations from _enter without _leave calls.
> when _enter is called, it determines if value_location is in the list, if not,
> it adds it and returns. if it is present already, it waits in a loop on the
> condition until it's not in the list anymore then returns.
> _leave then asserts value_location is in the list, makes sure
> *value_location==0 (via return_if_fail), removes it, assigns it signals
> condition and returns.
 
Humm, I don't understand why we would prefer using a locked global list rather then a struct with a atomic read/set (GOnce struct) ... seems to me the the list (wich could get big) introduces more of a performance penality no ?

Here's how I saw it until now:


static inline gboolean  g_once_init_enter (GOnce* once, volatile gsize *value_location)
{
  if (g_atomic_pointer_get (value_location) !=0)
  {
    return FALSE;
  }
  else
  {
    if (g_atomic_get_int (once->status) == G_ONCE_STATUS_NOTCALLED)
      return TRUE;
    else
    {
      while(g_atomic_get_int (once->status) != G_ONCE_STATUS_READY)
        g_cond_wait (g_once_cond, g_once_mutex);
      
      return FALSE;
    }
  }
}

void  g_once_init_leave (GOnce* once, volatile gsize *value_location,  gsize value)
{
  g_return_if_fail (value_location == 0);

  *value_location = value;

  g_atomic_set_int (once->status, G_ONCE_STATUS_READY);
  g_cond_broadcast (g_once_cond);
  
}

We don't need any locking for the brodcast/signal since it uses atomic operations , the mutex would be used only for the signaling/blocking... but maybe another mutex might be used in other not to conflict with g_once_impl...

Seems faster then locking /parsing a table... what do you think?

(In reply to comment #34)
> (In reply to comment #33)
> > g_once_init_enter gets a mutex, a condition and a global list. the list
> > contains value_locations from _enter without _leave calls.
> > when _enter is called, it determines if value_location is in the list, if not,
> > it adds it and returns. if it is present already, it waits in a loop on the
> > condition until it's not in the list anymore then returns.
> > _leave then asserts value_location is in the list, makes sure
> > *value_location==0 (via return_if_fail), removes it, assigns it signals
> > condition and returns.
> 
> Humm, I don't understand why we would prefer using a locked global list rather
> then a struct with a atomic read/set (GOnce struct) ...

not needing GOnce in the public API is the main point in the API i outlined,
i'm afraid you have to stick to that ;-)

> seems to me the the
> list (wich could get big) introduces more of a performance penality no ?

no, the list can not get big. it only contains entries if there is any thread between _enter/_leave. and the time spend there is supposed to be short. so the list is almost always empty.

> We don't need any locking for the brodcast/signal since it uses atomic
> operations , the mutex would be used only for the signaling/blocking... but
> maybe another mutex might be used in other not to conflict with g_once_impl...
> 
> Seems faster then locking /parsing a table... what do you think?

g_once_init_enter() as outlined in my previous comments is already very fast in the common case, because of it's initial if (atomic_get() != 0) return ...;
everything that comes thereafter maybe slow because it's very seldomly triggered.

> not needing GOnce in the public API is the main point in the API i outlined,
> i'm afraid you have to stick to that ;-)

ok :)

> no, the list can not get big. it only contains entries if there is any thread
> between _enter/_leave. and the time spend there is supposed to be short. so the
> list is almost always empty.

you're right it seemed bad to me since I tought of it as an O(N) operation with a fixed table and that it introduced a possible problem with the max size of the table too..

But I guess I could do it with a hash table and it would bo O(1) with no real limitation on size .. that's how you saw it I guess ?

And why declare g_once_init_enter() static ? doesn't make sense to me since it's a public fonction of the api .. ?

Created attachment 75891 [details] [review]
patch to implement the g_once_init fonctions , THIS PATCH DOES NOT WORK 

I tryed to implement the fonctions but it doesn't seem to work (see test program below).. not sure why.. if anyone wants to review... ?

Created attachment 75892 [details] [review]
Test to check the last patch 

Run this on a SMP machine with a compile line like :

LD_LIBRARY_PATH=/opt/glib/2.12.4/lib gcc -g -O0 -I/opt/glib/2.12.4/include/glib-2.0/  -I/opt/glib/2.12.4/include/ -o test  -pthread -L /opt/glib/2.12.4/lib -lgobject-2.0 -lgthread-2.0 -lgmodule-2.0 -ldl -lglib-2.0 test.c

You will endup with a crash (maybe after a few trys ) like :

+ Trace 82069

Currently I have no idea why... comments are welcome

I found my problem.. need to recheck after the 1st lock in the enter... will send a correct patch tomorow

Created attachment 75941 [details] [review]
Implementation of g_once_init_enter and _leave 

This patch is working and should be clean... this is my final patch

Please review/comment

Tim, does this implement what you had in mind  ?

Tim are you there ?

(In reply to comment #43)
> Tim are you there ?

sorry, been really busy with other projects meanwhile.
thanks for working on the patch.

(In reply to comment #36)
> > so the list is almost always empty.
> 
> you're right it seemed bad to me since I tought of it as an O(N) operation with
> a fixed table and that it introduced a possible problem with the max size of
> the table too..
> 
> But I guess I could do it with a hash table and it would bo O(1) with no real
> limitation on size .. that's how you saw it I guess ?

yes, allthough i think using a hashtable is overkill. that's because the hashtable/list is only filled in *contention* scenarios, and lookups are performed by an otherwise idle/blocked thread anyway. so O(N) list operation is by far good enough.
the patch looks like a good start, some nit picks:
- please use diff -up for generation
- please add g_return_if_fail (value_location != NULL); etc. checks at the start of your functions
- please put the return value onto an extra line, like:
  int
  foo ()
  {}
- also, your patch can actually use g_once_mutex and g_once_cond, so you can skip your initialization code.
- you don't need to guard atomic accessors with locks. the idea is to use an atomic accessor to speed up the common case (no contention) by getting around the need to use a lock, so this:
  +  g_mutex_lock (g_once_init_mutex);
  +  if (g_atomic_pointer_get (value_location) !=0)
  +    { g_mutex_unlock (g_once_init_mutex); return FALSE; }
  should be:
  +  if (g_atomic_pointer_get (value_location) !=0)
  +    return FALSE;
  +  g_mutex_lock (g_once_init_mutex);

as an aside, while reading over your patch, i think the technique used here could more generally be useful for initializations where you don't want to tear your code apart into initialization callbacks, e.g. also

an_initialization_function (int a, double b, void *foo)
{
  static gsize initialized = 0;
  if (g_once_init_enter (&initialized))
    {
      my_initialization_code (a, b);
      setup (foo);
      g_once_init_leave (&initialized, 1);
    }
}

this just bears the question of whether gsize is the right type. i think the answer to that is yes, because you'd almost always want an integer there, but the exceptional case where you want to store a pointer is still supported via casts.

Thanks a lot for the comments :)


I will rework the patch next week ( I hope )

Created attachment 86351 [details] [review]
patch to implement the g_once_init fonctions using a slist 

This patch uses a GSlist rather then a hash table
It also uses the g_once_mutex and cond rather then it's own
Added the return type on an extra line
Removed the unnecessary lock with a atomic function (don't know why I had this)
Made with diff -up

Hope it's ok :)

(In reply to comment #46)
> Created an attachment (id=86351) [edit]
> patch to implement the g_once_init fonctions using a slist 
> 
> This patch uses a GSlist rather then a hash table
> It also uses the g_once_mutex and cond rather then it's own
> Added the return type on an extra line
> Removed the unnecessary lock with a atomic function (don't know why I had this)
> Made with diff -up
> 
> Hope it's ok :)

looks good now except for some coding style changes. and we should slightly rename g_once_init_enter_locked() because often the "_locked" postfix indicates that a function is called assuming a specific lock is acquired. enter_locking() or enter_slow() migth be alternatives...
also, we should have docs for the new functions and use it for a few existing _get_type cases.

Tue Jul 10 12:24:35 2007  Tim Janik  <timj@imendio.com>

        * glib/gthread.[hc]: implemented g_once_init_enter(),
        g_once_init_enter_impl() and g_once_init_leave(), based on a patch by
        Antoine Tremblay, fixes #65041.
        adapted exported inline function mechanism from gutils.[hc] for inlining
        g_once_init_enter_impl() in gthread.[hc].

Tue Jul 10 12:31:50 2007  Tim Janik  <timj@imendio.com>

        * gtype.h: use g_once_init_enter/g_once_init_leave to guard critical
        initialization section of *_get_type implementations in G_DEFINE_TYPE,
        bug #65041.

This breaks Konqueror's nspluginviewer and operapluginwrap.  See https://bugzilla.novell.com/show_bug.cgi?id=294385 for details.

Are you compiling with -O3 ? 
gcc seems to have started moving our const-annotated get_type() functions wildly around, which could cause that issue.

(In reply to comment #50)
> Are you compiling with -O3 ? 
> gcc seems to have started moving our const-annotated get_type() functions
> wildly around, which could cause that issue.

Nope, plain -O2 plus some extra flags (-fstack-protector -fomit-frame-pointer etc.)

Can you verify if the problem is that a get_type function is called before 
g_type_init ?

(In reply to comment #49)
> This breaks Konqueror's nspluginviewer and operapluginwrap.  See
> https://bugzilla.novell.com/show_bug.cgi?id=294385 for details.

the warning "(process:19191): GLib-GObject-CRITICAL **: gtype.c:2242: initialization assertion failed, use IA__g_type_init() prior to this function" from that bug report is due to gcc moving around GNUC_CONST functions (before gtk_init() and other initializers). get rid of that attribute or call g_type_init() *very* early on in your main as a workaround, also see bug #446565 for a proper fix.

i did find a race in the once_init code however, fixed in SVN. if you run into any new issues, please open a new bug report and attach test cases/references there. this one is long enough already and the original issues of this report has been adressed.

Tue Aug 14 02:06:10 2007  Tim Janik  <timj@imendio.com>

        * glib/gthread.c (g_once_init_enter_impl): prevent race covered
        by g_once_init_enter(), by checking for previous initializations
        before entering initialisation branch.

        * tests/onceinit.c: added multi-thread/multi-initializer stress test
        using unoptimized g_once_init_enter_impl().