Re: rfc: alleviate GType write locking on ref/unref

[Tim Janik <timj imendio com>]

On Thu, 25 May 2006, Jindrich Makovicka wrote:

> Hello,
>
> we are currently working on a multithreaded application using GStreamer,
> and one of the most annoying performance issues we have is the lock
> contention of the GType write lock during creation and deletion of an
> instance. As the GStreamer buffers are GType instances, and most of them
> are only temporary, created at the beginning of the pipeline and
> destroyed when arriving at the end, it means these rw locks are under a
> heavy load.
>
> The attached patch attempts to relieve this lock pressure by allowing
> atomic modifications of class ref_count only with a read lock. I am
> however not sure if it doesn't fatally break things. Can someone with a
> better knowledge of GType/GObject internals comment on it?

well, at least this portion introduces breakage:

> +      if (g_atomic_int_dec_and_test(&node->data->common.ref_count))
> +        {
> +          /* we decremented 1->0 ; revert the counter for last_unref */

you can't toggle between 0 and non-0 refcounting in multi threaded
scenarios. in fact, any refcounting code that "temporarily" drops a
reference it's assuming to still be the owener off won't work in multi
threaded scenrios. e.g. think of a second thread deciding to initialize
a new class due to your invalid reference count.

> +          g_atomic_int_inc(&node->data->common.ref_count);

attempt to go 0 -> 1.

> +          G_READ_UNLOCK (&type_rw_lock);

no lock, ANYTHING can happen here, this is a multithreaded scenrio.

> +          G_WRITE_LOCK (&type_rw_lock);
> +          if (g_atomic_int_get(&node->data->common.ref_count) == 1)

and here you assume the ref_count is still 1? good luck.

> +            {
> +              if (!node->plugin)
> +                {
> +                  g_warning ("static type `%s' unreferenced too often",
> +                             NODE_NAME (node));

bailing out with a stale write lock? using g_error() instead would
be a gentler way to abort.


> +                  return;
> +                }

there are a few other issues with the patch, though mostly minor.
but before giving it a final go, i'd like to see proof that it really
improves things. 
can you come up with a test program that shows proovable performance gains
from application of this patch?
glib/tests/refcount/ and glib/tests/gobject/ contain numerous examples of
multithreaded ref counting tests and test object/class implementations to
get you started.

> Best regards,
> --
> Jindrich Makovicka

---
ciaoTJ