"GOT", but the "O" is a cute, smiling pufferfish. Index | Thread | Search

From:
Omar Polo <op@omarpolo.com>
Subject:
gotwebd: avoid calloc/free per fcgi record
To:
gameoftrees@openbsd.org
Date:
Fri, 29 Jul 2022 15:28:09 +0200

Download raw body.

Thread
to send something to the browser we have to go through
fcgi_send_response.

diff below uses a static buffer in fcgi_send_response (now
send_response) to avoid dynamically allocating ~16K for each bit of the
reply.

While here i'm also adding a wrapper around send_response to ensure that
we're not called with more than FCGI_CONTENT_SIZE.  This is impossible,
the maximum amount of data we can write in one go is when reading a blob
and it's currently 8K, but with that in place i can sleep well :)

I'm also removing the now unused struct fcgi_response.

ok?

diff /home/op/w/got
commit - 7375fc126e0f55289656336c6c8160c46efaba20
path + /home/op/w/got
blob - 845cfc6a1588e7b4e4d605f6892a16fb3ef5b601
file + gotwebd/fcgi.c
--- gotwebd/fcgi.c
+++ gotwebd/fcgi.c
@@ -39,7 +39,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 *, struct fcgi_response *);
+void	 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 *,
@@ -303,39 +303,13 @@ fcgi_timeout(int fd, short events, void *arg)
 int
 fcgi_gen_binary_response(struct request *c, const uint8_t *data, int len)
 {
-	struct fcgi_response *resp;
-	struct fcgi_record_header *header;
-	ssize_t n = 0;
-	int i;
-
 	if (c->sock->client_status == CLIENT_DISCONNECT)
 		return -1;
 
 	if (data == NULL || len == 0)
 		return 0;
 
-	if ((resp = calloc(1, sizeof(struct fcgi_response))) == NULL) {
-		log_warn("%s: cannot calloc fcgi_response", __func__);
-		return -1;
-	}
-
-	header = (struct fcgi_record_header*) resp->data;
-	header->version = 1;
-	header->type = FCGI_STDOUT;
-	header->id = htons(c->id);
-	header->padding_len = 0;
-	header->reserved = 0;
-
-	for (i = 0; i < len; i++) {
-		resp->data[i+8] = data[i];
-		n++;
-	}
-
-	header->content_len = htons(n);
-	resp->data_pos = 0;
-	resp->data_len = n + sizeof(struct fcgi_record_header);
-	fcgi_send_response(c, resp);
-
+	fcgi_send_response(c, FCGI_STDOUT, data, len);
 	return 0;
 }
 
@@ -347,40 +321,56 @@ fcgi_gen_response(struct request *c, const char *data)
 	return fcgi_gen_binary_response(c, data, strlen(data));
 }
 
