use a malloc'd buffer with memcpy in the inner loop of FMT_Format,

rather than a VSB

src/format.c

@@ -34,6 +34,7 @@
 #include <errno.h>
 #include <ctype.h>
 #include <stdint.h>
+#include <limits.h>
 
 #include "vapi/vsl.h"
 #include "vas.h"
@@ -45,16 +46,23 @@
 #include "format.h"
 #include "strfTIM.h"
 
+#ifdef PAGE_SIZE
+#define OBUF_SIZE PAGE_SIZE
+#else
+#define OBUF_SIZE 4096
+#endif
+
 typedef struct compiled_fmt_t {
     char **str;
     formatter_f **formatter;
     arg_t *args;
+    int *strlen;
     unsigned n;
 } compiled_fmt_t;
 
 static struct vsb *scratch;
 
-static char *payload;
+static char *payload, *obuf;
 static char empty[] = "";
 static char hit[] = "hit";
 static char miss[] = "miss";
@@ -64,6 +72,8 @@ static char error[] = "error";
 static char dash[] = "-";
 static char buf[BUFSIZ];
 
+static size_t obuf_sz = OBUF_SIZE;
+
 typedef struct inc_t {
     char *hdr;
     VSTAILQ_ENTRY(inc_t) inclist;
@@ -754,17 +764,24 @@ static void
 add_fmt(const compiled_fmt_t *fmt, struct vsb *os, unsigned n,
         formatter_f formatter, const char *name, enum VSL_tag_e tag, int fld)
 {
-    fmt->str[n] = (char *) malloc(VSB_len(os) + 1);
-    AN(fmt->str[n]);
+    if (VSB_len(os) > 0) {
+        fmt->str[n] = (char *) malloc(VSB_len(os) + 1);
+        AN(fmt->str[n]);
+        VSB_finish(os);
+        strcpy(fmt->str[n], VSB_data(os));
+        fmt->strlen[n] = VSB_len(os);
+    }
+    else {
+        fmt->str[n] = NULL;
+        fmt->strlen[n] = 0;
+    }
+    VSB_clear(os);
     if (name == NULL)
         fmt->args[n].name = NULL;
     else {
         fmt->args[n].name = strdup(name);
         AN(fmt->args[n].name);
     }
-    VSB_finish(os);
-    strcpy(fmt->str[n], VSB_data(os));
-    VSB_clear(os);
     fmt->formatter[n] = formatter;
     fmt->args[n].tag = tag;
     fmt->args[n].fld = fld;
@@ -922,6 +939,11 @@ compile_fmt(char * const format, compiled_fmt_t * const fmt,
         strcpy(err, strerror(errno));
         return 0;
     }
+    fmt->strlen = (int *) calloc(n, sizeof(int));
+    if (fmt->strlen == NULL) {
+        strcpy(err, strerror(errno));
+        return 0;
+    }
     for (int i = 0; i < n; i++)
         fmt->args[i].hdr_idx = -1;
     *hdrtbl = calloc(n, sizeof(char *));
@@ -1332,6 +1354,9 @@ FMT_Init(char *err)
     int idx = 0;
     char **chdrtbl = NULL, **bhdrtbl = NULL, **rhdrtbl = NULL;
 
+    obuf = calloc(OBUF_SIZE, sizeof(char));
+    if (obuf == NULL)
+        return ENOMEM;
     payload = malloc(config.max_reclen);
     if (payload == NULL)
         return ENOMEM;
@@ -1427,10 +1452,22 @@ FMT_Estimate_RecsPerTx(void)
     return recs_per_tx;
 }
 
-void
-FMT_Format(tx_t *tx, struct vsb *os)
+static inline void
+fmt_resize(size_t curlen)
+{
+    if (curlen > obuf_sz) {
+        obuf_sz >>= 1;
+        obuf = realloc(obuf, obuf_sz);
+        AN(obuf);
+    }
+}
+
+char *
+FMT_Format(tx_t *tx)
 {
     compiled_fmt_t fmt;
+    char *p = obuf;
+    size_t curlen = 0;
 
     CHECK_OBJ_NOTNULL(tx, TX_MAGIC);
     assert(tx->state == TX_SUBMITTED);
@@ -1451,21 +1488,33 @@ FMT_Format(tx_t *tx, struct vsb *os)
 
     tx->state = TX_FORMATTING;
 
+    *p = '\0';
     for (int i = 0; i < fmt.n; i++) {
         char *s = NULL;
         size_t len = 0;
 
-        if (fmt.str[i] != NULL)
-            VSB_cat(os, fmt.str[i]);
+        if (fmt.str[i] != NULL) {
+            curlen += fmt.strlen[i];
+            fmt_resize(curlen);
+            memcpy(p, fmt.str[i], fmt.strlen[i]);
+            p += fmt.strlen[i];
+        }
         if (fmt.formatter[i] != NULL) {
             (fmt.formatter[i])(tx, &fmt.args[i], &s, &len);
-            if (s != NULL && len != 0)
-                VSB_bcat(os, s, len);
+            if (s != NULL && len != 0) {
+                curlen += len;
+                fmt_resize(curlen);
+                memcpy(p, s, len);
+                p += len;
+            }
         }
     }
+    *p = '\0';
 
     assert(tx->state == TX_FORMATTING);
     tx->state = TX_WRITTEN;
+
+    return obuf;
 }
 
 static void
@@ -1491,6 +1540,7 @@ free_format(compiled_fmt_t *fmt)
             free(fmt->args[i].name);
     }
     free(fmt->str);
