Обсуждение: Re: [PATCHES] Win32 CHECK_FOR_INTERRUPTS() performance tweak

> > This patch improves the win32 CHECK_FOR_INTERRUPTS() performance by
> > testing if any unblocked signals are queued before check
> > pgwin32_signal_event. This avoids an unnecessary system call.
>
> http://archives.postgresql.org/pgsql-patches/2005-10/msg00191.php
>
> This looks to me like a pretty important performance tweak
> for Windows.
> Can any of the people who worked on the Windows signal
> implementation look it over and confirm it's OK?

I think it looks good. (Haven't tested it yet, but from reading it.)

It seems it's safe to skip the interlocking that we do. If we miss one
signal for some reason, we will find it on the next hit to
CHECK_FOR_INTERRUPTS(). And if we get an "extra hit", we still
re-examine the stuff when we're locked, so it shouldn't be a big
problem.

Do you have any numbers on how much performance increases with it? I
agree that it looks like it could be a significant help in some cases,
but it'd be great to have numbers...

I'm a little bit concerned about doing it this late in beta, but it does
look safe to me. When it's that late, it'd be good to have one more
person review it though :)


//Magnus

"Magnus Hagander" <mha@sollentuna.net> writes:
> Do you have any numbers on how much performance increases with it?

A rough estimate is that it would cost more than half as much as EXPLAIN
ANALYZE does: that imposes two extra syscalls per ExecProcNode, while
this adds one.  There are other high-frequency CHECK_FOR_INTERRUPTS
calls that I can't quantify as easily.  My guess is that it's costing us
less than a factor of 2, but well more than 10%, on typical queries
... so definitely worth fixing for 8.1 if we can convince ourselves
it's correct.
        regards, tom lane

"Tom Lane" <tgl@sss.pgh.pa.us> wrote
>
> ... so definitely worth fixing for 8.1 if we can convince ourselves
> it's correct.
>

Despite the performance, there is one thing I am not exactly sure. Shall we 
add "volatile" quanlifier to at least pg_signal_queue? The dangerous place 
is PGSemaphoreLock(). If the compiler cache this value somehow, then we are 
in trouble, but the original way (check event directly) does not have this 
problem.

Regards,
Qingqing

"Qingqing Zhou" <zhouqq@cs.toronto.edu> writes:
> Shall we add "volatile" quanlifier to at least pg_signal_queue?

If that's changed by a separate thread, "volatile" seems essential.
What about the mask variable?
        regards, tom lane

Qingqing Zhou wrote:

>"Tom Lane" <tgl@sss.pgh.pa.us> wrote
>  
>
>>... so definitely worth fixing for 8.1 if we can convince ourselves
>>it's correct.
>>
>>    
>>
>
>Despite the performance, there is one thing I am not exactly sure. Shall we 
>add "volatile" quanlifier to at least pg_signal_queue? The dangerous place 
>is PGSemaphoreLock(). If the compiler cache this value somehow, then we are 
>in trouble, but the original way (check event directly) does not have this 
>problem.
>
>
>  
>
The fact this question is asked worries me a bit.

Also, I have a small style question - why use a nested if instead of 
just saying
 if (UNBLOCKED_SIGNAL_QUEUE() && WaitForSingleObjectEx(pgwin32_signal_event,0,TRUE) == WAIT_OBJECT_0)

?

cheers

andrew

>
> Also, I have a small style question - why use a nested if instead of
> just saying
>
>   if (UNBLOCKED_SIGNAL_QUEUE() &&
> WaitForSingleObjectEx(pgwin32_signal_event,0,TRUE) == WAIT_OBJECT_0)
>

Yeah, this works but IMHO that style states things clearer,

Regards,
Qingqing