GNOME Bugzilla – Bug 660739
kill off g_{mutex,cond}_{new,free}()
Last modified: 2011-10-05 22:13:45 UTC
We currently have no fewer than 3 different lifecycles for each of GMutex and GCond: - static allocation - dynamic "embedded" allocation with _init() and _clear() - dynamic "malloc" allocation with _new() and _free() The last one is the one that has existed the longest, and also the one that I want to get rid of (read: deprecate). First, having 3 different lifecycles is confusing. The docs for GMutex actually spend some time talking about how g_mutex_new() seems nice but can be problematic if used in naïve ways. It's the least performant of the available options. It's also somewhat silly when you consider that GMutex is implemented with malloc() on some platforms -- so you have a pointer to a pointer to the actual lock. The only reason I can think of for keeping g_mutex_new() is that people may have GMutex* in exposed structures. Those people would still be able to g_slice_new() or g_new() a GMutex for themselves and initialise it. For GCond I can think of one more reason: it's not uncommon to have a NULL GCond* that is set non-null (via g_cond_new()) the first time it is used. That's sort of nice, but it's always just an optimisation (and possibly a misguided one, since the actual condition variable implementation probably does next-to-nothing when nobody is waiting anyway -- so you're just doubling the effort).
Created attachment 198176 [details] [review] GDBus: switch to struct-embedded GMutex and GCond Now that we have those, we should use them.
Created attachment 198177 [details] [review] GCancellable: use GCond and GMutex directly Use a statically-allocated GCond and directly use GMutex instead of making use of the undocumented G_LOCK_NAME() macro.
Created attachment 198178 [details] [review] GIO: switch a couple more GMutex users to _init() Move a couple more GIO users off of _new()/_free() to _init()/_clear().
Created attachment 198179 [details] [review] GMain, ThreadPool: embed GCond in struct Use an embedded GCond and g_cond_init()/clear() instead of a pointer with g_cond_new() and _free().
Created attachment 198181 [details] [review] GThreadedResolver: port to embedded GMutex/GCond This is the only case that was non trivial to port, due to some of the logic being based on checking the GCond* for being non-%NULL.
Review of attachment 198176 [details] [review]: Looks good
Review of attachment 198177 [details] [review]: Looks good
Review of attachment 198178 [details] [review]: ::: gio/gioscheduler.c @@ +348,3 @@ g_source_unref (source); + g_cond_wait (&proxy->ack_condition, &proxy->ack_lock); This looks like a cond_wait without a loop ?
Review of attachment 198179 [details] [review]: Looks good
Review of attachment 198181 [details] [review]: Should probably get danw to review this
Comment on attachment 198181 [details] [review] GThreadedResolver: port to embedded GMutex/GCond yeah, this looks right
Created attachment 198218 [details] [review] GDBus codegen: generate code with embedded GMutex Modify GDBus code generator to emit code that uses GMutex embedded into the structure of the skeleton instead of a pointer.
(In reply to comment #1) > Created an attachment (id=198176) [details] [review] > GDBus: switch to struct-embedded GMutex and GCond > > Now that we have those, we should use them. Looks good to me - assuming that there are no compile-warnings or test suite failures. Thanks.
Review of attachment 198218 [details] [review]: Looks good to me - assuming that there are no compile warnings or test suite failures (you may need to manually remove generated C code). Thanks.
Created attachment 198225 [details] [review] Fix an invalid non-looping use of GCond The GIOScheduler was using a GCond in a way that didn't deal with the possibility of spurious wakeups. Add an explicit predicate and a loop. Problem caught by Matthias Clasen.
Attachment 198176 [details] pushed as 5f48e2c - GDBus: switch to struct-embedded GMutex and GCond Attachment 198177 [details] pushed as 19cd57d - GCancellable: use GCond and GMutex directly Attachment 198179 [details] pushed as 518feb4 - GMain, ThreadPool: embed GCond in struct Attachment 198181 [details] pushed as 8bcdabf - GThreadedResolver: port to embedded GMutex/GCond Attachment 198218 [details] pushed as 7d4dea7 - GDBus codegen: generate code with embedded GMutex Pushing all of the accepted-commit_now patches. Please see the separate patch (just attached) to resolve the GIOScheduler condition variable issue.
Review of attachment 198225 [details] [review]: Looks good, thanks
Attachment 198178 [details] pushed as c474cd7 - GIO: switch a couple more GMutex users to _init() Attachment 198225 [details] pushed as 449a1e8 - Fix an invalid non-looping use of GCond
Created attachment 198269 [details] [review] Remove g_mutex_new()/g_cond_new() in testcases These were the last users of the dynamic allocation API.
Created attachment 198270 [details] [review] Deprecate g_{mutex,cond}_{new,free}() Now that we have _init() and _clear(), these old calls are no longer useful.
Review of attachment 198269 [details] [review]: ::: gio/tests/tls-interaction.c @@ +393,2 @@ typedef struct { + GMutex loop_mutex; This type of change looks good to me. ::: glib/tests/mutex.c @@ -59,3 @@ - g_mutex_lock (mutex); - g_mutex_unlock (mutex); - g_mutex_free (mutex); ...but I don't think we should remove tests that are *explicitly* testing deprecated functions like g_mutex_new(). Deprecated functions still need to continue to work. So if the tests are currently built with G_DISABLE_DEPRECATED I think that's wrong.
Review of attachment 198270 [details] [review]: ::: glib/deprecated/gthread-deprecated.c @@ +1414,3 @@ + * Allocates and initializes a new #GMutex. + * + * Returns: a newly allocated #GMutex. Use g_mutex_free() to free Should probably add a: Deprecated: 2.32: Statically 0-initialize a mutex, and just call e.g. g_mutex_lock().
Created attachment 198271 [details] [review] Remove g_mutex_new()/g_cond_new() in testcases These were the last users of the dynamic allocation API. Keep the uses in glib/tests/mutex.c since this is actually meant to test the API (which has to continue working, even if it is deprecated).
Review of attachment 198269 [details] [review]: ::: glib/tests/mutex.c @@ +52,2 @@ test_mutex3 (void) { Hmm, should we not still test deprecated API ? Or are you going to remove new/free outright, without deprecation ?!
Review of attachment 198271 [details] [review]: Hmm, weird. While I was writing a review comment about keeping the tests, a new patch showed up that keeps the tests...
Created attachment 198272 [details] [review] Deprecate g_{mutex,cond}_{new,free}() Now that we have _init() and _clear(), these old calls are no longer useful.
Comment on attachment 198271 [details] [review] Remove g_mutex_new()/g_cond_new() in testcases oops. marked the wrong patch obsolete.
Attachment 198272 [details] pushed as 69c0b44 - Deprecate g_{mutex,cond}_{new,free}()