Re: GObject threadsafety (#166020)

[Tim Janik <timj gtk org>]

On Thu, 10 Mar 2005, Matthias Clasen wrote:

> The bugs are:

> http://bugzilla.gnome.org/show_bug.cgi?id=166020
> (use GAtomic for refcounting)

hi wim.

i'm commenting in email on your patch, because i think bugzilla is an
inherently bad medium for patch review and discussion in general:

first, large portions of the patch are hard to comment on out of the box
because the affected function isn't clear. please produce diffs with
diff -up in the future.

Closure: using a lock is too slow, esp. a STATIC_LOCK from glib which routes all threads
using static locks through a single bottleneck mutex.
also, changing closures source incompatibly or in ABI is NOT an option, that includes:
 struct _GClosure
 {
   /*< private >*/       guint    ref_count : 15;
   [...]
-  /*< private >*/       GClosureNotifyData *notifiers;
+  GClosurePrivate *private;

   GClosureNotifyData *notifiers;
 } GClosurePrivate;
+struct GClosurePrivate { ... };

since it is both, source *and* ABI incompatible.
instead, use compare_and_swap to adjust the refcount, that really is not
that hard. the refcount is the first 15 bit of the first uint32 in a closure.
depending on the byte order, that may be the lower or upper 15 bit, but
that's just one ifdef required. (heck, one could even write a configure
test for those 15bit easily).

GObject (and other portions):
-  ref_count = object->ref_count;
+  ref_count = g_atomic_int_get (&object->ref_count);
   object->ref_count = 0;
   g_datalist_id_set_data (&object->qdata, quark_weak_refs, NULL);
   object->ref_count = ref_count;
this change is simply wrong. i'm inclined to say: use compare_and_swap()
instead and assert that the refcount stayed 0. but that's useful only
in a single threaded scenario. while the finalize handlers of the
datalist items shouldn't cause reference increments (which is caught by the
above =0 assignment and a return_if_fail(ref_count>0) in ref()), other
threads may very well do so, since the object is not literally finalized
yet. so i guess the best solution is to give up the extra check and leave
the refcount unaltered around destroying weak refs.

changes to assertions like the following are pointless in my eyes:
-  g_return_if_fail (object->ref_count > 0);
+  g_return_if_fail (g_atomic_int_get (&object->ref_count) > 0);
they are usefull for development platforms only which is mostly x86 which
doesn't need the extra accessors.

in some places, the readability of code degraded:
-  object->ref_count -= 1;
- 
-  if (object->ref_count == 0)	/* may have been re-referenced meanwhile */
+  /* may have been re-referenced meanwhile */
+  if (G_LIKELY (g_atomic_int_dec_and_test (&object->ref_count))) 
the code simply looks wrong since one would expect g_atomic_int_dec_and_test()
to test for !=0. but it does test for ==0, so use of the macro is highly
misleading. please use either another macro (dec_and_test0() or
!dec_and_test()) or spell it out (exchange_and_add(a,-1)==1).

in another place, temporary invalid object state got introduced:
-  if (object->ref_count > 1)
-    object->ref_count -= 1;
-  else
-    g_object_last_unref (object);
+  if (G_UNLIKELY (g_atomic_int_dec_and_test (&object->ref_count)))
+    {
+      /* inc again for performing last_unref */
+      g_atomic_int_inc (&object->ref_count);
+      g_object_last_unref (object);
+    }
causes ref_count==0 temporarily which is invalid in concurrent contexts,
which is what the patch is all about. so the code has to be changed to only
decrement the reference count if it is >1 (e.g. by using compare_and_swap),
and call last_unref otherwise.

the lock changes in gparamspec.c from mutexes to atomic are pointless 
also since using a mutex there is by no means performance critical
according to my estimates and any profiling data i have seen.

in the gsignal.c portions, the patch should not remove debugging code:
-#ifndef G_DISABLE_CHECKS
-  if (handler->ref_count >= HANDLER_MAX_REF_COUNT - 1)
-    g_error (G_STRLOC ": handler ref_count overflow, %s", REPORT_BUG);
-#endif


thanks for starting the work on making GObject reference counting thread
safe. please send a new version of the patch with the above issues fixed
to me by email and Cc: the list, so i can give it proper review in a
public discussion forum. (attaching it to bugzilla again might also be in
the interest of the other glib/gtk developers).

> Thanks,
>
> Matthias

---
ciaoTJ