GNOME Bugzilla – Bug 660635
Deprecate g_thread_foreach
Last modified: 2011-10-03 05:25:11 UTC
It is really not a very useful function, since there is virtually nothing you can do with a GThread*, and implementing it requires us to keep a list of threads around, and complicates things quite a bit.
Created attachment 197964 [details] [review] Deprecate g_thread_foreach
Created attachment 197966 [details] [review] Drop g_thread_all_threads This drops quite a bit of overhead in maintaining this list. To avoid iterating threads in g_static_private_free(), defer freeing the per-thread data to thread exit. The one complication here is that it is possible for the static private index to be reused while 'old' data is still around. To deal with that case, store the 'owner' with each per-thread data node, and free old data in g_static_private_get() if the owner doesn't match. The remaining possibility that a private index could be reused by a GStaticPrivate with the same address is sufficiently unlikely that we can probably ignore it. With this change, per-thread data is now truly private again, and we can drop the lock for it as well.
Created attachment 197970 [details] [review] Move g_thread_foreach to deprecated/ At the same time, add a warning when this function is called, since it does not do anything anymore.
Created attachment 197973 [details] [review] Drop use of g_thread_foreach in tests This was not reliable anyway, since we have private threads started by GDBus and others now.
So I had a thought about how we can fix this without breaking API in any way: In bug 580505 we are looking for a way to introduce an API to set thread names. I commented that we could do it by deprecating g_thread_create{,_full} and introducing g_thread_new{,_full}. We could clarify the semantics of g_thread_foreach() as being "iterates over all threads created with g_thread_create()" (exactly as it is now) and make it so that g_thread_new() doesn't work with g_thread_foreach(). That would allow us to avoid the extra bookkeeping, except for in the deprecated case.
(In reply to comment #5) > So I had a thought about how we can fix this without breaking API in any way: > > In bug 580505 we are looking for a way to introduce an API to set thread names. > I commented that we could do it by deprecating g_thread_create{,_full} and > introducing g_thread_new{,_full}. > > We could clarify the semantics of g_thread_foreach() as being "iterates over > all threads created with g_thread_create()" (exactly as it is now) and make it > so that g_thread_new() doesn't work with g_thread_foreach(). That would allow > us to avoid the extra bookkeeping, except for in the deprecated case. Sure, that would be a possibility.
Created attachment 198008 [details] [review] Whitespace fix
Created attachment 198009 [details] [review] Rework the way GStaticPrivate data is freed To avoid iterating threads in g_static_private_free(), defer freeing the per-thread data to thread exit. The one complication here is that it is possible for the static private index to be reused while 'old' data is still around. To deal with that case, store the 'owner' with each per-thread data node, and free old data in g_static_private_get() if the owner doesn't match. The remaining possibility that a private index could be reused by a GStaticPrivate with the same address is sufficiently unlikely that we can probably ignore it. With this change, per-thread data is now truly private again, and we can drop the lock for it as well.
Created attachment 198010 [details] [review] Add new thread creation API Deprecate both g_thread_create functions and add g_thread_new() and g_thread_new_full(). The new functions expect a name for the thread. Change GThreadPool, GMainContext and GDBus to create named threads.
Created attachment 198011 [details] [review] Make thread names useful in a debugger Associate the name with the system thread, so that debuggers can see it. This is currently only implemented for Linux, using prctl.
Created attachment 198012 [details] [review] Don't put named threads on the global list This lets us avoid the overhead of maintaining the global list in the non-deprecated case.
Created attachment 198013 [details] [review] Deprecate g_thread_foreach It is really not a very useful function, since there is virtually nothing you can do with a GThread*, and implementing it requires us to keep a list of threads around, and complicates things quite a bit.
Here is the other approach. I think it looks fine; two things I don't like about it are a) we keep all the code for dealing with the thread list in gthread.c b) thread->name plays a double role here; maybe we need to bite the bullet and add a separate 'is_in_list' field.
Created attachment 198043 [details] [review] Add new thread creation API Deprecate both g_thread_create functions and add g_thread_new() and g_thread_new_full(). The new functions expect a name for the thread. Change GThreadPool, GMainContext and GDBus to create named threads.
Created attachment 198044 [details] [review] Make thread names useful in a debugger Associate the name with the system thread, so that debuggers can see it. This is currently only implemented for Linux, using prctl.
Created attachment 198045 [details] [review] Don't put threads created with g_thread_new() on the list This lets us avoid the overhead of maintaining the global list in the non-deprecated case.
Created attachment 198046 [details] [review] Deprecate g_thread_foreach It is really not a very useful function, since there is virtually nothing you can do with a GThread*, and implementing it requires us to keep a list of threads around, and complicates things quite a bit.
Created attachment 198052 [details] [review] Deprecate GStaticPrivate Move GStaticPrivate, g_thread_foreach and all related functions to gthread-deprecated.c. We introduce some internal API to make this possible.
All done now