Re: Performance degradation in commit ac1d794

On 2016-03-21 01:31:30 +1300, Thomas Munro wrote:
> I couldn't get the second patch to apply for some reason,

Weird? Even efter appying the first one first?

> 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.

Yea, I didn't see an in-core need for that, so I didn't want to add
that yet.

Other than that, how did things go?

> 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'm inclined that if we add this, we memmmove everything later, and
adjust the offsets. As we e.g. pass the entire pollfd array to poll() at
once, not having gaps will be more important.

What's the use case where you need that?

> 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.

Doubles the amount of palloc()s required. As lots of these sets are
actually going to be very short-lived (a single WaitLatch call), that's
something I'd rather avoid. Although I guess you could just allocate
separate arrays at that moment.

> 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/
>

Thanks!

> 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...

No, I've not, sorry...

Andres