The thread pooling support (apr_thread_pool.[ch]) did not follow the convention of apr-utils services using APU_DECLARE vs APR_DECLARE. Added the following enhancements to the thread pooling support: 1) Time out of idle threads vs hard idle_max reap. Helps reduce thrashing of threads when requests are receive in bursts and allows idle to be set to zero w/o being penalized for thread create/destroy. 2) More statistics (#of tasks processed, high water mark of queued tasks, high water mark of threads, #of idle threads that timed out waiting for work). These updates are contained w/in diff files that will be attached momentarily.
Created attachment 20503 [details] source updates
Created attachment 20504 [details] APU_DECLARE and new stat prototypes
Quick review. Why did you removed the doxygen group tags? Could you separate the patches in three (APU_DECLARE, timeout, statistics)? Thanks.
I must have missed the group update. I will resync the files and submit the patches separately as requested. Do I need to submit three different bug reports for the separated patches? Or, is one bug ok?
(In reply to comment #4) > I must have missed the group update. I will resync the files and submit the > patches separately as requested. Thanks. > Do I need to submit three different bug reports for the separated patches? Or, > is one bug ok? Just this one is ok.
CC'ing the original author.
Nice. :-) One question: since the total number of tasks processed(tasks_run) is collected over time and could overflow, would it better to collect in interval? Anyway, that may not be a big concern. The code stopping the timeout idle threads is not quite correct: 1. The thread should only be joined when the thread is stopped in idle mode by trim_idle_thread, i.e, when elt->stop is 1. Otherwise, the thread should be detached and exit as no one would be there join the thread. 2. The thd_timed_out should be increased for the thread to quit after time out, ie. when elt->stop == 0. The code seems to have an unintended else with the if statement.
Oh, shoot! Thanks! I'll make these changes before submitting the updated diffs. I'm assuming the diffs should contain: 1) existing source to source w/APU updates 2) source w/APU updates to source w/time out updates 3) source w/APU & time out updates to source w/additional statistics
Created attachment 20505 [details] apr_thread_pool.c.diff.apu APU updates Replacing big attachments w/incremental attachments
Created attachment 20506 [details] apr_thread_pool.c.diff.timeout Time out updates
Created attachment 20507 [details] apr_thread_pool.c.diff.stats Statistics updates
Created attachment 20508 [details] apr_thread_pool.h.diff.apu APU updates
Created attachment 20509 [details] apr_thread_pool.h.diff.timeout Time out updates
Created attachment 20510 [details] apr_thread_pool.h.diff.stats Statistics updates
APU updates patch committed to trunk in revision 558527: http://svn.apache.org/viewvc?view=rev&revision=558527
Created attachment 20539 [details] timeout (idle wait) update The earlier patch may miss the scheduled task if the idle_wait is set and idle_max is larger than 0. This updated patch should address the scheduled task. The patch also refactoring a bit to use a state rather than a boolean to signal a thread's exit. This eliminate the additional code to quit the thread.
Created attachment 20540 [details] stats patch update A minor update to reflect changes on timeout
Created attachment 20542 [details] timeout (idle wait) update (typo correction) Fix typo in apr_thread_pool_idle_wait_set() @param timeout documentation. 'timeout' is microseconds... not milliseconds. And thanks for putting this together so quickly.
I applied the latest patches and observed that once a thread was put on probation, it was reaped even if it was woken up to process a task (vs actually timing out). Shouldn't a thread on probation that was woken to process a task have its state transition back to run? Thanks for your consideration.
I considered that, and I have no preference to either way. The reason I left it out is because the thread will only stop after becoming idle again, i.e., no other tasks in the queue. While this most likely mean all other threads will be idle as well and unless the max_idle was changed, the thread need to be stopped anyway. Make sense?
Yes, the implementation makes sense and I can go either way as well. My comment was more of from a thread in hand position. I was thinking that if the thread didn't really timeout, then there must be enough work to keep it busy and it should be given another chance to waste away. As an aside, I updated the thread pool code to change the state from probation to run after a task had been processed. In my use of the thread pool (~50 second run on dual core Wintel), giving an idle thread that woke up and did work another chance changed the number of reaped idle threads from 28 to 17 and shaved around 4 seconds off of the run. Thanks again for the implementation and your very prompt attention.
I haven't seen any activity since my last response. Is more information still required? Thanks.
I believe this is ready to roll so I am reassigning back to the folks that handle APR updates.
It's my understanding from Henry that these patches are no longer required as the patch for 43876 is now committed. Please correct my understanding if this is not true.
Your understanding is correct. 43876 included these fixes.