GNOME Bugzilla – Bug 548967
1 bit mutex lock
Last modified: 2011-02-18 15:59:31 UTC
glib could benefit from the addition of mutex locks that require only a single bit to store the locked state. The primary use for this would be to be able to have per-instance locks on size-sensitive structures.
Created attachment 117198 [details] [review] [PATCH] Bug 548967 — add support for 1-bit mutex locks. yyyy-mm-dd Ryan Lortie <desrt@desrt.ca> * configure.in: add check for HAVE_FUTEX * glib/gthread.h: * glib/glib.symbols: * docs/reference/glib/glib-sections.txt: add g_bit_{,un}lock. * glib/gthread.c (g_thread_init, g_bit_lock, g_bit_unlock): add support for 1-bit mutex locks. Add initialiser for futex emulation mutex lock. --- configure.in | 26 ++++ docs/reference/glib/glib-sections.txt | 4 + glib/glib.symbols | 2 + glib/gthread.c | 217 +++++++++++++++++++++++++++++++++ glib/gthread.h | 5 + 5 files changed, 254 insertions(+), 0 deletions(-)
Created attachment 117199 [details] [review] [PATCH] Bug 548967 — testcase for 1-bit mutex locks. yyyy-mm-dd Ryan Lortie <desrt@desrt.ca> * configure.in: generate Makefile in gthread/tests * gthread/Makefile.am: add tests/ to SUBDIRS, DIST_SUBDIRS * gthread/tests/Makefile.am: new file * gthread/tests/1bit-mutex.c: test for 1-bit mutex locking --- configure.in | 1 + gthread/Makefile.am | 3 + gthread/tests/1bit-mutex.c | 90 ++++++++++++++++++++++++++++++++++++++++++++ gthread/tests/Makefile.am | 10 +++++ 4 files changed, 104 insertions(+), 0 deletions(-)
to save you the effort of mentioning them, i've already found two small goofs: - 'condition' should read 'contention' in the comment at the top of the test - '@bit:' should read '@lock_bit:' in the gtkdoc comments
The beauty of this is that since reference_count's are already only accessed atomically, one of their high bits can be used as a lock with absolutely zero overhead.
Review of attachment 117198 [details] [review]: Patch looks beautiful. I suggest: - Move the new code into a separate file since it's a totally separate construct. - Add g_bit_try_lock()? - Perhaps do another patch adding g_object_lock()/unlock()/try_lock() to demonstrate a usecase for this? ::: glib/gthread.c @@ +1188,3 @@ + goto retry; + + if (g_bit_lock_contended[class]) Shouldn't you do an atomic access here? I think I understand why you aren't. But perhaps it's worth a comment?
I fully concur with Behdads review.
Created attachment 152497 [details] [review] Bug 548967 - 1 bit mutex lock: add tests Add a test case for the new API.
Created attachment 152498 [details] [review] Bug 548967 - 1 bit mutex lock Add support for a mutex lock that consumes only one bit of storage inside of an integer on systems that support futexes. Futex is emulated (at a higher cost) on systems that don't have it -- but only in the contended case.
Behdad has happily agreed to writing the g_object_lock() patch.
Comment on attachment 152497 [details] [review] Bug 548967 - 1 bit mutex lock: add tests Still has the condition vs contention typo you pointed out yourself, it seems...
I guess making the test cover the fallback code for !futex systems would be tricky...
fwiw, i compiled with HAVE_FUTEX disabled and tested. good thing i did, too, because it caught a pair of missing #include<>s that were needed for the futex emulation. i could possibly make a test case that #includes the gbitlock.c file directly and uses the functions defined there, with HAVE_FUTEX disabled. i'm afraid the galias stuff would mess it all up, though...
Created attachment 152517 [details] [review] g_bit_lock: always check emulated futex(2) Always check the emulated futex(2) implementation, even on systems with futex support.
it's ugly, but it works. comments?
I'd like to point out that g_object_lock() can be done without any refcounting hacks on 64bit arches since there is a hole in the GObject structure on 64bit: http://blogs.gnome.org/alexl/2009/10/12/gobject-performance-on-64bit-arches/
I created bug 608695.
Ryan, should the emulation use a hash table instead of a list?
i don't think that it's worth the overhead. the futex call is only made in the contended case so the number of items in the list is bounded to the number of locks that are currently contended (ie: someone holding it and someone waiting for it). you'd have to have an awful lot of threads and an awful lot of unique locks in play before it would be worth the trade-off.
Fair enough.
Created attachment 152776 [details] [review] Remove second declarations Would you consider this patch? I'm using MS compiler which complains (warning/error) because of the second declarations. This only applies if HAVE_FUTEX is NOT defined (or G_BIT_LOCK_FORCE_FUTEX_EMULATION is).
Haakon: thanks for the patch. That was an oversight on my part. I've applied it.