I agree on the main points below. Performance patches without actual benchmarks are not going to get applied.
It's not that hard. There's a benchmark script included in the perl client libraries. Plus there's memslap at http://tangent.org/552/libmemcached.html I'll spend some time reviewing the bug fix patches posted, but I would appreciate it if someone took the time to do benchmarks. On Fri, Nov 09, 2007 at 09:37:47AM -0800, Marc wrote: > Sorry I didn¹t reply to this sooner. It wound up in my junk folder. > > What is the ultimate goal of all these changes? The switch wasn¹t that > huge, nor do I think a bunch of callbacks is particularly cleaner or more > readable - especially when you start throwing macros into the mix. I don¹t > mind taking cleanup changes that are included with feature and, more > importantly, performance and scalability enhancements. But these seem > gratuitous. Are you actually testing them in a production environment? > Given the size of our operation, pushing out even a one-line critical fixes > is a major undertaking. I¹m sure the same is true for many other people on > this list. To push new code out means we need to go through detailed code > review, regression test and then be on call 24x7 for several days to react > to any problems that come up as software is pushed into production. Why > would I do that for this update or any of the patches you have submitted to > this list? > > On 11/7/07 11:51 AM, "Tomash Brechko" <[EMAIL PROTECTED]> wrote: > > > Replace huge switch() in process_command() with callback functions > > for each command. From now on, to add new command its processing > > function should be defined, this function should be declared in > > command.h, and command should be registered in command.gperf. > > > > diff --git a/trunk/server/command.gperf b/trunk/server/command.gperf > > index 028e4b1..037dc5b 100644 > > +++ b/trunk/server/command.gperf > > @@ -1,5 +1,6 @@ > > %language=ANSI-C > > %define lookup-function-name get_command > > +%define hash-function-name command_hash > > %compare-lengths > > %compare-strncmp > > %readonly-tables > > @@ -14,26 +15,26 @@ > > struct command; > > %% > > # > > -# keyword command code ntokens (negative means no less then > > -ntokens) > > +# keyword command code callback > > # > > -add, CMD_ADD, 6 > > -append, CMD_APPEND, 6 > > -bg, CMD_BG, 3 > > -bget, CMD_BGET, -3 > > -cas, CMD_CAS, 6 > > -decr, CMD_DECR, 4 > > -delete, CMD_DELETE, -3 > > -disown, CMD_DISOWN, 3 > > -flush_all, CMD_FLUSH_ALL, -2 > > -get, CMD_GET, -3 > > -gets, CMD_GETS, -3 > > -incr, CMD_INCR, 4 > > -own, CMD_OWN, 3 > > -prepend, CMD_PREPEND, 6 > > -quit, CMD_QUIT, 2 > > -replace, CMD_REPLACE, 6 > > -set, CMD_SET, 6 > > -slabs, CMD_SLABS, 5 > > -stats, CMD_STATS, -2 > > -verbosity, CMD_VERBOSITY, 3 > > -version, CMD_VERSION, 2 > > +add, CMD_ADD, process_update_command > > +append, CMD_APPEND, process_update_command > > +bg, CMD_BG, process_manage_command > > +bget, CMD_BGET, process_get_command > > +cas, CMD_CAS, process_update_command > > +decr, CMD_DECR, process_arithmetic_command > > +delete, CMD_DELETE, process_delete_command > > +disown, CMD_DISOWN, process_manage_command > > +flush_all, CMD_FLUSH_ALL, process_flush_all_command > > +get, CMD_GET, process_get_command > > +gets, CMD_GETS, process_get_command > > +incr, CMD_INCR, process_arithmetic_command > > +own, CMD_OWN, process_manage_command > > +prepend, CMD_PREPEND, process_update_command > > +quit, CMD_QUIT, process_quit_command > > +replace, CMD_REPLACE, process_update_command > > +set, CMD_SET, process_update_command > > +slabs, CMD_SLABS, process_slabs_command > > +stats, CMD_STATS, process_stat > > +verbosity, CMD_VERBOSITY, process_verbosity_command > > +version, CMD_VERSION, process_version_command > > diff --git a/trunk/server/command.h b/trunk/server/command.h > > index 4d46a3a..52f8611 100644 > > +++ b/trunk/server/command.h > > @@ -1,6 +1,8 @@ > > #ifndef COMMAND_H > > #define COMMAND_H 1 > > > > +#include "memcached.h" > > + > > > > enum command_code > > { > > @@ -31,7 +33,8 @@ struct command > > { > > const char *name; > > enum command_code code; > > - int ntokens; > > + void (*callback)(enum command_code code, > > + conn *c, token_t *tokens, size_t ntokens); > > }; > > > > > > @@ -68,4 +71,21 @@ const struct param * > > get_param(const char *str, unsigned int len); > > > > > > +#define DECLARE_CALLBACK(callback) \ > > + extern void callback(enum command_code code, conn *c, \ > > + token_t *tokens, size_t ntokens) > > + > > +DECLARE_CALLBACK(process_get_command); > > +DECLARE_CALLBACK(process_update_command); > > +DECLARE_CALLBACK(process_arithmetic_command); > > +DECLARE_CALLBACK(process_verbosity_command); > > +DECLARE_CALLBACK(process_stat); > > +DECLARE_CALLBACK(process_delete_command); > > +DECLARE_CALLBACK(process_manage_command); > > +DECLARE_CALLBACK(process_version_command); > > +DECLARE_CALLBACK(process_quit_command); > > +DECLARE_CALLBACK(process_flush_all_command); > > +DECLARE_CALLBACK(process_slabs_command); > > + > > + > > #endif /* ! COMMAND_H */ > > diff --git a/trunk/server/memcached.c b/trunk/server/memcached.c > > index 4d464f1..1aa8af8 100644 > > +++ b/trunk/server/memcached.c > > @@ -793,11 +793,6 @@ int do_store_item(item *it, int comm) { > > return stored; > > } > > > > -typedef struct token_s { > > - char *value; > > - size_t length; > > -} token_t; > > - > > #define COMMAND_TOKEN 0 > > #define SUBCOMMAND_TOKEN 1 > > #define KEY_TOKEN 1 > > @@ -906,13 +901,15 @@ inline static void process_stats_detail(conn *c, > > token_t > > *tokens) { > > } > > } > > > > -static void process_stat(conn *c, token_t *tokens, const size_t ntokens) { > > +void process_stat(enum command_code code, > > + conn *c, token_t *tokens, size_t ntokens) { > > rel_time_t now = current_time; > > struct param *param; > > > > assert(c != NULL); > > > > - if(ntokens < 2) { > > + /* FIXME: why not "ERROR" as other commands do? */ > > + if (ntokens < 2) { > > out_string(c, "CLIENT_ERROR bad command line"); > > return; > > } > > @@ -1075,6 +1072,7 @@ static void process_stat(conn *c, token_t *tokens, > > const > > size_t ntokens) { > > return; > > > > case PARAM_DETAIL: > > + /* FIXME: why not "ERROR" as other commands do? */ > > if (ntokens < 4) > > out_string(c, "CLIENT_ERROR usage: stats detail on|off|dump"); > > else > > @@ -1097,7 +1095,8 @@ static void process_stat(conn *c, token_t *tokens, > > const > > size_t ntokens) { > > } > > > > /* ntokens is overwritten here... shrug.. */ > > -static inline void process_get_command(conn *c, token_t *tokens, size_t > > ntokens, bool return_key_ptr) { > > +void process_get_command(enum command_code code, > > + conn *c, token_t *tokens, size_t ntokens) { > > char *key; > > size_t nkey; > > int i = 0; > > @@ -1107,6 +1106,11 @@ static inline void process_get_command(conn *c, > > token_t > > *tokens, size_t ntokens, > > uint32_t in_memory_ptr; > > assert(c != NULL); > > > > + if (ntokens < 3) { > > + out_string(c, "ERROR"); > > + return; > > + } > > + > > if (settings.managed) { > > int bucket = c->bucket; > > if (bucket == -1) { > > @@ -1155,7 +1159,7 @@ 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_key_ptr == true) > > + if (code == CMD_GETS) > > { > > in_memory_ptr = (uint32_t)item_get(key, nkey); > > sprintf(suffix," %d %d %lu\r\n", atoi(ITEM_suffix(it) + > > 1), > > it->nbytes - 2, in_memory_ptr); > > @@ -1226,7 +1230,8 @@ static inline void process_get_command(conn *c, > > token_t > > *tokens, size_t ntokens, > > return; > > } > > > > -static void process_update_command(conn *c, token_t *tokens, const size_t > > ntokens, int comm, bool handle_cas) { > > +void process_update_command(enum command_code code, > > + conn *c, token_t *tokens, const size_t > > ntokens) { > > char *key; > > size_t nkey; > > int flags; > > @@ -1237,6 +1242,11 @@ static void process_update_command(conn *c, token_t > > *tokens, const size_t ntoken > > > > assert(c != NULL); > > > > + if (ntokens != 6) { > > + out_string(c, "ERROR"); > > + return; > > + } > > + > > if (tokens[KEY_TOKEN].length > KEY_MAX_LENGTH) { > > out_string(c, "CLIENT_ERROR bad command line format"); > > return; > > @@ -1280,7 +1290,7 @@ static void process_update_command(conn *c, token_t > > *tokens, const size_t ntoken > > it = item_alloc(key, nkey, flags, realtime(exptime), vlen+2); > > > > /* HANDLE_CAS VALIDATION */ > > - if (handle_cas == true) > > + if (code == CMD_CAS) > > { > > item *itmp=item_get(key, it->nkey); > > /* Release the reference */ > > @@ -1325,11 +1335,29 @@ static void process_update_command(conn *c, token_t > > *tokens, const size_t ntoken > > c->item = it; > > c->ritem = ITEM_data(it); > > c->rlbytes = it->nbytes; > > - c->item_comm = comm; > > + switch (code) { > > + case CMD_ADD: > > + c->item_comm = NREAD_ADD; > > + break; > > + case CMD_SET: > > + c->item_comm = NREAD_SET; > > + break; > > + case CMD_REPLACE: > > + case CMD_CAS: > > + c->item_comm = NREAD_REPLACE; > > + break; > > + case CMD_PREPEND: > > + c->item_comm = NREAD_PREPEND; > > + break; > > + case CMD_APPEND: > > + c->item_comm = NREAD_APPEND; > > + break; > > + } > > conn_set_state(c, conn_nread); > > } > > > > -static void process_arithmetic_command(conn *c, token_t *tokens, const > > size_t > > ntokens, const bool incr) { > > +void process_arithmetic_command(enum command_code code, > > + conn *c, token_t *tokens, size_t ntokens) { > > char temp[sizeof("18446744073709551615")]; > > item *it; > > int64_t delta; > > @@ -1338,6 +1366,11 @@ static void process_arithmetic_command(conn *c, > > token_t > > *tokens, const size_t nt > > > > assert(c != NULL); > > > > + if (ntokens != 4) { > > + out_string(c, "ERROR"); > > + return; > > + } > > + > > if(tokens[KEY_TOKEN].length > KEY_MAX_LENGTH) { > > out_string(c, "CLIENT_ERROR bad command line format"); > > return; > > @@ -1372,7 +1405,7 @@ static void process_arithmetic_command(conn *c, > > token_t > > *tokens, const size_t nt > > return; > > } > > > > - out_string(c, add_delta(it, incr, delta, temp)); > > + out_string(c, add_delta(it, (code == CMD_INCR), delta, temp)); > > item_remove(it); /* release our reference */ > > } > > > > @@ -1426,7 +1459,8 @@ char *do_add_delta(item *it, const bool incr, const > > int64_t delta, char *buf) { > > return buf; > > } > > > > -static void process_delete_command(conn *c, token_t *tokens, const size_t > > ntokens) { > > +void process_delete_command(enum command_code code, > > + conn *c, token_t *tokens, size_t ntokens) { > > char *key; > > size_t nkey; > > item *it; > > @@ -1434,6 +1468,11 @@ static void process_delete_command(conn *c, token_t > > *tokens, const size_t ntoken > > > > assert(c != NULL); > > > > + if (ntokens < 3 || ntokens > 4) { > > + out_string(c, "ERROR"); > > + return; > > + } > > + > > if (settings.managed) { > > int bucket = c->bucket; > > if (bucket == -1) { > > @@ -1513,15 +1552,159 @@ char *do_defer_delete(item *it, time_t exptime) > > return "DELETED"; > > } > > > > -static void process_verbosity_command(conn *c, token_t *tokens, const > > size_t > > ntokens) { > > +void process_verbosity_command(enum command_code code, > > + conn *c, token_t *tokens, size_t ntokens) { > > unsigned int level; > > > > assert(c != NULL); > > > > + if (ntokens != 3) { > > + out_string(c, "ERROR"); > > + return; > > + } > > + > > level = strtoul(tokens[1].value, NULL, 10); > > settings.verbose = level > MAX_VERBOSITY_LEVEL ? MAX_VERBOSITY_LEVEL : > > level; > > out_string(c, "OK"); > > - return; > > +} > > + > > +void process_manage_command(enum command_code code, > > + conn *c, token_t *tokens, size_t ntokens) { > > + unsigned int bucket, gen; > > + bool res; > > + > > + if (ntokens != 3) { > > + out_string(c, "ERROR"); > > + return; > > + } > > + > > + if (!settings.managed) { > > + out_string(c, "CLIENT_ERROR not a managed instance"); > > + return; > > + } > > + > > + switch (code) { > > + case CMD_OWN: > > + case CMD_BG: > > + res = (sscanf(tokens[1].value, "%u:%u", &bucket, &gen) == 2); > > + break; > > + case CMD_DISOWN: > > + res = (sscanf(tokens[1].value, "%u", &bucket) == 1); > > + break; > > + } > > + > > + if (res) { > > + if ((bucket < 0) || (bucket >= MAX_BUCKETS)) { > > + if (code != CMD_BG) { > > + out_string(c, "CLIENT_ERROR bucket number out of range"); > > + return; > > + } > > + } > > + switch (code) { > > + case CMD_OWN: > > + buckets[bucket] = gen; > > + out_string(c, "OWNED"); > > + break; > > + case CMD_DISOWN: > > + buckets[bucket] = 0; > > + out_string(c, "DISOWNED"); > > + break; > > + case CMD_BG: > > + if (gen > 0) { > > + c->bucket = bucket; > > + c->gen = gen; > > + } > > + conn_set_state(c, conn_read); > > + break; > > + } > > + } else { > > + out_string(c, "CLIENT_ERROR bad format"); > > + } > > +} > > + > > +void process_flush_all_command(enum command_code code, > > + conn *c, token_t *tokens, size_t ntokens) { > > + if (ntokens < 2 || ntokens > 3) { > > + out_string(c, "ERROR"); > > + } > > + > > + time_t exptime = 0; > > + set_current_time(); > > + > > + if (ntokens == 2) { > > + settings.oldest_live = current_time - 1; > > + item_flush_expired(); > > + out_string(c, "OK"); > > + return; > > + } > > + > > + exptime = strtol(tokens[1].value, NULL, 10); > > + if (errno == ERANGE) { > > + out_string(c, "CLIENT_ERROR bad command line format"); > > + return; > > + } > > + > > + settings.oldest_live = realtime(exptime) - 1; > > + item_flush_expired(); > > + out_string(c, "OK"); > > +} > > + > > +void process_slabs_command(enum command_code code, > > + conn *c, token_t *tokens, size_t ntokens) { > > + if (ntokens != 5 > > + || strcmp(tokens[SUBCOMMAND_TOKEN].value, "reassign") != 0) { > > + out_string(c, "ERROR"); > > + return; > > + } > > + > > +#ifdef ALLOW_SLABS_REASSIGN > > + > > + int src, dst, rv; > > + > > + src = strtol(tokens[2].value, NULL, 10); > > + dst = strtol(tokens[3].value, NULL, 10); > > + > > + if (errno == ERANGE) { > > + out_string(c, "CLIENT_ERROR bad command line format"); > > + return; > > + } > > + > > + rv = slabs_reassign(src, dst); > > + if (rv == 1) { > > + out_string(c, "DONE"); > > + return; > > + } > > + if (rv == 0) { > > + out_string(c, "CANT"); > > + return; > > + } > > + if (rv == -1) { > > + out_string(c, "BUSY"); > > + return; > > + } > > +#else > > + out_string(c, "CLIENT_ERROR Slab reassignment not supported"); > > +#endif > > +} > > + > > +void process_version_command(enum command_code code, > > + conn *c, token_t *tokens, size_t ntokens) { > > + if (ntokens != 2) { > > + out_string(c, "ERROR"); > > + return; > > + } > > + > > + out_string(c, "VERSION " VERSION); > > +} > > + > > +void process_quit_command(enum command_code code, > > + conn *c, token_t *tokens, size_t ntokens) { > > + if (ntokens != 2) { > > + out_string(c, "ERROR"); > > + return; > > + } > > + > > + conn_set_state(c, conn_closing); > > } > > > > static void process_command(conn *c, char *command) { > > @@ -1552,190 +1735,10 @@ static void process_command(conn *c, char > > *command) { > > > > cmd = get_command(tokens[COMMAND_TOKEN].value, > > tokens[COMMAND_TOKEN].length); > > - > > - if (!cmd || (cmd->ntokens > 0 && cmd->ntokens != ntokens) > > - || (cmd->ntokens < 0 && -cmd->ntokens > ntokens)) { > > + if (cmd) { > > + (*cmd->callback)(cmd->code, c, tokens, ntokens); > > + } else { > > out_string(c, "ERROR"); > > - return; > > - } > > - > > - switch (cmd->code) { > > - case CMD_GET: > > - case CMD_BGET: > > - process_get_command(c, tokens, ntokens, false); > > - break; > > - case CMD_ADD: > > - process_update_command(c, tokens, ntokens, NREAD_ADD, false); > > - break; > > - case CMD_SET: > > - process_update_command(c, tokens, ntokens, NREAD_SET, false); > > - break; > > - case CMD_REPLACE: > > - process_update_command(c, tokens, ntokens, NREAD_REPLACE, false); > > - break; > > - case CMD_PREPEND: > > - process_update_command(c, tokens, ntokens, NREAD_PREPEND, false); > > - break; > > - case CMD_APPEND: > > - process_update_command(c, tokens, ntokens, NREAD_APPEND, false); > > - break; > > - case CMD_CAS: > > - process_update_command(c, tokens, ntokens, NREAD_REPLACE, true); > > - break; > > - case CMD_INCR: > > - process_arithmetic_command(c, tokens, ntokens, 1); > > - break; > > - case CMD_GETS: > > - process_get_command(c, tokens, ntokens, true); > > - break; > > - case CMD_DECR: > > - process_arithmetic_command(c, tokens, ntokens, 0); > > - break; > > - case CMD_DELETE: > > - if (ntokens <= 4) > > - process_delete_command(c, tokens, ntokens); > > - else > > - out_string(c, "ERROR"); > > - break; > > - case CMD_OWN: > > - { > > - unsigned int bucket, gen; > > - if (!settings.managed) { > > - out_string(c, "CLIENT_ERROR not a managed instance"); > > - return; > > - } > > - > > - if (sscanf(tokens[1].value, "%u:%u", &bucket,&gen) == 2) { > > - if ((bucket < 0) || (bucket >= MAX_BUCKETS)) { > > - out_string(c, "CLIENT_ERROR bucket number out of > > range"); > > - return; > > - } > > - buckets[bucket] = gen; > > - out_string(c, "OWNED"); > > - return; > > - } else { > > - out_string(c, "CLIENT_ERROR bad format"); > > - return; > > - } > > - } > > - break; > > - case CMD_DISOWN: > > - { > > - int bucket; > > - if (!settings.managed) { > > - out_string(c, "CLIENT_ERROR not a managed instance"); > > - return; > > - } > > - if (sscanf(tokens[1].value, "%u", &bucket) == 1) { > > - if ((bucket < 0) || (bucket >= MAX_BUCKETS)) { > > - out_string(c, "CLIENT_ERROR bucket number out of > > range"); > > - return; > > - } > > - buckets[bucket] = 0; > > - out_string(c, "DISOWNED"); > > - return; > > - } else { > > - out_string(c, "CLIENT_ERROR bad format"); > > - return; > > - } > > - } > > - break; > > - case CMD_BG: > > - { > > - int bucket, gen; > > - if (!settings.managed) { > > - out_string(c, "CLIENT_ERROR not a managed instance"); > > - return; > > - } > > - if (sscanf(tokens[1].value, "%u:%u", &bucket, &gen) == 2) { > > - /* we never write anything back, even if input's wrong */ > > - if ((bucket < 0) || (bucket >= MAX_BUCKETS) || (gen <= 0)) > > { > > - /* do nothing, bad input */ > > - } else { > > - c->bucket = bucket; > > - c->gen = gen; > > - } > > - conn_set_state(c, conn_read); > > - return; > > - } else { > > - out_string(c, "CLIENT_ERROR bad format"); > > - return; > > - } > > - } > > - break; > > - case CMD_STATS: > > - process_stat(c, tokens, ntokens); > > - break; > > - case CMD_FLUSH_ALL: > > - if (ntokens <= 3) { > > - time_t exptime = 0; > > - set_current_time(); > > - > > - if(ntokens == 2) { > > - settings.oldest_live = current_time - 1; > > - item_flush_expired(); > > - out_string(c, "OK"); > > - return; > > - } > > - > > - exptime = strtol(tokens[1].value, NULL, 10); > > - if(errno == ERANGE) { > > - out_string(c, "CLIENT_ERROR bad command line format"); > > - return; > > - } > > - > > - settings.oldest_live = realtime(exptime) - 1; > > - item_flush_expired(); > > - out_string(c, "OK"); > > - return; > > - } else { > > - out_string(c, "ERROR"); > > - } > > - break; > > - case CMD_VERSION: > > - out_string(c, "VERSION " VERSION); > > - break; > > - case CMD_QUIT: > > - conn_set_state(c, conn_closing); > > - break; > > - case CMD_SLABS: > > - if (strcmp(tokens[SUBCOMMAND_TOKEN].value, "reassign") == 0) { > > -#ifdef ALLOW_SLABS_REASSIGN > > - > > - int src, dst, rv; > > - > > - src = strtol(tokens[2].value, NULL, 10); > > - dst = strtol(tokens[3].value, NULL, 10); > > - > > - if(errno == ERANGE) { > > - out_string(c, "CLIENT_ERROR bad command line format"); > > - return; > > - } > > - > > - rv = slabs_reassign(src, dst); > > - if (rv == 1) { > > - out_string(c, "DONE"); > > - return; > > - } > > - if (rv == 0) { > > - out_string(c, "CANT"); > > - return; > > - } > > - if (rv == -1) { > > - out_string(c, "BUSY"); > > - return; > > - } > > -#else > > - out_string(c, "CLIENT_ERROR Slab reassignment not supported"); > > -#endif > > - > > - } else { > > - out_string(c, "ERROR"); > > - } > > - break; > > - case CMD_VERBOSITY: > > - process_verbosity_command(c, tokens, ntokens); > > - break; > > } > > } > > > > diff --git a/trunk/server/memcached.h b/trunk/server/memcached.h > > index 1ca73e8..7d0188f 100644 > > +++ b/trunk/server/memcached.h > > @@ -1,6 +1,10 @@ > > /* -*- Mode: C; tab-width: 4; c-basic-offset: 4; indent-tabs-mode: nil -*- > > */ > > /* $Id$ */ > > > > +#ifndef MEMCACHED_H > > +#define MEMCACHED_H 1 > > + > > + > > #ifdef HAVE_CONFIG_H > > #include "config.h" > > #endif > > @@ -207,6 +211,11 @@ typedef struct { > > int gen; /* generation requested for the bucket */ > > } conn; > > > > +typedef struct token_s { > > + char *value; > > + size_t length; > > +} token_t; > > + > > /* number of virtual buckets for a managed instance */ > > #define MAX_BUCKETS 32768 > > > > @@ -345,3 +354,4 @@ int mt_store_item(item *item, int comm); > > #endif /* !USE_THREADS */ > > > > > > +#endif /* ! MEMCACHED_H */ > > diff --git a/trunk/server/param.gperf b/trunk/server/param.gperf > > index cc1db38..21a1af8 100644 > > +++ b/trunk/server/param.gperf > > @@ -1,5 +1,6 @@ > > %language=ANSI-C > > %define lookup-function-name get_param > > +%define hash-function-name param_hash > > %compare-lengths > > %compare-strncmp > > %readonly-tables > > -- Paul Lindner ||||| | | | | | | | | | [EMAIL PROTECTED]