-void
-fcgi_send_response(struct request *c, struct fcgi_response *resp)
+static void
+send_response(struct request *c, int type, const uint8_t *data,
+    size_t len)
 {
-	struct fcgi_record_header *header;
+	static uint8_t buf[FCGI_RECORD_SIZE];
+	struct fcgi_record_header header;
 	struct timespec ts;
-	size_t padded_len, off;
+	void *p = buf;
 	ssize_t nw;
+	size_t padded_len, tot;
 	int err = 0, th = 2000;
 
 	ts.tv_sec = 0;
 	ts.tv_nsec = 50;
 
-	header = (struct fcgi_record_header*)resp->data;
+	memset(&header, 0, sizeof(header));
+	header.version = 1;
+	header.type = type;
+	header.id = htons(c->id);
+	header.content_len = htons(len);
 
+	tot = sizeof(header) + len;
+
 	/* The FastCGI spec suggests to align the output buffer */
-	padded_len = FCGI_ALIGN(resp->data_len);
-	if (padded_len > resp->data_len) {
-		/* There should always be FCGI_PADDING_SIZE bytes left */
-		if (padded_len > FCGI_RECORD_SIZE)
-			log_warn("response too long");
-		header->padding_len = padded_len - resp->data_len;
-		resp->data_len = padded_len;
+	padded_len = FCGI_ALIGN(tot);
+	if (padded_len > tot) {
+		header.padding_len = padded_len - tot;
+		tot = padded_len;
 	}
 
-	dump_fcgi_record("resp ", header);
+	memcpy(p, &header, sizeof(header));
+	p += sizeof(header);
 
+	memcpy(p, data, len);
+	p += len;
+
+	if (header.padding_len != 0)
+		memset(p, 0, header.padding_len);
+
+	dump_fcgi_record("resp ", &header);
+
 	/*
 	 * XXX: add some simple write heuristics here
 	 * On slower VMs, spotty connections, etc., we don't want to go right to
 	 * disconnect. Let's at least try to write the data a few times before
 	 * giving up.
 	 */
-	for (off = 0; off < resp->data_len; off += nw) {
-		nw = write(c->fd, resp->data + off, resp->data_len - off);
+	p = buf;
+	while (tot > 0) {
+		nw = write(c->fd, p, tot);
 		if (nw == 0) {
 			c->sock->client_status = CLIENT_DISCONNECT;
 			break;
@@ -388,7 +378,6 @@ fcgi_send_response(struct request *c, struct fcgi_resp
 		if (nw == -1) {
 			err++;
 			if (errno == EAGAIN && err < th) {
-				nw = 0;
 				nanosleep(&ts, NULL);
 				continue;
 			}
@@ -397,44 +386,45 @@ fcgi_send_response(struct request *c, struct fcgi_resp
 			break;
 		}
 
-		if (nw != resp->data_len - off)
-			log_debug("%s: partial write: %zu vs %zu", __func__, nw,
-			    resp->data_len - off);
+		if (nw != tot)
+			log_debug("%s: partial write: %zu vs %zu", __func__,
+			    nw, tot);
+
+		p += nw;
+		tot -= nw;
 	}
+}
 
-	free(resp);
+void
+fcgi_send_response(struct request *c, int type, const void *data,
+    size_t len)
+{
+	while (len > FCGI_CONTENT_SIZE) {
+		send_response(c, type, data, len);
+		if (c->sock->client_status == CLIENT_DISCONNECT)
+			return;
+
+		data += FCGI_CONTENT_SIZE;
+		len -= FCGI_CONTENT_SIZE;
+	}
+
+	if (len == 0)
+		return;
+
+	send_response(c, type, data, len);
 }
 
 void
 fcgi_create_end_record(struct request *c)
 {
-	struct fcgi_response *resp;
-	struct fcgi_record_header *header;
-	struct fcgi_end_request_body *end_request;
+	struct fcgi_end_request_body end_request;
 
-	if ((resp = calloc(1, sizeof(struct fcgi_response))) == NULL) {
-		log_warn("cannot calloc fcgi_response");
-		return;
-	}
-	header = (struct fcgi_record_header*) resp->data;
-	header->version = 1;
-	header->type = FCGI_END_REQUEST;
-	header->id = htons(c->id);
-	header->content_len = htons(sizeof(struct
-	    fcgi_end_request_body));
-	header->padding_len = 0;
-	header->reserved = 0;
-	end_request = (struct fcgi_end_request_body *) (resp->data +
-	    sizeof(struct fcgi_record_header));
-	end_request->app_status = htonl(0); /* script_status */
-	end_request->protocol_status = FCGI_REQUEST_COMPLETE;
-	end_request->reserved[0] = 0;
-	end_request->reserved[1] = 0;
-	end_request->reserved[2] = 0;
-	resp->data_pos = 0;
-	resp->data_len = sizeof(struct fcgi_end_request_body) +
-	    sizeof(struct fcgi_record_header);
-	fcgi_send_response(c, resp);
+	memset(&end_request, 0, sizeof(end_request));
+	end_request.app_status = htonl(0); /* script status */
+	end_request.protocol_status = FCGI_REQUEST_COMPLETE;
+
+	fcgi_send_response(c, FCGI_END_REQUEST, &end_request,
+	    sizeof(end_request));
 }
 
 void
blob - 789ae140f9fb7ff7c3be4252259d46cc4164ec0f
file + gotwebd/gotwebd.h
--- gotwebd/gotwebd.h
+++ gotwebd/gotwebd.h
@@ -134,13 +134,6 @@ struct fcgi_record_header {
 	uint8_t		reserved;
 }__attribute__((__packed__));
 
-struct fcgi_response {
-	TAILQ_ENTRY(fcgi_response)	entry;
-	uint8_t				data[FCGI_RECORD_SIZE];
-	size_t				data_pos;
-	size_t				data_len;
-};
-
 struct repo_dir {
 	char			*name;
 	char			*owner;