Thanks for these especially good comments, Rob!
On Sat, Jul 19, 2008 at 07:08:17PM +0100, Rob Shearman wrote:
> 2008/7/19 Dan Hipschman <[EMAIL PROTECTED]>:
> >
> > This patch implements the timer thread, running callbacks, cleaning up
> > timers, and all that good stuff. It should be very efficient, using mostly
> > atomic operations and one critical section for thread safety.
>
> It would be more efficient for queues with large numbers of timers if
> you used the approach used by server/fd.c of keeping a list of timers
> sorted by when they next expire to avoid having to search through the
> entire list every time a timer expires.
The answer to most of your questions is, because I was trying to start
simple. I'll look into this, though.
> > +static void queue_timer_expire(struct queue_timer *t)
> > +{
> > + /* We MUST hold the queue cs while calling this function. */
> > + if (!t->die)
> > + {
> > + ULONG flags =
> > + t->flags & (WT_EXECUTEINIOTHREAD | WT_EXECUTEINPERSISTENTTHREAD
> > + | WT_EXECUTELONGFUNCTION |
> > WT_TRANSFER_IMPERSONATION);
> > + InterlockedIncrement(&t->runcount);
> > + QueueUserWorkItem(timer_callback_wrapper, t, flags);
>
> You should check the return value and undo the incrementing of
> t->runcount if it failed. However, you're also calling
> QueueUserWorkItem inside a critical section so you're imposing a lock
> ordering on the loader lock. This may cause a deadlock that may or may
> not exist on Windows if a timer queue function is called from an
> application's DllMain.
Thanks for telling me about this. It should be safe to move
QueueUserWorkItem outside the CS. I'll think about it later.
> It would be better to use a thread pool thread for processing timer
> queue since it can be reused when the timer queue is destroyed. It
> might then also be better to only start the timer queue thread when
> there are timers to process and also release the timer queue thread
> back to the thread pool when the last timer is removed from the queue.
>
> Also, have you thought about using one thread for all timer queues?
> This might not be appropriate for WT_EXECUTEINTIMERTHREAD timers, but
> should improve the performance of the process as a whole without
> affecting other timers.
Yes, I thought about these, but again, trying to keep it simple...
> > @@ -1085,22 +1226,32 @@ BOOL WINAPI DeleteTimerQueueEx(HANDLE TimerQueue,
> > HANDLE CompletionEvent)
> > struct queue_timer *t, *temp;
> > BOOL ret;
> >
> > + RtlEnterCriticalSection(&q->cs);
> > + LIST_FOR_EACH_ENTRY_SAFE(t, temp, &q->timers, struct queue_timer,
> > entry)
> > + t->die = TRUE;
> > + q->quit = TRUE;
> > + RtlLeaveCriticalSection(&q->cs);
> > + SetEvent(q->event);
> > +
> > if (CompletionEvent == INVALID_HANDLE_VALUE)
> > {
> > ret = TRUE;
> > + WaitForSingleObject(q->thread, INFINITE);
>
> What if timer_queue_thread_proc exits before you reach here? q->thread
> will now be invalid (and could be re-used by another thread in the
> mean time). Another way of looking at it is that you don't own q after
> the critical section where q->quit is set.
This is a great catch. I utterly missed this. Maybe the easiest fix is
to dup the thread handle into a local. I'll look for a cleaner approach
as well.
> If you use an expire list in a local variable then you won't need
> locking when processing the expired timers and so this will be trivial
> to implement.
I'll take another look.
> Quite a few comments, but the patches are of a good general quality,
> so well done.
Thanks for taking the time to go over this so thoroughly. Thanks, Hans,
for the comments as well; I'll check out GetTickCount64.
Dan
