From: Omar Polo <op@omarpolo.com>
Subject: gotwebd: add some output buffering
To: gameoftrees@openbsd.org
Cc: Tracey Emery <tracey@traceyemery.net>
Date: Sun, 07 Aug 2022 12:32:19 +0200

grepping for "fcgi_gen_response" shows that gotwebd makes a lot of
calls to it with small strings.  Each call translate to a fastcgi
record and it's quite wasteful, so why don't add some buffering?

Diff below adds a per-request output buffer where to accumulate some
data.  The actual buffering is done in fcgi_gen_binary_response.

There are two use of fcgi_gen{_binary,}_response: one (the most
common) to send very small amounts of data very often and a second one
to send a big chunk in one go (when serving blobs.)  Buffering makes
sense mostly in the first case, so i'm trying to detect the second and
bypass the buffer there.

The size of the buffer is important: a buffer too small defeats the
point of buffering, but a buffer too large will delay the reply and
make gotwebd feel slower.  I went with 1K which seems to work fine in
my case: it reduces greatly the number of fcgi records sent but still
"feels" fast to reply.

(I'm also making fcgi_send_response failures "sticky", so that it's
safe to call multiple times fcgi_send_response.)

I'm running my gotwebd instance with this applied too.  some
unscientific testing seems to show that the page load time improved
(comparing ten runs of curl -s $url >/dev/null before and after.)


diff /home/op/w/got
commit - 14aa6a729393403e45e3c78a2224d1c323fe0c06
path + /home/op/w/got
blob - f0de1b24e2bf33d38244b293dc7316561566af60
file + gotwebd/fcgi.c
--- gotwebd/fcgi.c
+++ gotwebd/fcgi.c
@@ -40,7 +40,7 @@ size_t	 fcgi_parse_record(uint8_t *, size_t, struct re
 void	 fcgi_parse_begin_request(uint8_t *, uint16_t, struct request *,
 	    uint16_t);
 void	 fcgi_parse_params(uint8_t *, uint16_t, struct request *, uint16_t);
-void	 fcgi_send_response(struct request *, int, const void *, size_t);
+int	 fcgi_send_response(struct request *, int, const void *, size_t);
 
 void	 dump_fcgi_record_header(const char *, struct fcgi_record_header *);
 void	 dump_fcgi_begin_request_body(const char *,
@@ -137,6 +137,12 @@ fcgi_parse_record(uint8_t *buf, size_t n, struct reque
 		break;
 	case FCGI_STDIN:
 	case FCGI_ABORT_REQUEST:
+		if (c->sock->client_status != CLIENT_DISCONNECT &&
+		    c->outbuf_len != 0) {
+			fcgi_send_response(c, FCGI_STDOUT, c->outbuf,
+			    c->outbuf_len);
+		}
+
 		fcgi_create_end_record(c);
 		fcgi_cleanup_request(c);
 		return 0;
@@ -304,13 +310,39 @@ fcgi_timeout(int fd, short events, void *arg)
 int
 fcgi_gen_binary_response(struct request *c, const uint8_t *data, int len)
 {
+	int r;
+
 	if (c->sock->client_status == CLIENT_DISCONNECT)
 		return -1;
 
 	if (data == NULL || len == 0)
 		return 0;
 
-	fcgi_send_response(c, FCGI_STDOUT, data, len);
+	/*
+	 * special case: send big replies -like blobs- directly
+	 * without copying.
+	 */
+	if (len > sizeof(c->outbuf)) {
+		if (c->outbuf_len > 0) {
+			fcgi_send_response(c, FCGI_STDOUT,
+			    c->outbuf, c->outbuf_len);
+			c->outbuf_len = 0;
+		}
+		return fcgi_send_response(c, FCGI_STDOUT, data, len);
+	}
+
+	if (len < sizeof(c->outbuf) - c->outbuf_len) {
+		memcpy(c->outbuf + c->outbuf_len, data, len);
+		c->outbuf_len += len;
+		return 0;
+	}
+
+	r = fcgi_send_response(c, FCGI_STDOUT, c->outbuf, c->outbuf_len);
+	if (r == -1)
+		return -1;
+
+	memcpy(c->outbuf, data, len);
+	c->outbuf_len = len;
 	return 0;
 }
 
@@ -322,7 +354,7 @@ fcgi_gen_response(struct request *c, const char *data)
 	return fcgi_gen_binary_response(c, data, strlen(data));
 }
 
-static void
+static int
 send_response(struct request *c, int type, const uint8_t *data,
     size_t len)
 {
@@ -382,7 +414,7 @@ send_response(struct request *c, int type, const uint8
 			}
 			log_warn("%s: write failure", __func__);
 			c->sock->client_status = CLIENT_DISCONNECT;
-			break;
+			return -1;
 		}
 
 		if (nw != tot)
@@ -400,25 +432,29 @@ send_response(struct request *c, int type, const uint8
 			iov[i].iov_len = 0;
 		}
 	}
+
+	return 0;
 }
 
-void
+int
 fcgi_send_response(struct request *c, int type, const void *data,
     size_t len)
 {
+	if (c->sock->client_status == CLIENT_DISCONNECT)
+		return -1;
+
 	while (len > FCGI_CONTENT_SIZE) {
-		send_response(c, type, data, len);
-		if (c->sock->client_status == CLIENT_DISCONNECT)
-			return;
+		if (send_response(c, type, data, len) == -1)
+			return -1;
 
 		data += FCGI_CONTENT_SIZE;
 		len -= FCGI_CONTENT_SIZE;
 	}
 
 	if (len == 0)
-		return;
+		return 0;
 
-	send_response(c, type, data, len);
+	return send_response(c, type, data, len);
 }
 
 void
blob - b164df949130a41810c6bd822a285e903027709d
file + gotwebd/gotwebd.h
--- gotwebd/gotwebd.h
+++ gotwebd/gotwebd.h
@@ -213,6 +213,9 @@ struct request {
 	size_t				 buf_pos;
 	size_t				 buf_len;
 
+	uint8_t				 outbuf[1024];
+	size_t				 outbuf_len;
+
 	char				 querystring[MAX_QUERYSTRING];
 	char				 http_host[GOTWEBD_MAXTEXT];
 	char				 document_root[MAX_DOCUMENT_ROOT];