Created attachment 282012 [details] [review]
Remove atomics from g_clear_object/g_clear_pointer

Practically no caller of these functions require atomic behaviour,
but the atomics are much slower than normal operations, which makes
it desirable to get rid of them. We have not done this before because
that would be a break of the ABI.

However, I recently looked into this and it seems that even if the
atomics *are* used for g_clear_* it is not ever safe to use this.  The
atomics protects two threads that are racing to free a global/shared
object from freeing the object twice. However, any *user* of the global
object have no protection from the object being freed while in use,
because there is no paired operation the reads and refs the object
as an atomic unit (nor can such an operation be implemented using
purely atomic ops).

So, since nothing could safely have used the atomic aspects of these
functions I consider it acceptable to just remove it.

Review of attachment 282012 [details] [review]:

Seems legit, but see below.

::: glib/gmem.c
@@ +218,3 @@
 
+  _p = *pp;
+  *pp = NULL;

why not make the trivial improvement of moving the assignment inside of the conditional?

::: glib/gmem.h
@@ +119,3 @@
                                                                                \
+    _p = *_pp;                                                                 \
+    *_pp = NULL;                                                               \

ditto.

Created attachment 282056 [details] [review]
Remove atomics from g_clear_object/g_clear_pointer

Practically no caller of these functions require atomic behaviour,
but the atomics are much slower than normal operations, which makes
it desirable to get rid of them. We have not done this before because
that would be a break of the ABI.

However, I recently looked into this and it seems that even if the
atomics *are* used for g_clear_* it is not ever safe to use this.  The
atomics protects two threads that are racing to free a global/shared
object from freeing the object twice. However, any *user* of the global
object have no protection from the object being freed while in use,
because there is no paired operation the reads and refs the object
as an atomic unit (nor can such an operation be implemented using
purely atomic ops).

So, since nothing could safely have used the atomic aspects of these
functions I consider it acceptable to just remove it.

Commited to master as b1dd594a22e3499caafdeccd7fa223a032b9e177

I'm unsure, can GObject::dispose() be called from 2 threads at the same time? A typical usage of g_clear_pointer/object() is to implement dispose:

dispose()
{
  g_clear_object(self->priv->foo);
}

If 2 threads runs that dispose at the same time, then the atomic was safe in that case, afaik. No?

I don't think that this could reasonably happen, by extension of the same argument.

Assuming that dispose is running in two threads at the same time, it means that you were probably trying to make meaningful use of the object from two threads at the same time in which (at least) one of them may have been disposing the object.  It stands to reason that the 'other' thread would be trying to access the object that was being cleared, at which point Alex's original argument kicks in.

yeah, makes sense, indeed.