[PATCH] epollexclusive: avoid Ruby object allocation for buffer

[PATCH] epollexclusive: avoid Ruby object allocation for buffer

[Eric Wong]

Leaving a dangling Ruby object around is suboptimal since it
adds to GC pressure.

`__attribute__((__cleanup__(foo)))' is an old gcc extension to
provide automatic cleanup functionality by running a pre-declared
function at the end of scope.

gcc introduced this two decades ago and clang's had it for over
a decade.  Even tinycc supports it since 2019, and I expect it
to be easy to support in any C compiler since they're already
required to do cleanup for on-stack allocations.

So no, it's not standardized in C, but it makes life significantly
easier for C hackers.
---
 ext/unicorn_http/epollexclusive.h | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/ext/unicorn_http/epollexclusive.h b/ext/unicorn_http/epollexclusive.h
index 677e1fe..67a9ba6 100644
--- a/ext/unicorn_http/epollexclusive.h
+++ b/ext/unicorn_http/epollexclusive.h
@@ -78,6 +78,14 @@ static void *do_wait(void *ptr) /* runs w/o GVL */
 				epw->maxevents, epw->timeout_msec);
 }
 
+/* cleanup is supported since 2003 (gcc), 2009 (clang), tinycc: 2019 */
+#define AUTO_XFREE __attribute__((__cleanup__(cleanup_xfree)))
+static void cleanup_xfree(void *any)
+{
+	void **p = any;
+	xfree(*p);
+}
+
 /* :nodoc: */
 /* readers must not change between prepare_readers and get_readers */
 static VALUE
@@ -85,22 +93,18 @@ get_readers(VALUE epio, VALUE ready, VALUE readers, VALUE timeout_msec)
 {
 	struct ep_wait epw;
 	long i, n;
-	VALUE buf;
+	AUTO_XFREE void *buf = NULL;
 
 	Check_Type(ready, T_ARRAY);
 	Check_Type(readers, T_ARRAY);
 	epw.maxevents = RARRAY_LENINT(readers);
-	buf = rb_str_buf_new(sizeof(struct epoll_event) * epw.maxevents);
-	epw.events = (struct epoll_event *)RSTRING_PTR(buf);
+	epw.events = buf = ALLOC_N(struct epoll_event, epw.maxevents);
 	epio = rb_io_get_io(epio);
 	GetOpenFile(epio, epw.fptr);
 
 	epw.timeout_msec = NUM2INT(timeout_msec);
 	n = (long)rb_thread_call_without_gvl(do_wait, &epw, RUBY_UBF_IO, NULL);
-	if (n < 0) {
-		if (errno != EINTR) rb_sys_fail("epoll_wait");
-		n = 0;
-	}
+	if (n < 0 && errno != EINTR) rb_sys_fail("epoll_wait");
 	/* Linux delivers events in order received */
 	for (i = 0; i < n; i++) {
 		struct epoll_event *ev = &epw.events[i];
@@ -109,7 +113,6 @@ get_readers(VALUE epio, VALUE ready, VALUE readers, VALUE timeout_msec)
 		if (RTEST(obj))
 			rb_ary_push(ready, obj);
 	}
-	rb_str_resize(buf, 0);
 	return Qfalse;
 }
 #endif /* USE_EPOLL */

Re: [PATCH] epollexclusive: avoid Ruby object allocation for buffer

[Eric Wong]

Eric Wong <bofh@yhbt.net> wrote:
> Leaving a dangling Ruby object around is suboptimal since it
> adds to GC pressure.
> 
> `__attribute__((__cleanup__(foo)))' is an old gcc extension to
> provide automatic cleanup functionality by running a pre-declared
> function at the end of scope.

Unfortunately, this can't be usable due to exceptions.
Even if we avoid throwing exceptions ourselves; Thread#raise can
be called by a Rack app which spawns a background thread

>  	n = (long)rb_thread_call_without_gvl(do_wait, &epw, RUBY_UBF_IO, NULL);

Thread#raise can hit rb_thread_call_wihtout_gvl

> -	if (n < 0) {
> -		if (errno != EINTR) rb_sys_fail("epoll_wait");
> -		n = 0;
> -	}
> +	if (n < 0 && errno != EINTR) rb_sys_fail("epoll_wait");
>  	/* Linux delivers events in order received */
>  	for (i = 0; i < n; i++) {
>  		struct epoll_event *ev = &epw.events[i];
> @@ -109,7 +113,6 @@ get_readers(VALUE epio, VALUE ready, VALUE readers, VALUE timeout_msec)
>  		if (RTEST(obj))
>  			rb_ary_push(ready, obj);
>  	}

While the `for' loop can be moved ahead of the rb_sys_fail call
and limited in scope, Thread#raise can't be predicted.

I suppose rb_ensure works, but that's a PITA.

Re: [PATCH] epollexclusive: avoid Ruby object allocation for buffer

[Jean Boussier]

Interesting patch, I wonder if you have considered RB_ALLOCV_N, e.g:

https://patch-diff.githubusercontent.com/raw/Shopify/pitchfork/pull/37.patch