Created attachment 222622 [details] [review]
add compare-and-exchange for datalist

Created attachment 222623 [details] [review]
drop destroy-notify-bearing variants

Created attachment 222624 [details] [review]
add compare-and-exchange for object data

Still missing: tests

looks cool, and definitely useful.

I'd do s/exchange/replace/ for the GObject functions, and keep it similar to the equivalent API in GHashTable, though; exchange may map more closely to the g_atomic_* API, but it feels a bit out of place in a high-level module.

Sure, why not.

Created attachment 222626 [details] [review]
add compare-and-exchange for data lists

Created attachment 222627 [details] [review]
add compare-and-exchange for object data

Review of attachment 222626 [details] [review]:

::: glib/gdataset.c
@@ +875,3 @@
+                             GQuark     key_id,
+                             gpointer   oldval,
+                             gpointer   newval)

Why not allowing destroy_func to be specified for newval?

@@ +907,3 @@
+                     if (data != data_end)
+                       *data = *data_end;
+                     d->len--;

This never frees the datalist when it goes empty.

Review of attachment 222626 [details] [review]:

::: glib/gdataset.c
@@ +860,3 @@
+ * @oldval with @newval.
+ *
+ * Note that the destroy notify for @oldval is not called.

Why not call destroy notify?

Review of attachment 222627 [details] [review]:

::: gobject/gobject.c
@@ +3278,3 @@
+ * @newval.
+ *
+ * Note that the destroy notify for @oldval is not called.

Why not, and why not destroy_notify for newval?

The first patch I attached had a destroy notify for newval in it.
We can do it, I'm just not sure it is very useful when you're doing compare-and-exchange, because you're always returning the old value - so you kinda can't destroy it. Or can you ?

<mclasen> alex: the issue is that we're returning the old value, which kinda prevents us from destroying it first, doesn't it ?
<alex> not really
<alex> all you need to do is compare the pointer
<alex> you should never deref the return value
* thaytan has quit (Ping timeout: 600 seconds)
<alex> I guess one corner case is newptr == oldptr
<mclasen> but then, can you ever deref the value returned by get_data ?
<mclasen> if the one isn't safe, the other isn't either...
<alex> mclasen: You can, if you control all the uses of that particular key
<mclasen> not like we're handing out new references here
<alex> Hmm, true, in a multithreaded app accessing a particular key
<alex> and you got a copy of that
<alex> how do you know if its safe to access it?
<alex> You ref it, but thats racy
<mclasen> you basically can only safely access it after you've swapped it out for some other value
<alex> Well, you could pass in a ref/dup to a locked get_data
<alex> g_object_dup_data (object, key, g_strdup)
<alex> kinda like that
<mclasen> yes
<alex> Without that any kind of cmpxchg will be racy i think
<mclasen> trjue
<mclasen> I guess I'll go back to behdad and see what use he had in mind for this

Basically, any usage of g_object_get_data() in combination with another thread updating the data is racy as you can't control the lifecycle of the data returned from g_object_get_data(). I.e. it might be freed when control reaches the code that called get_data.

The only safe usage is:
a) Only store ints-as-pointers in the qdata
b) Have refcounted/boxed things in the qdata and dup/ref inside the datalist lock.

So, you want something like:

gpointer    g_object_dup_data                 (GObject        *object,
					       const gchar    *key,
                                               GBoxedCopyFunc dup_func);
gpointer g_object_replace_qdata (GObject *object,
 			        GQuark quark,
				gpointer oldval,
                                GBoxedCopyFunc dup_func,
				gpointer newval,
                                GDestroyNotify newval_destroy);
^^ This dups oldval if replaced and dup_func != NULL, otherwise it
may return a destroyed value (by the previously set oldval) and you 
can only use it to compare the old pointer value to newval.

My use-case is a bit different.  To make get_data thread-safe, I want to make sure that as soon as I add a qdata, it's never overwritten.  Hence the cmpexch.  I don't need it to return the old key, just a boolean, but that doesn't make any difference.  My usage pattern will always use NULL for old key, so maybe, if people are comfortable, a version of set_qdata that doesn't overwrite (called insert for example) would do also.

Here's my pattern:

SomeStruct *
get_some_struct (Object *obj)
{
  SomeStruct *some;
retry:
  some = (SomeStruct *) g_object_get_qdata (obj, quark);
  if (G_UNLIKELY (!some)) {
    some = create_some (obj);
    if (!g_object_replace_qdata_full (obj, quark, NULL, some, destroy_some)) {
      destroy_some (some);
      goto retry:
    }
  }
  return some;
}

Another pattern I use atomic pointer cmpexch in HarfBuzz for is caching a list of objects.  As long as the items from the list are kept live indefinitely, you wouldn't have the racy getter problem.  And an atomic cmpexch can be used to safely prepend to the list.

Same pattern can be useful with qdata.  But now I see that running destroy_notify would get in the way.  So perhaps what Matthias has is exactly what I'm asking for.

if they are kept live indefinitively, you can just set destroy = NULL and avoid the issue either way, I guess ?

Well, by "indefinitely" I really meant till the parent object is destroyed...  So destroy = NULL wouldn't work.

I think the proposal in comment #14 is more generically useful, and will also allow what behdad wants.

