Re: Performance degradation in commit ac1d794

On Wed, Mar 16, 2016 at 10:04 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> Questions:
> * I'm kinda inclined to merge the win32 and unix latch
> implementations. There's already a fair bit in common, and this is
> just going to increase that amount.

Don't care either way.

> * Right now the caller has to allocate the WaitEvents he's waiting for
> locally (likely on the stack), but we also could allocate them as part
> of the WaitEventSet. Not sure if that'd be a benefit.

I'm not seeing this. What do you mean?

0001: Looking at this again, I'm no longer sure this is a bug.
Doesn't your patch just check the same conditions in the opposite
order?

0002: I think I reviewed this before. Boring. Just commit it already.

0003: Mostly boring. But the change to win32_latch.c seems to remove
an unrelated check.

0004:

+ * drain it everytime WaitLatchOrSocket() is used. Should the
+ * pipe-buffer fill up in some scenarios - widly unlikely - we're

every time
wildly

Why is it wildly (or widly) unlikely?

The rejiggering this does between what is on which element of pfds[]
appears to be unrelated to the ostensible purpose of the patch.

+ * Check again wether latch is set, the arrival of a signal/self-byte

whether. Also not clearly related to the patch's main purpose.

/* at least one event occurred, so check masks */
+ if (FD_ISSET(selfpipe_readfd, &input_mask))
+ {
+ /* There's data in the self-pipe, clear it. */
+ drainSelfPipe();
+ }

The comment just preceding this added hunk now seems to be out of
place, and maybe inaccurate as well. I think the new code could have
a bit more detailed comment. My understanding is something like /*
Since we didn't clear the self-pipe before attempting to wait,
select() may have returned immediately even though there has been no
recent change to the state of the latch. To prevent busy-looping, we
must clear the pipe before attempting to wait again. */

I'll look at 0005 next, but thought I would send these comments along first.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company