Created attachment 212613 [details] [review]
Add g_clear_pointer()

Also reimplement g_clear_object() using g_clear_pointer()

I'm not sure why g_clear_object() is implemented both as macro and function, if it's just for the casting, it could be

#define g_clear_object(op) (g_clear_object ((void *) (op)));

like some of atomic.h

Sounds a lot like bug 620263. :)

oh that was already rejected... I totally disagree with you tbh :)

I use tp_clear_pointer() literally every day, I can't implement a dispose function without it anymore. Note that my implementation here is a little bit different since it profit of your thread-safe version.

But I agree the boxed version is useful only to users of dbus-glib which constructs complex types one cannot free with a simple function.

Created attachment 212668 [details] [review]
Add g_clear_pointer()

Also reimplement g_clear_object() using g_clear_pointer()

This new patch also fix 2 issues with current g_clear_object():

1) the macro does not build with g++ because gpointer *_pp = (gpointer) object_ptr; is invalid casting (void* to void**) but gcc does not warning about it. Note that the function version still have that cast because otherwise it is the g_atomic_pointer_get() that will warn, and glib itself is build with gcc anyway.

2) a side effect of that casting was to remove warning in case of a common typo like GObject *obj; g_clear_object(obj); that will now warn with my patch thanks to static asserts in g_atomic_pointer_get().

So IMO even if you refuse g_clear_pointer() I think we should still do this little fix.

Created attachment 212669 [details] [review]
Add g_clear_pointer()

Also reimplement g_clear_object() using g_clear_pointer()

That was a small update just to add g_clear_pointer into the gtkdoc and symbols.

Just to proof it is useful:
telepathy-glib$ git grep tp_clear_pointer | wc -l
217

(In reply to comment #6)
> This new patch also fix 2 issues with current g_clear_object():
> 
> 1) the macro does not build with g++ because gpointer *_pp = (gpointer)
> object_ptr; is invalid casting (void* to void**) but gcc does not warning about
> it.

Assigning void * to any other pointer type is valid and well-defined in C but invalid in C++ (in an attempt to have better type-safety in C++), so it is correct that gcc doesn't warn and g++ does.

telepathy-glib has a regression test for this, which is compiled if we find a C++ compiler (it's our only bit of C++).

I think it'd be worth adding this to GLib, to avoid regressions when using GLib from C++ code: here are the two commits. (It hasn't been touched since.)

http://cgit.freedesktop.org/telepathy/telepathy-glib/commit/?h=6e7ea379275cf43bf376466e654c5e3e7f779c04

http://cgit.freedesktop.org/telepathy/telepathy-glib/commit/?h=1a52d83535c292ad0ec9e339f4b0777aa95dc51a

I'd probably use g_clear_pointer in my projects, FWIW.

lets reconsider this, then - my rejections are never final...

Created attachment 212779 [details] [review]
Do not detect GNUstep as Cocoa

GNUstep also installs Foundation/Foundation.h

https://bugzilla.gnome.org/show_bug.cgi?id=674172

Signed-off-by: William Hua <william@attente.ca>

Created attachment 212780 [details] [review]
Fix g_clear_object macro with C++ compilers

In C++ it is invalid to cast a void* to void**.

A good side effect of this is to also warn if *object_ptr is
not the size of a gpointer, thanks to static checks in
g_atomic_pointer_get(). This avoid common mistake of missing '&'
in g_clear_object(&obj);

Sorry about me noobing with git-bz.

So here is a commit for glib-3-32 stable branch that just fix g_clear_pointer() to work in C++ code.

Comment on attachment 212669 [details] [review]
Add g_clear_pointer()

>+ * g_clear_pointer: (skip)
>+ * @atomic: a pointer to a variable, struct member etc. holding a pointer

I'd call it "pointer", not "atomic". (Everywhere.) Likewise, I think this belongs in gmem.h, not gatomic.h

