Bug 42889

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.