Re: Performance degradation in commit ac1d794

On Wed, Mar 16, 2016 at 10:04 PM, Andres Freund <andres@anarazel.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