>+#define g_clear_pointer(atomic, destroy) \
>+  G_STMT_START {                                                                 \
>+    /* Only one access, please */                                                \
>+    gpointer _p;                                                                 \
>+                                                                                 \
>+    do                                                                           \
>+      _p = g_atomic_pointer_get (atomic);                                        \
>+    while G_UNLIKELY (!g_atomic_pointer_compare_and_exchange (atomic, _p, NULL));\

The "Only one access, please" comment in g_clear_object() means "only evaluate @atomic once, in case the expression has side effects". Which this code does not do. It should assign atomic to a temporary first, like g_clear_object() does.

(And why is g_clear_object() written using atomics anyway? When is that a useful behavior?)

(In reply to comment #15)
> (From update of attachment 212669 [details] [review])

> The "Only one access, please" comment in g_clear_object() means "only evaluate
> @atomic once, in case the expression has side effects". Which this code does
> not do. It should assign atomic to a temporary first, like g_clear_object()
> does.

Ah, did not understand this like that, makes sense.

> (And why is g_clear_object() written using atomics anyway? When is that a
> useful behavior?)

In the case 2 threads does g_clear_object(&obj) at the same time, it makes sure obj is unreffed only once.

(In reply to comment #16)
> In the case 2 threads does g_clear_object(&obj) at the same time, it makes sure
> obj is unreffed only once.

But that implies that the two threads are sharing a single ref. What kind of real-world code could be doing that without it being a bug?

(In reply to comment #17)
> (In reply to comment #16)
> > In the case 2 threads does g_clear_object(&obj) at the same time, it makes sure
> > obj is unreffed only once.
> 
> But that implies that the two threads are sharing a single ref. What kind of
> real-world code could be doing that without it being a bug?

I think some code bases want dispose to be thread-safe.

Created attachment 212800 [details] [review]
Add g_clear_pointer()

Also reimplement g_clear_object() using g_clear_pointer()

Voilà, moved to gmem together with g_free(), and added back the "only one access" in the macro. Added a G_STATIS_ASSERT() to still make sure the casting does not suppress important compiler warning.

Created attachment 212803 [details] [review]
Add g_clear_pointer()

Also reimplement g_clear_object() using g_clear_pointer()

Review of attachment 212803 [details] [review]:

Looks reasonable to me.  In particular in my project I *heavily* use gvariant, and the common pattern is:

 {
   GVariant *foo = NULL;
   /* code */

   ret = TRUE;
  out:
   if (foo) g_variant_unref (foo);
}

So I can now do:

out:
  g_clear_pointer (foo, g_variant_unref);

which is definitely nicer.

Merged to master, thanks.

What about fixing the C++ build warn in 2.32 as well?

Created attachment 212928 [details] [review]
Fix g_clear_object macro with C++ compilers

In C++ it is invalid to cast a void* to void**.

A good side effect of this is to also warn if *object_ptr is
not the size of a gpointer, thanks to static checks in
g_atomic_pointer_get(). This avoid common mistake of missing '&'
in g_clear_object(&obj);

Created attachment 212929 [details] [review]
Fix g_clear_object macro with C++ compilers

In C++ it is invalid to cast a void* to void**.

Also add a static check to ensure
sizeof(*object_ptr) == sizeof (gpointer).
This avoid common mistake of missing '&'
in g_clear_object(&obj);

Is it really a good idea to have every free call do a cmpxchg operation. That locks the buss and stuff which makes it scale badly with multiple core.

Its ok to use atomics when you're implementing something that is use between threads. But to use it everywhere even when it might not be needed? That seems like a recipe for performance disaster.

Otoh, this is in the unref codepath generally, and thats full of atomics use anyway...

(In reply to comment #26)
> Is it really a good idea to have every free call do a cmpxchg operation. That
> locks the buss and stuff which makes it scale badly with multiple core.
> 
> Its ok to use atomics when you're implementing something that is use between
> threads. But to use it everywhere even when it might not be needed? That seems
> like a recipe for performance disaster.

It would be good to get a definitive answer from the Telepathy people why they wanted it to be atomic in the first place.  I didn't see any rationale for it when we introduced g_clear_object().  Am I correct that it's about multiple threads entering GObject dispose handlers?

If that's the case, while we can't change g_clear_object(), it's worth noting that for the case of function-local variables, we don't need *either* the set-to-NULL or the atomic semantics.

So we could introduce g_destroy_pointer() which was just like:

void
g_destroy_pointer (gpointer a, GDestroyNotify b)
{
  if (a)
    b (a);
}

Or possibly invent a time machine, and go back and make g_object_unref() and g_variant_unref() etc. %NULL-safe...

In tp-glib they are not thread safe, it is desrt who made g_clear_object() thread safe in bug #620263, I don't know why... so I just copied its idea for g_clear_pointer().

Even for unref() we don't need atomicity or set-to-NULL (as finalize is inherently serialized). 

Both of these are required for a threadsafe dispose though, so there are places where its needed. Set-to-null is kinda cheap, but the atomic one is not, so we should imho at least document the disadvantages in using this when not needed, which kinda makes the existance of this api problematic.

(In reply to comment #28)
> If that's the case, while we can't change g_clear_object(), it's worth noting
> that for the case of function-local variables, we don't need *either* the
> set-to-NULL or the atomic semantics.

I wrote the original implementation(s), in telepathy-glib. It was inspired by Py_CLEAR_OBJECT from CPython.

The original implementation made no attempt to be thread-safe, because telepathy-glib isn't, in general, thread-safe either (we're doomed to lack of thread-safety by using dbus-glib, and in any case we aren't sufficiently pedantic about "always return a copy/ref even if it's less convenient for everyone"). I don't know why desrt rewrote it.

Py_CLEAR_OBJECT gets its "atomicity" from only being valid to call if you're holding the Global Interpreter Lock, so it doesn't need to be atomic either.

The set-to-NULL is mainly useful for dispose-like operations where you're acting on something that's directly or indirectly visible via a struct:

    tp_clear_pointer (&self->priv->map, g_hash_table_unref);

It's sometimes (particularly in regression tests) also useful as part of a "whatever just happened, get a clean slate" pattern - the same places where g_clear_error is useful:

    GHashTable *map = NULL;
    GError *error = NULL;

    if (get_a_map (&map, &error))
      {
        /* ... */
      }
    else
      {
        g_debug ("couldn't get map: %s", error->message);
      }

    tp_clear_pointer (&map, g_hash_table_unref);
    g_clear_error (&error);

    if (get_another_map (&map, &error))
      {
        /* ... */
      }
    else
      {
        g_debug ("couldn't get another map: %s", error->message);
      }

    tp_clear_pointer (&map, g_hash_table_unref);
    g_clear_error (&error);

Yes, in this simple situation it'd be just as easy to put the cleanup inside the if/else, but sometimes it's conditional or in a loop or whatever. The point is that it's instantly obvious that control passes through the "cleanup" step, so you can easily see that you're not leaking - IMO being *obviously* correct is considerably better than merely being correct.

(In reply to comment #26)
> That seems like a recipe for performance disaster.

Does anyone have numbers for this?

If people are worried, it seems to me that the conservative thing to do would be to have g_clear_pointer not guarantee thread-safety (i.e. exactly how we implemented tp_clear_pointer in Telepathy!), and upgrade it to guaranteeing thread-safety later if we decide we need it (as was done for GObject refcounts, for instance). Adding more guarantees is a compatible change, taking away guarantees is not.

IMO it'd be OK for g_clear_object() and g_clear_pointer() not to be the same, since g_clear_pointer would still be consistent with g_clear_error, and you can implement the original (non-atomic) tp_clear_object() as

    (tp|g)_clear_pointer (obj_ptr, g_object_unref);

(and indeed we do).

I would object to g_clear_pointer() not having its current "NULL-safe, and set-to-NULL" semantics - it's meant to be just like g_clear_error() (including the need to add an extra &), and that's also where its naming comes from.

The fact that the implementation moves the pointer out of the way before freeing it also avoids certain nasty conditions if dispose has side-effects. For instance, this pseudocode:

    /* connected to Thing::died */
    thing_died_cb (Container *container,
        Thing *thing)
    {
      if (container->priv->things != NULL)
        {
          g_hash_table_remove (container->priv->things, thing);
        }
    }

    thing_dispose (Thing *thing)
    {
      if (!disposed)
        {
          disposed = TRUE;
          emit ("died");
        }
    }

    container_dispose (Container *container)
    {
       /* might cause last-unref for each thing in things */
       g_clear_pointer (&container->priv->things, g_hash_table_unref);
    }

In fact, we have that exact situation in Telepathy (Container is the TpChannelManager, Channel is the TpChannel and died is really called closed) due to some historical design mistakes.

(In reply to comment #27)
> Am I correct that it's about multiple
> threads entering GObject dispose handlers?

You'd have to ask desrt - but I doubt it, since I can't see how this can arise when refcounts are atomic.

If two threads race each other into something like this (which, AIUI, is approximately GObject):

    if (g_atomic_int_dec_and_test (&thing->refcount))
      {
        dispose (thing);

        if (g_atomic_int_get (&thing->refcount) == 0)
          finalize (thing);
      }

then only one of them is going to see the refcount drop to 0 and enter the block, surely? You can get into fun problems if dispose results in the object being resurrected, but as far as I can see, the worst that can happen is recursion - for instance, in the pseudocode above, emitting Thing::died refs thing in the signal closure (back to 1 ref) then unrefs it afterwards (back to 0, causing a recursive call to dispose).

If thread A hands over a resurrected ref to thread B, then whatever mechanism you used to hand it over had better be thread-safe, which means it has a memory barrier, which means the ref in thread A and the unref in thread B are at least consistently ordered?

(In reply to comment #30)
> Both of these are required for a threadsafe dispose though, so there
> are places where its needed.

Could you name one, please?

(In reply to comment #31)
> (In reply to comment #28)
> > If that's the case, while we can't change g_clear_object(), it's worth noting
> > that for the case of function-local variables, we don't need *either* the
> > set-to-NULL or the atomic semantics.
> 
> I wrote the original implementation(s), in telepathy-glib. It was inspired by
> Py_CLEAR_OBJECT from CPython.

<snip explanation of set-to-%NULL behavior>  That makes a lot of sense, thanks Simon!

But I feel like we're still left now with the question of "Why did Ryan make g_clear_object() atomic", and the related question of "Is it worth making g_clear_pointer() atomic too?"

I think the problem here though is if we don't make g_clear_pointer() atomic, it'll massively confuse people who rely on g_clear_object()'s atomicity, and that one is already released as part of 2.32 =/

After discussion with desrt, I've pushed the fix for C++ build warning in glib-2-32 branch, and also tweaked the static checks to also detect struct {gpointer x;} a; g_clear_object(&a); it won't detect gsize i; g_clear_object(&i); but it seems there is no way it can be done. Notably (void) 0 ? (void) **(object_ptr) : 0; won't work for GVariant where the struct is private.

Also ported the same checks into g_clear_pointer() in master.

(In reply to comment #32)
> > Am I correct that it's about multiple
> > threads entering GObject dispose handlers?
> 
> You'd have to ask desrt - but I doubt it, since I can't see how this can arise
> when refcounts are atomic.

if you call g_object_run_dispose() manually

About atomics, since g_hash_table_unref() and all _unref are also made of atomics the argument stands only for things like g_clear_pointer (&str, g_free); right? That already limits a lot the scope of the problem.

In general, I agree with Simon, if in doubt better to not guarantee something since that can be added later in API-stable way.

I would still like desrt to comment why he made g_clear_object() threadsafe in the first place.

1) It's not very slow and there are plans to make it faster (bug #667191)

2) g_object_unref() already involves several atomic operations (ie: on the refcount) so we've already crossed that particular bridge in terms of performance.


I think there is legitimate room for discussing making GObject less threadsafe (ie: non-atomic refcounting) but until we do that, g_clear_object() should probably be equally safe.

Created attachment 213170 [details]
Test program

Here is a test program, it compares time with and without g_clear_pointer to destroy 1000000 object/hash/string in 1-4 threads.

Created attachment 213173 [details]
Test program results on my Core2 Duo CPU with 64bits system

The difference with and without g_clear_pointer() is measurable, but is it really significant?

Btw, I'm not sure it's the right way the measure time... Suggestions to improve are welcome :)

(In reply to comment #35)
> if you call g_object_run_dispose() manually

Ah, good point. Is this something that's generally expected to work? I thought GObject mostly followed the default GLib thread model, in which concurrent access to globals is thread-safe unless documented to be unsafe, but concurrent access to one object is thread-unsafe unless documented to be safe.

(In reply to comment #37)
> 1) It's not very slow and there are plans to make it faster

I modified Xavier's benchmark a bit to use times() for real, user and system time, do 10 times as many iterations, and display relative times for clear_pointer as percentages (< 100% = sped up, > 100% = slowed down). Running on a mostly idle dual-core laptop (Lenovo X200s, Core2 Duo L9400 @ 1.86GHz), so the 3- and 4-core cases have more threads than CPUs.

GObject:

threads real user sys
  1     103% 103% 100%
  2      98%  102% 71%
  3     104% 102% 105%
  4     103% 104%  84%

GHashTable:

threads real user sys
  1     107% 107% none
  2     104% 107%   0%
  3     108% 108% 100%
  4     108% 108% 100%

string:

threads real user sys
  1     125% 124% none
  2     123% 122% none
  3     129% 125% none
  4     121% 125% 50%

For GObject and GHashTable we take no more than 10% longer, presumably because the unref operation already caused a cache flush. For strings (the g_free case) we do take ~25% longer, though.

Created attachment 213191 [details]
revised test program

Created attachment 213193 [details] [review]
Return g_clear_object() to being  independently-implemented

---

If people consider g_clear_pointer() to be too CPU-expensive, this is step 1.

This returns g_clear_object() to its glib-2-32 branch implementation.

Created attachment 213194 [details] [review]
Make g_clear_pointer not be thread-safe

There doesn't seem to be consensus whether it should be thread-safe, so
the conservative course of action is for it not to be - we can always
add more guarantees later.

---

... and this is step 2.

Review of attachment 213193 [details] [review]:

You also need to modify the implementation in gobject.c

I've noticed that g_clear_pointer() on master has some issues with non-GDestroyNotify callbacks that you cast through GDestroyNotify for compatibility.  GCC is being an arse about intentionally aborting the program in this case...

(In reply to comment #45)
> I've noticed that g_clear_pointer() on master has some issues with
> non-GDestroyNotify callbacks that you cast through GDestroyNotify for
> compatibility.  GCC is being an arse about intentionally aborting the program
> in this case...

If the callback has a non-void return value, a non-pointer argument, multiple arguments, or varargs, then gcc is totally within its rights to complain. If not, then gcc is being pointlessly anal (perhaps because you're explicitly asking it to be?), because we already know that glib only works on platforms where all functions-that-take-one-pointer-and-return-void use the same calling convention.

Specifically, this code:

  ((GDestroyNotify) g_variant_unref) (value);

gives this warning:

  warning: function called through a non-compatible type [enabled by default]
  note: if this code is reached, the program will abort

and, indeed, GCC outputs this code:

  4004b8:	0f 0b                	ud2    

which is documented thus:


  Logical Form: UD2
  Description: Caused #UD exception 
  Note : This instruction caused #UD. Intel guaranteed that in future
         Intel's CPUs this instruction will caused #UD.

and indeed, does:

  Illegal instruction

Created attachment 213822 [details] [review]
g_clear_pointer: work around gcc helpfulness

gcc gets upset when we do "((GDestroyNotify) destroy) (_p)" because
it's non-portable. But we don't care; we already know glib wouldn't
work on any platform where different pointer types have different
calling conventions. So tweak the code to avoid the warning.

----

Tested, but not against any actual failing modules, since I didn't
know what modules were failing...

Review of attachment 213822 [details] [review]:

Good in principle but...

::: glib/gmem.h
@@ +105,3 @@
     gpointer _p;                                                               \
+    /* This assignment is needed to avoid a gcc warning */                     \
+    GDestroyNotify _destroy = (destroy);                                       \

Isn't this going to warn too? For instance in the case Ryan noted:

    g_clear_pointer (&variant, g_variant_unref);

this line expands to:

    GDestroyNotify _destroy = (g_variant_unref);

which needs a cast, because void (*) (void *) is not compatible with void (*) (GVariant *).

(In reply to comment #49)
> Isn't this going to warn too? For instance in the case Ryan noted:
> 
>     g_clear_pointer (&variant, g_variant_unref);

Oh... I was thinking you were supposed to say:

      g_clear_pointer (&variant, (GDestroyNotify) g_variant_unref);

since that's what the non-macro version would require.

The purpose of the macro is to be able to do:

    g_clear_pointer (&variant, g_variant_unref);

instead of the rather unwieldy

    g_clear_pointer ((gpointer **) &variant, (GDestroyNotify) g_variant_unref);

Created attachment 213867 [details] [review]
g_clear_pointer: work around gcc helpfulness

gcc gets upset when we do "((GDestroyNotify) destroy) (_p)" because
it's non-portable. But we don't care; we already know glib wouldn't
work on any platform where different pointer types have different
calling conventions. So tweak the code to avoid the warning.

Review of attachment 213867 [details] [review]:

ok

Comment on attachment 213867 [details] [review]
g_clear_pointer: work around gcc helpfulness

Attachment 213867 [details] pushed as 0ecbb0a - g_clear_pointer: work around gcc helpfulness

So, Can we close this now?