+    free(fmt->strlen);
     free(fmt->formatter);
     free(fmt->args);
 }
@@ -1511,6 +1561,7 @@ free_incl(includehead_t inclhead[])
 void
 FMT_Fini(void)
 {
+    free(obuf);
     VSB_delete(scratch);
     free(payload);
 

src/test/test_format.c

@@ -1869,13 +1869,12 @@ static const char
 *test_FMT_interface(void)
 {
 #define NTAGS 25
-    char err[BUFSIZ], strftime_s[BUFSIZ];
+    char err[BUFSIZ], strftime_s[BUFSIZ], *os;
     int status;
     tx_t tx;
     rec_node_t node[NTAGS], *nptr[NTAGS];
     rec_t rec[NTAGS];
     chunk_t c[NTAGS];
-    struct vsb *os;
     struct tm *tm;
     time_t t = 1427743146;
 
@@ -1921,8 +1920,6 @@ static const char
             break;
         }
 
-    os = VSB_new_auto();
-    MAN(os);
 
     init_tx(&tx, node, nptr);
     tx.state = TX_SUBMITTED;
@@ -1945,19 +1942,16 @@ static const char
     add_record_data(&tx, SLT_RespStatus, &rec[9],  &c[9], "200");
     add_record_data(&tx, SLT_ReqAcct,    &rec[10], &c[10], REQACCT_PAYLOAD);
 
-    FMT_Format(&tx, os);
-    VSB_finish(os);
+    os = FMT_Format(&tx);
 #define EXP_DEFAULT_OUTPUT "127.0.0.1 - varnish [%d/%b/%Y:%T %z] "\
         "\"GET http://bazquux.com/foo HTTP/1.1\" 200 105 "\
         "\"http://foobar.com/\" \"Mozilla\"\n"
     tm = localtime(&t);
     MAN(strftime(strftime_s, BUFSIZ, EXP_DEFAULT_OUTPUT, tm));
-    VMASSERT(strcmp(VSB_data(os), strftime_s) == 0, "'%s' != '%s'",
-             VSB_data(os), strftime_s);
+    VMASSERT(strcmp(os, strftime_s) == 0, "'%s' != '%s'", os, strftime_s);
 
     /* Client format with all formatters */
     FMT_Fini();
-    VSB_clear(os);
 
 #define FULL_CLIENT_FMT "%b %d %D %H %h %I %{Foo}i %{Bar}o %l %m %O %q %r %s "\
         "%t %T %{%F-%T.%i}t %U %u %{Varnish:time_firstbyte}x "\
@@ -2030,8 +2024,7 @@ static const char
         }
 
     setup_full_client_tx(&tx, node, nptr, rec, c);
-    FMT_Format(&tx, os);
-    VSB_finish(os);
+    os = FMT_Format(&tx);
 #define EXP_FULL_CLIENT_OUTPUT "105 c 15963 HTTP/1.1 127.0.0.1 60 foohdr "\
         "barhdr - GET 283 bar=baz&quux=wilco GET "\
         "http://foobar.com/foo?bar=baz&quux=wilco HTTP/1.1 200 "\
