remove inline_ascii_response option

Has defaulted to false since 1.5.0, and with -o modern for a few years
before that. Performance is fine, no reported bugs. Always was the
intention. Code is simpler without the options.

7 files changed, 32 insertions, 119 deletions

diff --git a/doc/protocol.txt b/doc/protocol.txt
index b569bc7..81025b3 100644
--- a/doc/protocol.txt
+++ b/doc/protocol.txt
@@ -863,9 +863,7 @@ other stats command.
 | track_sizes       | bool     | If yes, a "stats sizes" histogram is being   |
 |                   |          | dynamically tracked.                         |
 | inline_ascii_response                                                       |
-|                   | bool     | If yes, stores numbers from VALUE response   |
-|                   |          | inside an item, using up to 24 bytes.        |
-|                   |          | Small slowdown for ASCII get, faster sets.   |
+|                   | bool     | Does nothing as of 1.5.15                    |
 | drop_privileges   | bool     | If yes, and available, drop unused syscalls  |
 |                   |          | (see seccomp on Linux, pledge on OpenBSD)    |
 |-------------------+----------+----------------------------------------------|
diff --git a/items.c b/items.c
index d4ce5a1..408f394 100644
--- a/items.c
+++ b/items.c
@@ -168,15 +168,10 @@ unsigned int do_get_lru_size(uint32_t id) {
  */
 static size_t item_make_header(const uint8_t nkey, const unsigned int flags, const int nbytes,
                      char *suffix, uint8_t *nsuffix) {
-    if (settings.inline_ascii_response) {
-        /* suffix is defined at 40 chars elsewhere.. */
-        *nsuffix = (uint8_t) snprintf(suffix, 40, " %u %d\r\n", flags, nbytes - 2);
+    if (flags == 0) {
+        *nsuffix = 0;
     } else {
-        if (flags == 0) {
-            *nsuffix = 0;
-        } else {
-            *nsuffix = sizeof(flags);
-        }
+        *nsuffix = sizeof(flags);
     }
     return sizeof(item) + nkey + *nsuffix + nbytes;
 }
@@ -334,11 +329,7 @@ item *do_item_alloc(char *key, const size_t nkey, const unsigned int flags,
     it->nbytes = nbytes;
     memcpy(ITEM_key(it), key, nkey);
     it->exptime = exptime;
-    if (settings.inline_ascii_response) {
-        memcpy(ITEM_suffix(it), suffix, (size_t)nsuffix);
-    } else if (nsuffix > 0) {
-        memcpy(ITEM_suffix(it), &flags, sizeof(flags));
-    }
+    memcpy(ITEM_suffix(it), &flags, sizeof(flags));
     it->nsuffix = nsuffix;
 
     /* Initialize internal chunk. */
diff --git a/memcached.c b/memcached.c
index f88aeac..2acec4f 100644
--- a/memcached.c
+++ b/memcached.c
@@ -300,7 +300,6 @@ static void settings_init(void) {
     settings.warm_lru_pct = 40;
     settings.hot_max_factor = 0.2;
     settings.warm_max_factor = 2.0;
-    settings.inline_ascii_response = false;
     settings.temp_lru = false;
     settings.temporary_ttl = 61;
     settings.idle_timeout = 0; /* disabled */
@@ -1756,7 +1755,7 @@ static void process_bin_get_or_touch(conn *c) {
         rsp->message.header.response.cas = htonll(ITEM_get_cas(it));
 
         // add the flags
-        FLAGS_CONV(settings.inline_ascii_response, it, rsp->message.body.flags);
+        FLAGS_CONV(it, rsp->message.body.flags);
         rsp->message.body.flags = htonl(rsp->message.body.flags);
         add_iov(c, &rsp->message.body, sizeof(rsp->message.body));
 
@@ -2999,7 +2998,7 @@ enum store_item_type do_store_item(item *it, int comm, conn *c, const uint32_t h
             if (stored == NOT_STORED) {
                 /* we have it and old_it here - alloc memory to hold both */
                 /* flags was already lost - so recover them from ITEM_suffix(it) */
-                FLAGS_CONV(settings.inline_ascii_response, old_it, flags);
+                FLAGS_CONV(old_it, flags);
                 new_it = do_item_alloc(key, it->nkey, flags, old_it->exptime, it->nbytes + old_it->nbytes - 2 /* CRLF */);
 
                 /* copy data from it and old_it to new_it */
@@ -3386,7 +3385,7 @@ static void process_stat_settings(ADD_STAT add_stats, void *c) {
     APPEND_STAT("watcher_logbuf_size", "%u", settings.logger_watcher_buf_size);
     APPEND_STAT("worker_logbuf_size", "%u", settings.logger_buf_size);
     APPEND_STAT("track_sizes", "%s", item_stats_sizes_status() ? "yes" : "no");
-    APPEND_STAT("inline_ascii_response", "%s", settings.inline_ascii_response ? "yes" : "no");
+    APPEND_STAT("inline_ascii_response", "%s", "no"); // setting is dead, cannot be yes.
 #ifdef HAVE_DROP_PRIVILEGES
     APPEND_STAT("drop_privileges", "%s", settings.drop_privileges ? "yes" : "no");
 #endif
@@ -3637,24 +3636,22 @@ static void process_stat(conn *c, token_t *tokens, const size_t ntokens) {
 /* nsuffix == 0 means use no storage for client flags */
 static inline int make_ascii_get_suffix(char *suffix, item *it, bool return_cas, int nbytes) {
     char *p = suffix;
-    if (!settings.inline_ascii_response) {
-        *p = ' ';
+    *p = ' ';
+    p++;
+    if (it->nsuffix == 0) {
+        *p = '0';
         p++;
-        if (it->nsuffix == 0) {
-            *p = '0';
-            p++;
-        } else {
-            p = itoa_u32(*((uint32_t *) ITEM_suffix(it)), p);
-        }
-        *p = ' ';
-        p = itoa_u32(nbytes-2, p+1);
     } else {
-        p = suffix;
+        p = itoa_u32(*((uint32_t *) ITEM_suffix(it)), p);
     }
+    *p = ' ';
+    p = itoa_u32(nbytes-2, p+1);
+
     if (return_cas) {
         *p = ' ';
         p = itoa_u64(ITEM_get_cas(it), p+1);
     }
+
     *p = '\r';
     *(p+1) = '\n';
     *(p+2) = '\0';
@@ -3831,7 +3828,7 @@ static inline int _get_extstore(conn *c, item *it, int iovst, int iovcnt) {
     if (ntotal > settings.slab_chunk_size_max) {
         // Pull a chunked item header.
         uint32_t flags;
-        FLAGS_CONV(settings.inline_ascii_response, it, flags);
+        FLAGS_CONV(it, flags);
         new_it = item_alloc(ITEM_key(it), it->nkey, flags, it->exptime, it->nbytes);
         assert(new_it == NULL || (new_it->it_flags & ITEM_CHUNKED));
         chunked = true;
@@ -3979,7 +3976,6 @@ static inline void process_get_command(conn *c, token_t *tokens, size_t ntokens,
                  *   " " + flags + " " + data length + "\r\n" + data (with \r\n)
                  */
 
-                if (return_cas || !settings.inline_ascii_response)
                 {
                   MEMCACHED_COMMAND_GET(c->sfd, ITEM_key(it), it->nkey,
                                         it->nbytes, ITEM_get_cas(it));
@@ -3994,7 +3990,6 @@ static inline void process_get_command(conn *c, token_t *tokens, size_t ntokens,
                   int suffix_len = make_ascii_get_suffix(suffix, it, return_cas, nbytes);
                   if (add_iov(c, "VALUE ", 6) != 0 ||
                       add_iov(c, ITEM_key(it), it->nkey) != 0 ||
-                      (settings.inline_ascii_response && add_iov(c, ITEM_suffix(it), it->nsuffix - 2) != 0) ||
                       add_iov(c, suffix, suffix_len) != 0)
                       {
                           item_remove(it);
@@ -4020,30 +4015,6 @@ static inline void process_get_command(conn *c, token_t *tokens, size_t ntokens,
                       goto stop;
                   }
                 }
-                else
-                {
-                  MEMCACHED_COMMAND_GET(c->sfd, ITEM_key(it), it->nkey,
-                                        it->nbytes, ITEM_get_cas(it));
-                  if (add_iov(c, "VALUE ", 6) != 0 ||
-                      add_iov(c, ITEM_key(it), it->nkey) != 0)
-                      {
-                          item_remove(it);
-                          goto stop;
-                      }
-                  if ((it->it_flags & ITEM_CHUNKED) == 0)
-                      {
-                          if (add_iov(c, ITEM_suffix(it), it->nsuffix + it->nbytes) != 0)
-                          {
-                              item_remove(it);
-                              goto stop;
-                          }
-                      } else if (add_iov(c, ITEM_suffix(it), it->nsuffix) != 0 ||
-                                 add_chunked_item_iovs(c, it, it->nbytes) != 0) {
-                          item_remove(it);
-                          goto stop;
-                      }
-                }
-
 
                 if (settings.verbose > 1) {
                     int ii;
@@ -4104,10 +4075,8 @@ stop:
 
     c->icurr = c->ilist;
     c->ileft = i;
-    if (return_cas || !settings.inline_ascii_response) {
-        c->suffixcurr = c->suffixlist;
-        c->suffixleft = si;
-    }
+    c->suffixcurr = c->suffixlist;
+    c->suffixleft = si;
 
     if (settings.verbose > 1)
         fprintf(stderr, ">%d END\n", c->sfd);
@@ -4414,7 +4383,7 @@ enum delta_result_type do_add_delta(conn *c, const char *key, const size_t nkey,
     } else if (it->refcount > 1) {
         item *new_it;
         uint32_t flags;
-        FLAGS_CONV(settings.inline_ascii_response, it, flags);
+        FLAGS_CONV(it, flags);
         new_it = do_item_alloc(ITEM_key(it), it->nkey, flags, it->exptime, res + 2);
         if (new_it == 0) {
             do_item_remove(it);
@@ -6606,9 +6575,6 @@ static void usage(void) {
            "   - worker_logbuf_size:  size in kilobytes of per-worker-thread buffer\n"
            "                          read by background thread, then written to watchers.\n"
            "   - track_sizes:         enable dynamic reports for 'stats sizes' command.\n"
-           "   - no_inline_ascii_resp: save up to 24 bytes per item.\n"
-           "                           small perf hit in ASCII, no perf difference in\n"
-           "                           binary protocol. speeds up all sets.\n"
            "   - no_hashexpand:       disables hash table expansion (dangerous)\n"
            "   - modern:              enables options which will be default in future.\n"
            "             currently: nothing\n"
@@ -7615,10 +7581,8 @@ int main (int argc, char **argv) {
                 item_stats_sizes_init();
                 break;
             case NO_INLINE_ASCII_RESP:
-                settings.inline_ascii_response = false;
                 break;
             case INLINE_ASCII_RESP:
-                settings.inline_ascii_response = true;
                 break;
             case NO_CHUNKED_ITEMS:
                 settings.slab_chunk_size_max = settings.slab_page_size;
@@ -7882,7 +7846,6 @@ int main (int argc, char **argv) {
                 settings.slab_reassign = false;
                 settings.slab_automove = 0;
                 settings.maxconns_fast = false;
-                settings.inline_ascii_response = true;
                 settings.lru_segmented = false;
                 hash_type = JENKINS_HASH;
                 start_lru_crawler = false;
@@ -7957,15 +7920,6 @@ int main (int argc, char **argv) {
             exit(EX_USAGE);
         }
 
-        /* This is due to the suffix header being generated with the wrong length
-         * value for the ITEM_HDR replacement. The cuddled nbytes no longer
-         * matches, so we end up losing a few bytes on readback.
-         */
-        if (settings.inline_ascii_response) {
-            fprintf(stderr, "Cannot use inline_ascii_response with extstore enabled\n");
-            exit(EX_USAGE);
-        }
-
         if (settings.udpport) {
             fprintf(stderr, "Cannot use UDP with extstore enabled (-U 0 to disable)\n");
             exit(EX_USAGE);
diff --git a/memcached.h b/memcached.h
index 176a9fb..df1e7d5 100644
--- a/memcached.h
+++ b/memcached.h
@@ -145,10 +145,8 @@
     APPEND_NUM_FMT_STAT("%d:%s", num, name, fmt, val)
 
 /** Item client flag conversion */
-#define FLAGS_CONV(iar, it, flag) { \
-    if ((iar)) { \
-        flag = (uint32_t) strtoul(ITEM_suffix((it)), (char **) NULL, 10); \
-    } else if ((it)->nsuffix > 0) { \
+#define FLAGS_CONV(it, flag) { \
+    if ((it)->nsuffix > 0) { \
         flag = *((uint32_t *)ITEM_suffix((it))); \
     } else { \
         flag = 0; \
@@ -416,7 +414,6 @@ struct settings {
     double hot_max_factor; /* HOT tail age relative to COLD tail */
     double warm_max_factor; /* WARM tail age relative to COLD tail */
     int crawls_persleep; /* Number of LRU crawls to run before sleeping */
-    bool inline_ascii_response; /* pre-format the VALUE line for ASCII responses */
     bool temp_lru; /* TTL < temporary_ttl uses TEMP_LRU */
     uint32_t temporary_ttl; /* temporary LRU threshold */
     int idle_timeout;       /* Number of seconds to let connections idle */
diff --git a/storage.c b/storage.c
index 45554cf..8b7764a 100644
--- a/storage.c
+++ b/storage.c
@@ -33,7 +33,7 @@ static int storage_write(void *storage, const int clsid, const int item_age) {
     uint32_t flags;
     if ((it->it_flags & ITEM_HDR) == 0 &&
             (item_age == 0 || current_time - it->time > item_age)) {
-        FLAGS_CONV(settings.inline_ascii_response, it, flags);
+        FLAGS_CONV(it, flags);
         item *hdr_it = do_item_alloc(ITEM_key(it), it->nkey, flags, it->exptime, sizeof(item_hdr));
         /* Run the storage write understanding the start of the item is dirty.
          * We will fill it (time/exptime/etc) from the header item on read.
diff --git a/t/inline_asciihdr.t b/t/inline_asciihdr.t
deleted file mode 100644
index 774922d..0000000
--- a/t/inline_asciihdr.t
+++ /dev/null
@@ -1,31 +0,0 @@
-#!/usr/bin/perl
-# Ensure get and gets can mirror flags + CAS properly when not inlining the
-# ascii response header.
-
-use strict;
-use Test::More tests => 17;
-use FindBin qw($Bin);
-use lib "$Bin/lib";
-use MemcachedTest;
-
-my $server = new_memcached('-o no_inline_ascii_resp');
-my $sock = $server->sock;
-
-# 0 flags and size
-print $sock "set foo 0 0 0\r\n\r\n";
-is(scalar <$sock>, "STORED\r\n", "stored");
-
-mem_get_is($sock, "foo", "");
-
-for my $flags (0, 123, 2**16-1, 2**31, 2**32-1) {
-    print $sock "set foo $flags 0 6\r\nfooval\r\n";
-    is(scalar <$sock>, "STORED\r\n", "stored foo");
-    mem_get_is({ sock => $sock,
-                 flags => $flags }, "foo", "fooval", "got flags $flags back");
-    my @res = mem_gets($sock, "foo");
-    mem_gets_is({ sock => $sock,
-                  flags => $flags }, $res[0], "foo", "fooval", "got flags $flags back");
-
-}
-
-
diff --git a/t/issue_42.t b/t/issue_42.t
index 6ab1543..d0cb32d 100644
--- a/t/issue_42.t
+++ b/t/issue_42.t
@@ -10,12 +10,16 @@ my $server = new_memcached("-o no_modern");
 my $sock = $server->sock;
 my $value = "B"x10;
 my $key = 0;
+my $key_count = 10;
 
-for ($key = 0; $key < 10; $key++) {
+for ($key = 0; $key < $key_count; $key++) {
     print $sock "set key$key 0 0 10\r\n$value\r\n";
     is (scalar <$sock>, "STORED\r\n", "stored key$key");
 }
 
-my $first_stats = mem_stats($sock, "slabs");
-my $req = $first_stats->{"1:mem_requested"};
-ok ($req == "640" || $req == "800" || $req == "770", "Check allocated size");
+my $stats = mem_stats($sock, "slabs");
+my $req = $stats->{"1:mem_requested"};
+my $top = $stats->{"1:chunk_size"} * $key_count;
+# unreasonable for the result to be < 500 bytes (min item header is 48), but
+# should be less than the maximum potential number.
+ok ($req > 500 && $req < $top, "Check allocated size");