Created attachment 189029 [details] [review]
gdbus: Avoid busy wait loop

Review of attachment 189029 [details] [review]:

Thanks for doing this - looks good but please use spaces, not tabs. Thanks!

Oh, so I'm smoking crack - uou're already using spaces, it's just that the patch is formatted in a fancy way that led me to think otherwise. Sorry. Just go ahead and commit the patch. Thanks!

Should use a static mutex, perhaps ?

Created attachment 189033 [details] [review]
gdbus: Avoid blocking on worker thread in connection initialization

I can't see a reason to spin until the worker thread runs, so don't.
This avoids ugly sched_yield() calls that show up in strace and
annoy me; the code is cleaner now too.

We now grab the types needed for the WebKit workaround in the
thread creation area, but only release them when the thread itself
exits.

Created attachment 189035 [details] [review]
gdbus: Avoid blocking on worker thread in connection initialization

Cleaner patch; use g_once_init with gsize, avoid global static

Review of attachment 189035 [details] [review]:

::: gio/gdbusprivate.c
@@ +279,1 @@
+  release_required_types ();

This way we'll never release the required types (because shared_thread_unref() is unimplemented) which is different from before (where they were released when the thread was known to be initialized). I don't think this is a problem though.

@@ +289,3 @@
+  GError *error = NULL;
+  static gsize shared_thread_data = 0;
+  SharedThreadData *data_return;

Suggest s/data_return/ret/ since rest of GDBus consistently uses the variable name "ret" for the return value.

Rest of GDBus also avoid initializing variables when declaring them but no need to change this (I'm kinda changing my mind about this).

@@ +311,3 @@
     }
 
+  data_return = (void*) shared_thread_data;

Hmm, I don't like how you fit a pointer into the gsize - I think adding another static variable to avoid people reading the code thinking "does a pointer really fit in a gsize?" (and wasting time looking it up) is appropriate. Or maybe just add a comment about it.

Also suggest s/(void*)/(SharedThreadData *)/ since we're using @data_return in that way later on.

@@ +322,3 @@
 #if 0
+  g_assert (data != NULL);
+  if (g_atomic_int_dec_and_test (&data->refcount))

Did you try destroying the shared thread? FWIW, I never got this to work reliably with the test cases so that's why it's disabled... but that's over a year ago and I'm hazy on the details but the problem fundamentally has to do with how GDBusConnection uses the worker and how g_dbus_worker_stop() works.

I think we should probably just say "we'll never nuke the shared thread" and remove _g_dbus_shared_thread_unref() and SharedThreadData.ref_count. It will make the code easier to read and slightly smaller.

(In reply to comment #7)
>
> 
> Hmm, I don't like how you fit a pointer into the gsize - I think adding another
> static variable to avoid people reading the code thinking "does a pointer
> really fit in a gsize?" (and wasting time looking it up) is appropriate. Or
> maybe just add a comment about it.

This is pretty standard usage of g_once_init_enter; see e.g.
g_time_zone_monitor_get() and g_settings_backend_get_default(); though, both of those were written by Ryan I guess =)

> Did you try destroying the shared thread?

Nope, it's still #if 0, but I updated the code for clarity.

> I think we should probably just say "we'll never nuke the shared thread" and
> remove _g_dbus_shared_thread_unref() and SharedThreadData.ref_count. It will
> make the code easier to read and slightly smaller.

Hmm..."never" is a strong word; can we just leave the #if 0 in case someone  later wants to come along and make valgrind work?  Both of us have been motivated to work on that in the past anyways.

(In reply to comment #8)
> (In reply to comment #7)
> >
> > 
> > Hmm, I don't like how you fit a pointer into the gsize - I think adding another
> > static variable to avoid people reading the code thinking "does a pointer
> > really fit in a gsize?" (and wasting time looking it up) is appropriate. Or
> > maybe just add a comment about it.
> 
> This is pretty standard usage of g_once_init_enter; see e.g.
> g_time_zone_monitor_get() and g_settings_backend_get_default(); though, both of
> those were written by Ryan I guess =)

Sure, not saying it's wrong but a comment like "/* a gsize is guaranteed to fit a pointer */" goes a long way.

> > Did you try destroying the shared thread?
> 
> Nope, it's still #if 0, but I updated the code for clarity.
> 
> > I think we should probably just say "we'll never nuke the shared thread" and
> > remove _g_dbus_shared_thread_unref() and SharedThreadData.ref_count. It will
> > make the code easier to read and slightly smaller.
> 
> Hmm..."never" is a strong word; can we just leave the #if 0 in case someone 
> later wants to come along and make valgrind work?  Both of us have been
> motivated to work on that in the past anyways.

OK, sounds good to me.

Attachment 189035 [details] pushed as 7ed328a - gdbus: Avoid blocking on worker thread in connection initialization

*** Bug 660423 has been marked as a duplicate of this bug. ***