@@ -2040,12 +2033,10 @@ static const char
         "1429213569.602005 0.000000 0.000000 60 0.000125 4711 1147\n"
     tm = localtime(&t);
     MAN(strftime(strftime_s, BUFSIZ, EXP_FULL_CLIENT_OUTPUT, tm));
-    VMASSERT(strcmp(VSB_data(os), strftime_s) == 0, "'%s' != '%s'",
-             VSB_data(os), strftime_s);
+    VMASSERT(strcmp(os, strftime_s) == 0, "'%s' != '%s'", os, strftime_s);
 
     /* Backend format with all formatters */
     FMT_Fini();
-    VSB_clear(os);
 
 #define FULL_BACKEND_FMT "%b %d %D %H %h %I %{Foo}i %{Bar}o %l %m %O %q %r %s "\
         "%t %T %{%F-%T.%i}t %U %u %{Varnish:time_firstbyte}x %{VCL_Log:baz}x "\
@@ -2109,8 +2100,7 @@ static const char
         }
 
     setup_full_backend_tx(&tx, node, nptr, rec, c);
-    FMT_Format(&tx, os);
-    VSB_finish(os);
+    os = FMT_Format(&tx);
 #define EXP_FULL_BACKEND_OUTPUT "105 b 15703 HTTP/1.1 default(127.0.0.1,,80) "\
         "283 foohdr barhdr - GET 60 bar=baz&quux=wilco GET "\
         "http://foobar.com/foo?bar=baz&quux=wilco HTTP/1.1 200 "\
@@ -2119,12 +2109,10 @@ static const char
         "1429210777.728290 0.000048 0.000048 283 0.000048 4711 1147\n"
     tm = localtime(&t);
     MAN(strftime(strftime_s, BUFSIZ, EXP_FULL_BACKEND_OUTPUT, tm));
-    VMASSERT(strcmp(VSB_data(os), strftime_s) == 0, "'%s' != '%s'",
-             VSB_data(os), strftime_s);
+    VMASSERT(strcmp(os, strftime_s) == 0, "'%s' != '%s'", os, strftime_s);
 
     /* Both backend and client formats */
     FMT_Fini();
-    VSB_clear(os);
 
     VSB_clear(config.cformat);
     VSB_cpy(config.cformat, FULL_CLIENT_FMT);
@@ -2225,25 +2213,19 @@ static const char
         }
 
     setup_full_client_tx(&tx, node, nptr, rec, c);
-    FMT_Format(&tx, os);
-    VSB_finish(os);
+    os = FMT_Format(&tx);
     tm = localtime(&t);
     MAN(strftime(strftime_s, BUFSIZ, EXP_FULL_CLIENT_OUTPUT, tm));
-    VMASSERT(strcmp(VSB_data(os), strftime_s) == 0, "'%s' != '%s'",
-             VSB_data(os), strftime_s);
+    VMASSERT(strcmp(os, strftime_s) == 0, "'%s' != '%s'", os, strftime_s);
 
     setup_full_backend_tx(&tx, node, nptr, rec, c);
-    VSB_clear(os);
-    FMT_Format(&tx, os);
-    VSB_finish(os);
+    os = FMT_Format(&tx);
     tm = localtime(&t);
     MAN(strftime(strftime_s, BUFSIZ, EXP_FULL_BACKEND_OUTPUT, tm));
-    VMASSERT(strcmp(VSB_data(os), strftime_s) == 0, "'%s' != '%s'",
-             VSB_data(os), strftime_s);
+    VMASSERT(strcmp(os, strftime_s) == 0, "'%s' != '%s'", os, strftime_s);
 
     /* Raw format */
     FMT_Fini();
-    VSB_clear(os);
 
 #define FULL_RAW_FMT "%t %{%F-%T.%i}t %{tag:Backend_health}x %{vxid}x"
     VSB_clear(config.rformat);
@@ -2274,14 +2256,12 @@ static const char
 #define HEALTH_PAYLOAD "b Still healthy 4--X-RH 5 4 5 0.032728 0.035774 " \
         "HTTP/1.1 200 OK"
     add_record_data(&tx, SLT_Backend_health, &rec[0],  &c[0], HEALTH_PAYLOAD);
