From: Tracey Emery <tracey@traceyemery.net>
Subject: Re: gotwebd: add some output buffering
To: Omar Polo <op@omarpolo.com>
Cc: gameoftrees@openbsd.org
Date: Tue, 9 Aug 2022 07:49:45 -0600

On Sun, Aug 07, 2022 at 12:32:19PM +0200, Omar Polo wrote:
> 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];

Please make a define for 1024 so there are no magic numbers in the
struct definition. Otherwise thanks and ok!

> +	size_t				 outbuf_len;
> +
>  	char				 querystring[MAX_QUERYSTRING];
>  	char				 http_host[GOTWEBD_MAXTEXT];
>  	char				 document_root[MAX_DOCUMENT_ROOT];

-- 

Tracey Emery