Re: [HACKERS] kqueue

Hi,

On 2018-09-28 10:55:13 +1200, Thomas Munro wrote:
> On Tue, May 22, 2018 at 12:07 PM Thomas Munro
> <thomas.munro@enterprisedb.com> wrote:
> > On Mon, May 21, 2018 at 7:27 PM, Mateusz Guzik <mjguzik@gmail.com> wrote:
> > > I have benchmarked the change on a FreeBSD box and found an big
> > > performance win once the number of clients goes beyond the number of
> > > hardware threads on the target machine. For smaller number of clients
> > > the win was very modest.
> >
> > So to summarise your results:
> >
> > 32 connections: ~445k -> ~450k = +1.2%
> > 64 connections: ~416k -> ~544k = +30.7%
> > 96 connections: ~331k -> ~508k = +53.6%
> 
> I would like to commit this patch for PostgreSQL 12, based on this
> report.  We know it helps performance on macOS developer machines and
> big FreeBSD servers, and it is the right kernel interface for the job
> on principle.

Seems reasonable.


> Matteo Beccati reported a 5-10% performance drop on a
> low-end Celeron NetBSD box which we have no explanation for, and we
> have no reports from server-class machines on that OS -- so perhaps we
> (or the NetBSD port?) should consider building with WAIT_USE_POLL on
> NetBSD until someone can figure out what needs to be fixed there
> (possibly on the NetBSD side)?

Yea, I'm not too worried about that. It'd be great to test that, but
otherwise I'm also ok to just plonk that into the template.

> @@ -576,6 +592,10 @@ CreateWaitEventSet(MemoryContext context, int nevents)
>  	if (fcntl(set->epoll_fd, F_SETFD, FD_CLOEXEC) == -1)
>  		elog(ERROR, "fcntl(F_SETFD) failed on epoll descriptor: %m");
>  #endif							/* EPOLL_CLOEXEC */
> +#elif defined(WAIT_USE_KQUEUE)
> +	set->kqueue_fd = kqueue();
> +	if (set->kqueue_fd < 0)
> +		elog(ERROR, "kqueue failed: %m");
>  #elif defined(WAIT_USE_WIN32)

Is this automatically opened with some FD_CLOEXEC equivalent?


> +static inline void
> +WaitEventAdjustKqueueAdd(struct kevent *k_ev, int filter, int action,
> +						 WaitEvent *event)
> +{
> +	k_ev->ident = event->fd;
> +	k_ev->filter = filter;
> +	k_ev->flags = action | EV_CLEAR;
> +	k_ev->fflags = 0;
> +	k_ev->data = 0;
> +
> +	/*
> +	 * On most BSD family systems, udata is of type void * so we could simply
> +	 * assign event to it without casting, or use the EV_SET macro instead of
> +	 * filling in the struct manually.  Unfortunately, NetBSD and possibly
> +	 * others have it as intptr_t, so here we wallpaper over that difference
> +	 * with an unsightly lvalue cast.
> +	 */
> +	*((WaitEvent **)(&k_ev->udata)) = event;

I'm mildly inclined to hide that behind a macro, so the other places
have a reference, via the macro definition, to this too.

> +	if (rc < 0 && event->events == WL_POSTMASTER_DEATH && errno == ESRCH)
> +	{
> +		/*
> +		 * The postmaster is already dead.  Defer reporting this to the caller
> +		 * until wait time, for compatibility with the other implementations.
> +		 * To do that we will now add the regular alive pipe.
> +		 */
> +		WaitEventAdjustKqueueAdd(&k_ev[0], EVFILT_READ, EV_ADD, event);
> +		rc = kevent(set->kqueue_fd, &k_ev[0], count, NULL, 0, NULL);
> +	}

That's, ... not particulary pretty. Kinda wonder if we shouldn't instead
just add a 'pending_events' field, that we can check at wait time.

> diff --git a/src/include/pg_config.h.in b/src/include/pg_config.h.in
> index 90dda8ea050..4bcabc3b381 100644
> --- a/src/include/pg_config.h.in
> +++ b/src/include/pg_config.h.in
> @@ -330,6 +330,9 @@
>  /* Define to 1 if you have isinf(). */
>  #undef HAVE_ISINF
>  
> +/* Define to 1 if you have the `kqueue' function. */
> +#undef HAVE_KQUEUE
> +
>  /* Define to 1 if you have the <langinfo.h> header file. */
>  #undef HAVE_LANGINFO_H
>  
> @@ -598,6 +601,9 @@
>  /* Define to 1 if you have the <sys/epoll.h> header file. */
>  #undef HAVE_SYS_EPOLL_H
>  
> +/* Define to 1 if you have the <sys/event.h> header file. */
> +#undef HAVE_SYS_EVENT_H
> +
>  /* Define to 1 if you have the <sys/ipc.h> header file. */
>  #undef HAVE_SYS_IPC_H

Should adjust pg_config.win32.h too.

Greetings,

Andres Freund