Thread: Re: [PATCHES] Win32 CHECK_FOR_INTERRUPTS() performance
> >>And the patch that was applied gives the same result. > >> > >>What is more, I am not seeing the reported speedup - in fact I am > >>seeing no speedup worth mentioning. > >> > >>This is on XP-Pro, with default postgres settings. The test > sets were > >>somewhat larger than thos Magnus used - basically TPC-H > lineitems and > >>orders tables (6m and 1.5m rows respectively). > >> > >> > > > >First, that was Merlin and not me :-) > > > > > > My apologies to both of you. Or to at least one of you, anyway. :-) :-) > >Second, it didn't really show any improvement for him either in his > >normal test. But when he re-ran it with just the count(*) test it > >showed improvement. Did you run a count(*) test or some other test? > > > The tests were <snip> That certainly looks like a similar test. Just checking :-) You were on XP - Merlin, what OS were you on? It could be possible things have chenged there as well... > But it looks to me like we need to leave this for 8.2 now > anyway, unless someone can quickly get to the bottom of why > it's hanging. Agreed. I definitly vote for backing out the patch so we don't block the release. If we find the problem we put it back in before release, but if it takes a while we just wait for 8.2. //Magnus
"Magnus Hagander" <mha@sollentuna.net> writes:
> Agreed. I definitly vote for backing out the patch so we don't block the
> release. If we find the problem we put it back in before release, but if
> it takes a while we just wait for 8.2.
After digging in the Microsoft documentation a little bit, I think I
see the problem: we implement SIGALRM timeouts by means of a "waitable
timer", and the system's action when the timeout expires is:
When a timer expires, the timer is set to the signaled state. If the timer has a completion routine, it is
queuedto the thread that set the timer. The completion routine remains in the APC queue of the thread until
thethreads enters an alertable wait state. At that time, the APC is dispatched and the completion routine
iscalled.
In other words, since the main thread is the one that set the timer,
it has to run the completion routine (which just sets the signal flag).
The completion routine will never be run if the main thread is in a
tight loop with no kernel calls. In particular, it was the frequent
execution of WaitForSingleObjectEx via CHECK_FOR_INTERRUPTS that allowed
this technique to work at all.
Isn't there some way we can get the timer completion routine to be run
by the signal thread instead? This coding seems pretty unreliable to me
even without QQ's patch.
regards, tom lane
I wrote:
> Isn't there some way we can get the timer completion routine to be run
> by the signal thread instead? This coding seems pretty unreliable to me
> even without QQ's patch.
After further thought it seems like the right thing to do is to redesign
port/win32/timer.c so that it sets up a separate thread whose
responsibility is to wait for timeouts and deliver a SIGALRM signal back
to the main thread when they happen. It's probably a bit late to
consider doing this for 8.1 :-(
I've temporarily disabled Qingqing's patch by the expedient of removing
the UNBLOCKED_SIGNAL_QUEUE() check in the macro, so that the out-of-line
routine is always called (since we're going to do a kernel call anyway,
one extra layer of subroutine call doesn't seem very important). We can
put it back after fixing timer.c.
regards, tom lane
Tom Lane wrote: > >After further thought it seems like the right thing to do is to redesign >port/win32/timer.c so that it sets up a separate thread whose >responsibility is to wait for timeouts and deliver a SIGALRM signal back >to the main thread when they happen. It's probably a bit late to >consider doing this for 8.1 :-( > > > > The hard part looks to be cancelling/changing the timer, which means that we can't just create a set and forget listener thread for a given timeout. Otherwise that seems to me the straightforward approach. But maybe one of the Windows wizards has a better idea. I doubt the changes would be very invasive - with luck just confined to timer.c. cheers andrew
Andrew Dunstan <andrew@dunslane.net> writes:
> The hard part looks to be cancelling/changing the timer, which means
> that we can't just create a set and forget listener thread for a given
> timeout. Otherwise that seems to me the straightforward approach.
Yeah. I think probably the cleanest way is to create a persistent
thread that manages the timer. We need a way for the main thread to
tell it to cancel the timer or change the setting. Dunno enough about
Windows' interthread communication primitives to propose details.
> I doubt the changes would be very invasive - with luck just confined to
> timer.c.
I don't see a need for anything else to know about it, either.
regards, tom lane
> Andrew Dunstan <andrew@dunslane.net> writes:
> > The hard part looks to be cancelling/changing the timer, which means
> > that we can't just create a set and forget listener thread for a given
> > timeout. Otherwise that seems to me the straightforward approach.
>
> Yeah. I think probably the cleanest way is to create a persistent
> thread that manages the timer. We need a way for the main thread to
> tell it to cancel the timer or change the setting. Dunno enough about
> Windows' interthread communication primitives to propose details.
>
Oh my ... fortunately we got a timer test in regression.
I've come up with a quick patch implementing above discussions. Also,
seems by patching this, we can support setitimer(.,.,ovalue != NULL) --
because it is saved in the memory.
Regards,
Qingqing
---
Index: timer.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/port/win32/timer.c,v
retrieving revision 1.5
diff -u -r1.5 timer.c
--- timer.c 31 Dec 2004 22:00:37 -0000 1.5
+++ timer.c 23 Oct 2005 00:53:56 -0000
@@ -15,8 +15,16 @@
#include "libpq/pqsignal.h"
+/* Communication area of timer settings */
+typedef struct timerCA{
+ int which;
+ struct itimerval value;
+ HANDLE event;
+}timerCA;
+static timerCA timerCommArea;static HANDLE timerHandle = INVALID_HANDLE_VALUE;
+static HANDLE timerThreadHandle = INVALID_HANDLE_VALUE;
static VOID CALLBACKtimer_completion(LPVOID arg, DWORD timeLow, DWORD timeHigh)
@@ -28,16 +36,14 @@/* * Limitations of this implementation: *
- * - Does not support setting ovalue * - Does not support interval timer (value->it_interval) * - Only supports
ITIMER_REAL*/
-int
-setitimer(int which, const struct itimerval * value, struct itimerval * ovalue)
+static int
+do_setitimer(int which, const struct itimerval * value){ LARGE_INTEGER dueTime;
- Assert(ovalue == NULL); Assert(value != NULL); Assert(value->it_interval.tv_sec == 0 &&
value->it_interval.tv_usec== 0); Assert(which == ITIMER_REAL);
@@ -69,3 +75,56 @@
return 0;}
+
+/* Timer ticking thread */
+static DWORD WINAPI
+pg_timer_thread(LPVOID param)
+{
+ Assert(param == NULL);
+
+ for (;;)
+ {
+ if (WaitForSingleObjectEx(timerCommArea.event, INFINITE, TRUE) == WAIT_OBJECT_0)
+ {
+ do_setitimer(timerCommArea.which, &timerCommArea.value);
+ ResetEvent(timerCommArea.event);
+ }
+ }
+
+ return 0;
+}
+
+/*
+ * Win32 setitimer emulation by creating a persistent thread
+ * to handle the timer setting and notification upon timeout.
+ */
+int
+setitimer(int which, const struct itimerval * value, struct itimerval * ovalue)
+{
+ Assert(value != NULL);
+
+ if (timerThreadHandle == INVALID_HANDLE_VALUE)
+ {
+ /* First call in this backend, create event and the timer thread */
+ timerCommArea.event = CreateEvent(NULL, TRUE, FALSE, NULL);
+ if (timerCommArea.event == NULL)
+ ereport(FATAL,
+ (errmsg_internal("failed to create timer event: %d", (int) GetLastError())));
+ MemSet(&timerCommArea.value, 0, sizeof(struct itimerval));
+
+ timerThreadHandle = CreateThread(NULL, 0, pg_timer_thread, NULL, 0, NULL);
+ if (timerThreadHandle == INVALID_HANDLE_VALUE)
+ ereport(FATAL,
+ (errmsg_internal("failed to create timer thread: %d", (int) GetLastError())));
+ }
+
+ /* Request the timer thread to change settings */
+ if (ovalue)
+ *ovalue = timerCommArea.value;
+ timerCommArea.which = which;
+ timerCommArea.value = *value;
+ SetEvent(timerCommArea.event);
+
+ /* Timer thread will handle possible errors */
+ return 0;
+}
Well, first, have you tested it with "make check"? I am not sure if there's any great value in supporting a non null old value param. Second, please note that the PostgreSQL community convention is for patches as context diffs, not unidiffs. I guess there are several ways to skin this cat - the way I had sort of worked out reading the MSDN docs was to call QueueUserAPC on the timer thread. I'd like to know what Magnus and Merlin especially think out it. cheers andrew Qingqing Zhou wrote: >>Andrew Dunstan <andrew@dunslane.net> writes: >> >> >>>The hard part looks to be cancelling/changing the timer, which means >>>that we can't just create a set and forget listener thread for a given >>>timeout. Otherwise that seems to me the straightforward approach. >>> >>> >>Yeah. I think probably the cleanest way is to create a persistent >>thread that manages the timer. We need a way for the main thread to >>tell it to cancel the timer or change the setting. Dunno enough about >>Windows' interthread communication primitives to propose details. >> >> >> > >Oh my ... fortunately we got a timer test in regression. > >I've come up with a quick patch implementing above discussions. Also, >seems by patching this, we can support setitimer(.,.,ovalue != NULL) -- >because it is saved in the memory. > > >
On Sat, 22 Oct 2005, Andrew Dunstan wrote: > > Well, first, have you tested it with "make check"? I am not sure if > there's any great value in supporting a non null old value param. > Yeah, I've managed to install in my slow windows box and tested it. Supporting ovalue is just the by-product. If it works, I will complete the patch by adding threading safe critical section protect. > Second, please note that the PostgreSQL community convention is for > patches as context diffs, not unidiffs. > Ok. > I guess there are several ways to skin this cat - the way I had sort of > worked out reading the MSDN docs was to call QueueUserAPC on the timer > thread. I'd like to know what Magnus and Merlin especially think out it. > I am not sure - does this not require another thread in an alterable state? Regards, Qingqing
Here is the full patch of the timer implemenation with threading safty
added. Basic test is by several rounds of "make check" and threading
safty test is by a SQL file with many lines of "set statement_timeout =
x". I don't know if there are any corner cases that I should consider, if
any, let me know.
Regards,
Qingqing
---
Index: timer.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/port/win32/timer.c,v
retrieving revision 1.5
diff -c -r1.5 timer.c
*** timer.c 31 Dec 2004 22:00:37 -0000 1.5
--- timer.c 23 Oct 2005 03:04:53 -0000
***************
*** 15,22 ****
--- 15,31 ----
#include "libpq/pqsignal.h"
+ /* Communication area of timer settings */
+ typedef struct timerCA{
+ int which;
+ struct itimerval value;
+ HANDLE event;
+ CRITICAL_SECTION crit_sec;
+ }timerCA;
+ static timerCA timerCommArea; static HANDLE timerHandle = INVALID_HANDLE_VALUE;
+ static HANDLE timerThreadHandle = INVALID_HANDLE_VALUE;
static VOID CALLBACK timer_completion(LPVOID arg, DWORD timeLow, DWORD timeHigh)
***************
*** 24,43 **** pg_queue_signal(SIGALRM); }
- /* * Limitations of this implementation: *
- * - Does not support setting ovalue * - Does not support interval timer (value->it_interval) * - Only supports
ITIMER_REAL */
! int
! setitimer(int which, const struct itimerval * value, struct itimerval * ovalue) { LARGE_INTEGER dueTime;
- Assert(ovalue == NULL); Assert(value != NULL); Assert(value->it_interval.tv_sec == 0 &&
value->it_interval.tv_usec== 0); Assert(which == ITIMER_REAL);
--- 33,49 ---- pg_queue_signal(SIGALRM); }
/* * Limitations of this implementation: * * - Does not support interval timer (value->it_interval) * - Only
supportsITIMER_REAL */
! static int
! do_setitimer(int which, const struct itimerval * value) { LARGE_INTEGER dueTime;
Assert(value != NULL); Assert(value->it_interval.tv_sec == 0 && value->it_interval.tv_usec == 0);
Assert(which== ITIMER_REAL);
***************
*** 69,71 ****
--- 75,136 ----
return 0; }
+
+ /* Timer ticking thread */
+ static DWORD WINAPI
+ pg_timer_thread(LPVOID param)
+ {
+ Assert(param == NULL);
+
+ for (;;)
+ {
+ if (WaitForSingleObjectEx(timerCommArea.event, INFINITE, TRUE) == WAIT_OBJECT_0)
+ {
+ EnterCriticalSection(&timerCommArea.crit_sec);
+ do_setitimer(timerCommArea.which,
+ &timerCommArea.value);
+ ResetEvent(timerCommArea.event);
+ LeaveCriticalSection(&timerCommArea.crit_sec);
+ }
+ }
+
+ return 0;
+ }
+
+ /*
+ * Win32 setitimer emulation by creating a persistent thread
+ * to handle the timer setting and notification upon timeout.
+ */
+ int
+ setitimer(int which, const struct itimerval * value, struct itimerval * ovalue)
+ {
+ Assert(value != NULL);
+
+ if (timerThreadHandle == INVALID_HANDLE_VALUE)
+ {
+ /* First call in this backend, create event and the timer thread */
+ timerCommArea.event = CreateEvent(NULL, TRUE, FALSE, NULL);
+ if (timerCommArea.event == NULL)
+ ereport(FATAL,
+ (errmsg_internal("failed to create timer event: %d", (int) GetLastError())));
+ MemSet(&timerCommArea.value, 0, sizeof(struct itimerval));
+ InitializeCriticalSection(&timerCommArea.crit_sec);
+
+ timerThreadHandle = CreateThread(NULL, 0, pg_timer_thread, NULL, 0, NULL);
+ if (timerThreadHandle == INVALID_HANDLE_VALUE)
+ ereport(FATAL,
+ (errmsg_internal("failed to create timer thread: %d", (int) GetLastError())));
+ }
+
+ /* Request the timer thread to change settings */
+ EnterCriticalSection(&timerCommArea.crit_sec);
+ if (ovalue)
+ *ovalue = timerCommArea.value;
+ timerCommArea.which = which;
+ timerCommArea.value = *value;
+ LeaveCriticalSection(&timerCommArea.crit_sec);
+ SetEvent(timerCommArea.event);
+
+ /* Timer thread will handle possible errors */
+ return 0;
+ }
Qingqing Zhou wrote: >>I guess there are several ways to skin this cat - the way I had sort of >>worked out reading the MSDN docs was to call QueueUserAPC on the timer >>thread. I'd like to know what Magnus and Merlin especially think out it. >> >> >> >I am not sure - does this not require another thread in an alterable >state? > > > > Maybe, I don't know. My impression from the docs was that the thread could call WaitForSingleObjectEx on the timer handle and it would also respond to the APC call. But my Windows API knowledge is not exactly large. As long as what you have works it should be OK. cheers andrew