Re: Performance degradation in commit ac1d794

On Sun, Mar 20, 2016 at 5:14 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> On 2016-03-19 18:45:36 -0700, Andres Freund wrote:
>> On 2016-03-19 16:44:49 +0530, Amit Kapila wrote:
>> > On Fri, Mar 18, 2016 at 1:34 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
>> > >
>> > >
>> > > Attached is a significantly revised version of the earlier series. Most
>> > > importantly I have:
>> > > * Unified the window/unix latch implementation into one file (0004)
>> > >
>> >
>> > After applying patch 0004* on HEAD, using command patch -p1 <
>> > <path_of_patch>, I am getting build failure:
>> >
>> > c1 : fatal error C1083: Cannot open source file:
>> > 'src/backend/storage/ipc/latch.c': No such file or directory
>> >
>> > I think it could not rename port/unix_latch.c => storage/ipc/latch.c. I
>> > have tried with git apply, but no success. Am I doing something wrong?
>>
>> You skipped applying 0003.
>>
>> I'll send an updated version - with all the docs and such - in the next
>> hours.
>
> Here we go. I think this is getting pretty clos eto being committable,
> minus a bit of testing edge cases on unix (postmaster death,
> disconnecting clients in various ways (especially with COPY)) and
> windows (uh, does it even work at all?).
>
> There's no large code changes in this revision, mainly some code
> polishing and a good bit more comment improvements.

Hi

I couldn't get the second patch to apply for some reason, but I have
been trying out your "latch" branch on some different OSs and porting
some code that does a bunch of waiting on many sockets over to this
API to try it out.

One thing that I want to do but can't with this interface is remove an
fd from the set. I can AddWaitEventToSet returning a position, and I
can ModifyWait to provide new event mask by position including zero
mask, I can't actually remove the fd (for example to avoid future
error events that can't be masked, or to allow that fd to be closed
and perhaps allow that fd number to coincidentally be readded later,
and just generally to free the slot). There is an underlying way to
remove an fd from a set with poll (sort of), epoll, kqueue. (Not sure
about Windows. But surely...). I wonder if there should be
RemoveWaitEventFromSet(set, position) which recycles event slots,
sticking them on a freelist (and setting corresponding pollfd structs'
fd to -1).

I wonder if it would be useful to reallocate the arrays as needed so
there isn't really a hard limit to the number of things you add, just
an initial size.

A couple of typos:

+ * Wait using linux' epoll_wait(2).

linux's

+ /*
+ * If neither the event mask nor the associated latch changes, return
+ * early. That's an important optimization for some sockets, were
+ * ModifyWaitEvent is frequently used to switch from waiting
for reads to
+ * waiting on writes.
+ */

s/were/where/

+ /*
+ * Even though unused, we also poss epoll_ev as the data argument if
+ * EPOLL_CTL_DELETE is passed as action. There used to be an epoll bug
+ * requiring that, and acutally it makes the code simpler...
+ */

s/poss/pass/
s/EPOLL_CTL_DELETE/EPOLL_CTL_DEL/
s/acutally/actually/

There is no code passing EPOLL_CTL_DEL, but maybe this comment is a
clue that you have already implemented remove on some other branch...

--
Thomas Munro
http://www.enterprisedb.com