(In reply to comment #19)
> I think the proposal in comment #14 is more generically useful, and will also
> allow what behdad wants.

Yes it does.

While there probably worth also fixing the weak_pointer stuff in the same way.

The only uncertainty is what is the dup function allowed to do with respect to the object.  Ie. the object has some lock held, so perhaps any qdata operations will be undefined, but probably need to allow wekref manipulations for the least.

Yeah, there is always risks when calling out with a core lock held, although dup is generally safer than destroy.

If we make the lock recursive we risk the dup callback actually triggering the destroy notify of the qdata (although that is quite unlikely), but it would allow setting other qdatas (including weak refs). However, I hate recursive locks. They're almost always a sign of design problems (as well as being slower).

Created attachment 222703 [details] [review]
implementation of alex' api

Created attachment 222704 [details] [review]
implementation of alex' api, for object data

Created attachment 222705 [details] [review]
tests

Created attachment 222706 [details] [review]
paperwork

Review of attachment 222703 [details] [review]:

::: glib/gdataset.c
@@ +822,3 @@
+ * take a reference on a ref-counted object.
+ *
+ * Note that @dup_func is called while the datalist is locked.

Should also mention that this means the dup_function is not allowed to read or
modify the datalist.

@@ +825,3 @@
+ *
+ * This function can be useful to avoid races when multiple
+ * threads are using the same datalist.

"the same datalist key"

@@ +894,3 @@
+ * to create the return value. Otherwise, the pointer to the
+ * member is returned as-is. If the old member is replaced and
+ * had a destroy notify associated with it, it will be called.

Hmm, in typical use you don't want to dup the old member if it was not replaced. That just means you have to destroy it before you try again.

@@ +897,3 @@
+ *
+ * Unless a @dup_func is provided, it is not safe to dereference
+ * the pointer that is returned by this function.

... if the key is used from multiple threads.

@@ +949,3 @@
+                     if (data != data_end)
+                       *data = *data_end;
+                     d->len--;

This should free the datalist if len ends up at 0, or we'll never free the datalist.

@@ +990,3 @@
+
+  if (val != NULL && old_destroy)
+    old_destroy (val);

The destroy method should be called outside the datalist lock, same as g_data_set.

Review of attachment 222704 [details] [review]:

::: gobject/gobject.c
@@ +3124,3 @@
+ * Note that @dup_func is called while user data of @object
+ * is locked.
+ *

same docs issues as in the datalist part.

::: gobject/gobject.h
@@ +496,3 @@
+gpointer    g_object_dup_qdata                (GObject        *object,
+                                               GQuark          quark,
+                                               GBoxedCopyFunc dup_func);

Maybe you should use the new GDuplicateFunc here too?

Hmm, I'm trying to implement g_object_weak_ref using this, and there are some issues:

In a read-update-check-if-retry cycle i need to use dup_data to safely reference the data. However, the check-if-retry part needs to know the non-duped value in order to compare with the existing one. So, g_datalist_id_dup_data needs to also
return the non-duped value, or take a user_data so letting the dup callback store it somewhere.

When i replace the WeakRefStack object in the atomic replace it will be destroyed. I do want it to be g_freed, but I don't want the weak ref callbacks
to be called. So, either I need the dup function to neuter the WeakStack so the
callbacks are not run, or i need to avoid the automatic destruction of the replaced value.

A version that has dup neuter the WeakStack means dup must *only* be run on an
actual replace. However, I'm not sure such a neutering is always doable, so it seems more correct to pass total ownership of the replaced data to the caller, to do with as he chooses. I'll have a go at that.

Created attachment 222734 [details] [review]
Add compare-and exchange for data lists

Also, make it possible to get a 'new ref' on a datalist member
in a race-free way.
This is useful when using object data in thread-safe libraries.

Created attachment 222735 [details] [review]
Add an atomic compare-and-exchange operation for object data

This is useful when using object data in thread-safe libraries.

Created attachment 222736 [details] [review]
Add some tests for new object data api

These are non-threaded, but the do test dup and destroy somewhat.

Created attachment 222737 [details] [review]
Add new api to symbol lists and docs

Created attachment 222738 [details] [review]
Remove global lock in g_object_weak_ref/unref

We use the new atomic gobject data replace functions instead.

Here is a new approach, which includes a new global-lock-free version of g_object_ref/unref. Note that while we avoid a lock we new instead have to copy the whole array of weak-refs on each ref/unref. This might be slightly slower, although it is a local problem rather than the global contention of the weak_ref mutex.

Review of attachment 222734 [details] [review]:

Interesting idea - you don't need an out param for the old value, because you only replace it if it is the same as the passed-in oldval - so the caller already has it. Might be good to spell that out explicitly in the docs.

Thanks Alex.  I like the API.  Will give it a test drive soon.

On irc, we decided that the replace / dup api looks good and safe, and we'll merge it after Behdad has ported pango to use it (for 2.34). The weak_ref refactoring can be handled separately, after careful review.

I've pushed these patches as wip/threadsafe-qdata now.

Created attachment 223145 [details] [review]
port pango qdata caches to the new api

And here's a patch to port pango over. I haven't touched the 'warn once' uses of qdata, I don't think there's any danger there.

Of course, threaded pango still crashes in fontconfig right away, so not sure how to test it. I did write a threaded test for g_object_replace_data, though and put it on the glib branch.

My current plan is to merge the qdata api (without the weak ref port) for Monday's release.

Thanks Matthias.  I'll spend a couple hours to see if I can get a threadsafe-ish pango going on.  Can you merge the glib patches so I can commit the pango stuff too?

pushed. Retitling this bug for whats left in the wip/threadsafe-qdata branch

-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/glib/issues/599.