GNOME Bugzilla – Bug 660887
g_slice_set_config() is broken
Last modified: 2011-10-04 21:35:46 UTC
g_slice_set_config() can only be called before gslice has been initialised: void g_slice_set_config (GSliceConfig ckey, gint64 value) { g_return_if_fail (sys_page_size == 0); ... In the past, that used to be before the first g_slice_() calls, or before g_thread_init() was called. Now. gslice initialisation happens as a ctor. It is therefore impossible to ever call g_slice_set_config(). Similar (somewhat theoretical) problem with replacing the gmem vtable: gslice initialisation uses g_malloc() and you're not supposed to replace the gmem vtable after you've already called g_malloc(). In this case, I think it's not actually a problem since gslice never frees or reallocates those particular arrays and because gmem doesn't have checking for this condition.
Created attachment 198249 [details] [review] gslice: stop using ctors We can't initialise gslice from a ctor because g_slice_set_config() must be called before gslice initialisation.
Created attachment 198251 [details] [review] Test that g_slice_set_config() works For a while it didn't work, due to the ctor-based initialisation of gslice.
Created attachment 198255 [details] [review] gslice: stop using ctors We can't initialise gslice from a ctor because g_slice_set_config() must be called before gslice initialisation. Instead, do the initialisation in a threadsafe way from the initialisation function for the thread private data. This will only be called once per thread so the synchronisation doesn't pose a significant overhead here. Ensure that we try to grab the thread private data directly on entrance to g_slice_alloc() so that we force the initialisation to occur. Grabbing the private data is the common case anyway.
Review of attachment 198255 [details] [review]: Only cosmetic suggestions - otherwise makes sense to me. ::: glib/gslice.c @@ +419,3 @@ + if (sys_page_size == 0) + g_slice_init_nomessage (); + g_mutex_unlock (&init_mutex); This might be better moved inside g_slice_init_nomessage () - make it a no-op if already initialized. Then you can call it g_slice_ensure_initialized() and just sprinkle it where needed. @@ +768,3 @@ g_slice_alloc (gsize mem_size) { + ThreadMemory *tmem = thread_memory_from_self(); I tend to only do initialization of automatics at declaration to constants; especially it's kind of evil to have functions with side effects there. So this would move... @@ +771,3 @@ gsize chunk_size; gpointer mem; guint acat; ...here.
Created attachment 198263 [details] [review] gslice: stop using ctors I take one of your suggestions (about the evil automatic variable initialiser) and add a comment to make sure nobody tries to be "clever" and undo the move as an optimisation. I'm going to leave the other one as-is, since checking sys_page_size in that way is used in many places throughout the file to check for initialisation and it's theoretically very slightly faster.
Attachment 198251 [details] pushed as bb5d90a - Test that g_slice_set_config() works Attachment 198263 [details] pushed as 5bfb64d - gslice: stop using ctors