Thread: Re: [PATCHES] Win32 CHECK_FOR_INTERRUPTS() performance

> > Here's another version of this patch ;-) I've based it on
> your patch,
> > so the changes to ovalue etc should sitill be there.
>
> In the spirit of incremental improvement ... I've taken
> Magnus' version and added the proposed change to re-enable
> Qingqing's patch by skipping WaitForSingleObjectEx altogether
> in the CHECK_FOR_INTERRUPTS code path.
> I also removed WaitForSingleObjectEx in
> pgwin32_poll_signals(), which AFAICS should be just like
> CHECK_FOR_INTERRUPTS.  I think this is what we are proposing
> to actually apply to 8.1beta4.  I can't test it though, so
> please check it over...

That does seem right. I think the only reason it was there i nthe first
place was to deliver the APCs.

But. in theory, we can get a false positive from
UNBLOCKED_SIGNAL_QUEUE(), right? Since we do it unlocked between two
threads. If we do that, we'll "recover" in dispatch_signals, because
we'l lcheck again locked and not dispatch any signals. *but*. If this
happens, we will return EINTR even when there is no signal. That doesn't
seem correct to me. It's a very small window, but it should be possible,
no?

We probably need an actual check, so for example have
dispatch_queued_signals return a value indicating if any signals were
actually dispatched, and use that to control EINTR?
Comments? Or am I completely off being too tired right now? ;-)

//Magnus

On Sun, 23 Oct 2005, Magnus Hagander wrote:

>
> But. in theory, we can get a false positive from
> UNBLOCKED_SIGNAL_QUEUE(), right? Since we do it unlocked between two
> threads. If we do that, we'll "recover" in dispatch_signals, because
> we'l lcheck again locked and not dispatch any signals. *but*. If this
> happens, we will return EINTR even when there is no signal. That doesn't
> seem correct to me. It's a very small window, but it should be possible,
> no?
>
> We probably need an actual check, so for example have
> dispatch_queued_signals return a value indicating if any signals were
> actually dispatched, and use that to control EINTR?
>
> Comments? Or am I completely off being too tired right now? ;-)
>

You are not. Basically that's what I just sent an email about :-) Since
signals are not quite often happened, so I am thinking just adding a
UNBLOCKED_SIGNAL_QUEUE() is more safe maybe for now.

Regards,
Qingqing

"Magnus Hagander" <mha@sollentuna.net> writes:
> But. in theory, we can get a false positive from
> UNBLOCKED_SIGNAL_QUEUE(), right?

We could have gotten a false positive from the old coding, too.
The event was certainly not any more tightly tied to the presence
of an unserviced signal flag than UNBLOCKED_SIGNAL_QUEUE, and arguably
less so.

I think this concern is irrelevant anyway.  Returning EINTR from
select() is OK even if no signal was actually serviced, so long as
it doesn't recur indefinitely.  EINTR just means "I didn't do the
select(), try again".
        regards, tom lane