GNOME Bugzilla – Bug 324228
support threadpool exiting idle threads
Last modified: 2011-02-18 15:49:17 UTC
The threadpool should support: * queue sorting so threads can be prioritised from a sort function. * cleaning up threads that have been idle for 'n' ms. The reason for this is to allow GnomeVFS to use glib for their priority thread handling.
Created attachment 56053 [details] [review] Initial attempt. I have updated the threadpool-test to test sorting thread pools and to test cleaning up idle threads. I have added debugging to the gthreadpool.c source to make it easier to understand what is going on - this is disabled by default in the patch.
Tim pointed out that once you call g_thread_pool_set_max_idle_time (pool, 15000) then how do you get back to the default behaviour? You could call g_thread_pool_set_max_unused_threads ( > -1 ) but then if someone sets it back to -1 then they shouldn't expect it to just use the idle code again, so I guess the pool->max_idle_time should be set to 0 once you do this. This means we would have to apply the pool->max_idle_time change to ALL threads pools I guess since max_unused_threads is not bound to a specific GThreadPool.
Can't you just allow resetting max_idle_time to 0 ? Also, why can't max_unused_threads and max_idle_time be used together ? Apart from it being maybe a bit more complicated to implement, I can see a combination of the two make sense. "Keep unused threads around, but not for longer than 120 seconds, and not more than 30" makes sense to me... On the other hand, it is slightly confusing that max_unused_threads is global, while max_idle_time is per pool. So it would be more like "keep idle threads in the pool for 120 seconds, and keep a global reserve of up to 30 unused threads". Right ?
Setting the max_idle_time to 0 seems like a good way forward here. I will update the patch accordingly. I can see the point of having unused threads and a max idle time together but like you say it is confusing because of the global pool, and the implementation is quite complicated too. My thinking at the time was: 1. if you don't want threads lying around. 2. if you want thread left for potential jobs. However, if you want a combination 1. and 2. (not leaving threads idle but also having them around until the queue is quiet) then I thought perhaps it should be different implementation. However, I will attempt to work the patch so you can used max_unused_threads and max_idle_time together. Matthias, do you think the max_idle_time should be global like the max_unused_threads? I did it per GThreadPool because I thought people might want to use the max_idle_time in some cases and not in others, but I guess if make use of the max_idle_time feature it renders the max_unused_threads feature inconsistent depending on the pool. I think we should make the max_idle_time global.
First I think, we should split this patch into two. The first part with just the sorted thread pool queue looks fine and is, I suppose, largely undisputed. [I would have preferred the patch with less whitespace noise, however] The second part (max_idle_time etc.) is ot only completely unrelated to the other change, but also I don't completly get it. I mean the whole point of having thread pools in GLib is to share the pools between different thread pools of systems using GLib, so defining seperate maximum idle times for the individual thread pools is a bit pointless, because after not being used, those threads automatically return to global thread pool, where you can already set a number of maximum threads. That a thread is waiting for 1/2 a second before returning to the global thread pool is just an (maybe unnecessary) optimization to avoid handing the thread around to often, when work arrives shortly after the old is done. A pure implementation detail. What you could, however, reasonably do is defining a maximal idle time for the global thread pool, after which a thread would exit. Still the implementation would not be easy: * When would counting the idle time of a thread start? * How to react upon changing that value: waking up all idle threads? how would the thread then know, how long it's been in the queue to calculate when to exit. Could you maybe elaborate, why GnomeVFS needs the maximum-idle-time feature, that we can find a good solution?
Sebastian, thanks for your comments. OK, I will divide this work into 2 patches. The sort work is a relatively small change anyway. After talking to Alex and Gicmo (about GnomeVFS's priority thread pool code and the possibility of getting threads to exit after a period of inactivity), they seemed surprised that it wasn't already doing that. Like the 1/2 second optimisation in the threadpool code to prevent unnecessary context switching, we wanted to do the same thing where idle threads exited over a period of inactivity. The reason for this, is to reduce threads, load, power used, etc. on the Nokia 770. We can already do this to some extent by reducing the max thread count and that is good, but calling the _stop_unused() threads API on a regular timeout is less optimal to each thread dying after it has been twiddling its thumbs for 15 seconds (or some time). As I said to Matthias, I think the max_idle_time should be a global preference and you seem to agree too Sebastian :) With regards to implementation, the method I used in the first patch was to block on the pool queue for the next job OR time out after 'n' seconds (using g_async_queue_timed_pop_unlocked()). The idle time of a thread should start the time it re-enters the pool.
I agree with Sebastian, the sorting part of this patch can go in quickly. One little detail: I would prefer if set_sort_function() was able to reconstruct the initial state (of no sort function), ie it should accept NULL as a sort function.
I have created a separate bug for the sort functionality (#324479) and am marking this as depending on it since the the new patch for this bug adds to the test harness updated in #324479.
as for the rationale sebastian asked for: having large amounts of idle threads hanging around unneccessarily consumes resources which can be harmfull on devices with sparse resources. reducing the thread pool to just a single thread can severely hamper interactivity for gnome-vfs scenarios so some compromise needs to be found here. a combination of two global settings for the maximum number of threads and maximum idle time for a thread can provide this. for sparse resource devices like the N770, the maximum number of threads can be set to something like 5 or 11, so it can adapt to situations with high IO and still provide good interactivity, and it can throttle its resource consumption again for longer idle times. e.g. because the user switched processes and needs the gnome-vfs interactiveness in another process now. to some extend the maximum idle time mimicks the "working set" time, described and defined in section 3.7 of: [Bonwick01] Bonwick and Jonathan Adams, Magazines and vmem: Extending the slab allocator to many cpu's and arbitrary resources. USENIX 2001, http://citeseer.ist.psu.edu/bonwick01magazines.html in another paper, bonwick explains that 15 seconds is what the solaris kernel allocator uses to prevent trashing. this is more or less applicable to adapting the number of idle threads, so that's how it got suggested to martyn on an occasion ;)
Created attachment 56291 [details] [review] Second attempt. This patch will keep idle threads in the pool for some time, and keep a global reserve of up to some unused threads (however specified by the user).
i can't say too much on the implementation here, sebastian will hopefully do that. so for a few cosmetic things: i think the global locks should be merged into on as: +G_LOCK_DEFINE_STATIC (queue_settings); +static gint max_unused_threads = 0; +static guint max_idle_time = 0; comments are usually prefixed by " * " on each line, you miss some of these. +guint g_thread_pool_get_max_idle_time (void) this should be +guint +g_thread_pool_get_max_idle_time (void) +#define MAX_UNUSED_THREADS -1 /* if this is > 0 the test will continously r why isn't this "0" then? or was the comment meant to say ">=0" ?
Tim, thanks for your comments. I agree with having one lock, it should simplify things a bit. The comment(s) prefix "*" and the function name on another line is my carelessness, thanks for pointing those out. Tim: The MAX_UNUSED_THREADS is more influential for other reasons (-1 = unlimited unused threads and 0 = no unused threads). But for the threadpool-test, I wait for the thread count to reach 0 so I know when to quit. The test harness could be improved further I guess but time was running out to get this into GLib before the freeze. If you set this value to '2' it should keep 2 threads around and any surplus threads are timed out on the idle time. I should have another patch some time tomorrow morning.
Created attachment 56328 [details] [review] Third attempt OK, so this patch has been updated from Tim's comments: * Comments prefixed with "*"'s * Function name is on the line after the return type. * Use one static lock for unused threads and idle time. I re-tested the threadpool-test and it all seems fine. One question I do have regarding this ideal time of "15" seconds which Tim talks about, is it worth putting that in the documentation?
Created attachment 56913 [details] [review] Really apply the maximal idle time to threads in the global pool only. As promised here is a, in my book, better version of the maximal idle time patch against current CVS. Also it cleans up some issues, I found while implementing it. Now the logic is as follows: A thread waits at the specified thread pool for at most 1/2 second before going to the global pool. There it will wait for at most max-idle-time (might be indefinite), and will stop if no new pool has been assigned until then. Upon changing the max-idle-time currently waiting threads will be woken up to adhere to the new value. The fixed issue is, that now a superflous thread of a pool will first process the received task, before exiting, thus avoiding reinserting the task into the queue therby possibly changing the order (this can happen now, when the maximal number of threads is reduced). Currently I have no CVS write access, but if no-one objects, I will commit the patch next week, when I'm at my home computer. Martyn, could you please check, whether the new semantics match your intention?
Sebastian: This looks good! The tests all run fine so I am happy with it. Don't forget debugging is turned on in this patch ;) (before committing).
Created attachment 57496 [details] [review] Patch to make it work again Using g_thread_pool_set_sort_function totally breaks the threadpool use in gnome-vfs. I guess its due to the missing g_cond_singal (). The little 2liner fixes the issue for me.
*** Bug 327290 has been marked as a duplicate of this bug. ***
2006-01-17 Sebastian Wilhelmi <seppi@seppi.de> * glib/gthreadpool.c: To avoid deadlocks get rid of the settings G_LOCK. Use the unused_thread_queue lock instead. Change g_thread_pool_thread_proxy such that threads only wait on non-exlusive pools for at most a 1/2 second. Do not reorder tasks due to superfluous tasks. Global tasks wait at most for max-idle-time milliseconds. Make sure, that no task is woken up twice for the same event via a wakeup_serial. This fixes #324228. * tests/threadpool-test.c: Adapt test accordingly. Do not pass invalid NULL into the thread pools. This as well fixes #327290.