-    FMT_Format(&tx, os);
-    VSB_finish(os);
+    os = FMT_Format(&tx);
 #define EXP_FULL_RAW_OUTPUT "[%d/%b/%Y:%T %z] %F-%T.529143 "\
         "b Still healthy 4--X-RH 5 4 5 0.032728 0.035774 HTTP/1.1 200 OK 4711\n"
     tm = localtime(&t);
     MAN(strftime(strftime_s, BUFSIZ, EXP_FULL_RAW_OUTPUT, tm));
-    VMASSERT(strcmp(VSB_data(os), strftime_s) == 0, "'%s' != '%s'",
-             VSB_data(os), strftime_s);
+    VMASSERT(strcmp(os, strftime_s) == 0, "'%s' != '%s'", os, strftime_s);
 
     /* Illegal backend formats */
     FMT_Fini();

src/varnishevent.h

@@ -284,7 +284,7 @@ void MON_Output(void);
 int FMT_Init(char *err);
 int FMT_GetMaxIdx(void);
 int FMT_Estimate_RecsPerTx(void);
-void FMT_Format(tx_t *tx, struct vsb *os);
+char *FMT_Format(tx_t *tx);
 void FMT_Fini(void);
 
 /* handler.c */

src/writer.c

@@ -74,8 +74,6 @@ chunkhead_t wrt_freechunks;
 
 static unsigned	wrt_nfree_tx, wrt_nfree_recs, wrt_nfree_chunks;
 
-static struct vsb *os;
-
 static FILE *fo;
 static struct pollfd fds[1];
 static int blocking = 0, timeout = -1;
@@ -178,6 +176,7 @@ wrt_return_freelist(void)
 void
 wrt_write(tx_t *tx)
 {
+    char *os;
     int ready = 1;
     
     CHECK_OBJ_NOTNULL(tx, TX_MAGIC);
@@ -205,10 +204,8 @@ wrt_write(tx_t *tx)
     }
     AZ(pthread_mutex_unlock(&reopen_lock));
 
-    VSB_clear(os);
     VRMB();
-    FMT_Format(tx, os);
-    VSB_finish(os);
+    os = FMT_Format(tx);
     assert(tx->state == TX_WRITTEN);
 
     if (blocking) {
@@ -226,14 +223,13 @@ wrt_write(tx_t *tx)
         if (fds[0].revents & POLLERR) {
             LOG_Log(LOG_ERR,
                     "Error waiting for ready output %d (%s), "
-                    "DATA DISCARDED: %s", errno, strerror(errno), VSB_data(os));
+                    "DATA DISCARDED: %s", errno, strerror(errno), os);
             errors++;
         }
         else if (nfds == 0) {
             wrt_return_freelist();
             LOG_Log(LOG_ERR,
-                    "Timeout waiting for ready output, DATA DISCARDED: %s",
-                    VSB_data(os));
+                    "Timeout waiting for ready output, DATA DISCARDED: %s", os);
             timeouts++;
         }
         else if (nfds != 1)
@@ -245,16 +241,16 @@ wrt_write(tx_t *tx)
     }
     if (ready) {
         double start = VTIM_mono();
-        int ret = fprintf(fo, "%s", VSB_data(os));
+        int ret = fprintf(fo, "%s", os);
         writet += VTIM_mono() - start;
         if (ret < 0) {
             LOG_Log(LOG_ERR, "Output error %d (%s), DATA DISCARDED: %s",
-                    errno, strerror(errno), VSB_data(os));
+                    errno, strerror(errno), os);
             errors++;
         }
         else {
             writes++;
-            bytes += VSB_len(os);
+            bytes += strlen(os);
         }
     }
 
@@ -349,9 +345,6 @@ WRT_Init(void)
     wrt_data.magic = WRITER_DATA_MAGIC;
     wrt_data.state = WRT_NOTSTARTED;
 
-    /* XXX: fixed size? */
-    os = VSB_new_auto();
-
     if (config.output_timeout != 0.)
         timeout = config.output_timeout * 1e3;
 
@@ -425,6 +418,5 @@ WRT_Fini(void)
     AZ(run);
     fclose(fo);
     free(obuf);
-    VSB_delete(os);
     AZ(pthread_mutex_destroy(&reopen_lock));
 }