Skip to content

Commit

Permalink
Replace dict with hashset in command tables (#1065)
Browse files Browse the repository at this point in the history 
This changes the type of command tables from dict to hashset. Command
table lookup takes ~3% of overall CPU time in benchmarks, so it is a
good candidate for optimization.

My initial SET benchmark comparison suggests that hashset is about 4.5
times faster than dict and this replacement reduced overall CPU time by
2.79% 🥳

---------

Signed-off-by: Rain Valentine <[email protected]>
Signed-off-by: Rain Valentine <[email protected]>
Co-authored-by: Rain Valentine <[email protected]>
  • Loading branch information
@SoftlyRaining @zuiderkwast
2 people authored and zuiderkwast committed Oct 18, 2024
1 parent 02221d1 commit 3038293
Show file tree
Hide file tree
Showing 6 changed files with 191 additions and 181 deletions.
62 changes: 31 additions & 31 deletions src/acl.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -652,14 +652,14 @@ void ACLChangeSelectorPerm(aclSelector *selector, struct serverCommand *cmd, int
unsigned long id = cmd->id;
ACLSetSelectorCommandBit(selector, id, allow);
ACLResetFirstArgsForCommand(selector, id);
if (cmd->subcommands_dict) {
dictEntry *de;
dictIterator *di = dictGetSafeIterator(cmd->subcommands_dict);
while ((de = dictNext(di)) != NULL) {
struct serverCommand *sub = (struct serverCommand *)dictGetVal(de);
if (cmd->subcommands_set) {
hashsetIterator iter;
hashsetInitSafeIterator(&iter, cmd->subcommands_set);
struct serverCommand *sub;
while (hashsetNext(&iter, (void **)&sub)) {
ACLSetSelectorCommandBit(selector, sub->id, allow);
}
dictReleaseIterator(di);
hashsetResetIterator(&iter);
}
}

Expand All @@ -669,19 +669,19 @@ void ACLChangeSelectorPerm(aclSelector *selector, struct serverCommand *cmd, int
* value. Since the category passed by the user may be non existing, the
* function returns C_ERR if the category was not found, or C_OK if it was
* found and the operation was performed. */
void ACLSetSelectorCommandBitsForCategory(dict *commands, aclSelector *selector, uint64_t cflag, int value) {
dictIterator *di = dictGetIterator(commands);
dictEntry *de;
while ((de = dictNext(di)) != NULL) {
struct serverCommand *cmd = dictGetVal(de);
void ACLSetSelectorCommandBitsForCategory(hashset *commands, aclSelector *selector, uint64_t cflag, int value) {
hashsetIterator iter;
hashsetInitIterator(&iter, commands);
struct serverCommand *cmd;
while (hashsetNext(&iter, (void **)&cmd)) {
if (cmd->acl_categories & cflag) {
ACLChangeSelectorPerm(selector, cmd, value);
}
if (cmd->subcommands_dict) {
ACLSetSelectorCommandBitsForCategory(cmd->subcommands_dict, selector, cflag, value);
if (cmd->subcommands_set) {
ACLSetSelectorCommandBitsForCategory(cmd->subcommands_set, selector, cflag, value);
}
}
dictReleaseIterator(di);
hashsetResetIterator(&iter);
}

/* This function is responsible for recomputing the command bits for all selectors of the existing users.
Expand Down Expand Up @@ -732,26 +732,26 @@ int ACLSetSelectorCategory(aclSelector *selector, const char *category, int allo
return C_OK;
}

void ACLCountCategoryBitsForCommands(dict *commands,
void ACLCountCategoryBitsForCommands(hashset *commands,
aclSelector *selector,
unsigned long *on,
unsigned long *off,
uint64_t cflag) {
dictIterator *di = dictGetIterator(commands);
dictEntry *de;
while ((de = dictNext(di)) != NULL) {
struct serverCommand *cmd = dictGetVal(de);
hashsetIterator iter;
hashsetInitIterator(&iter, commands);
struct serverCommand *cmd;
while (hashsetNext(&iter, (void **)&cmd)) {
if (cmd->acl_categories & cflag) {
if (ACLGetSelectorCommandBit(selector, cmd->id))
(*on)++;
else
(*off)++;
}
if (cmd->subcommands_dict) {
ACLCountCategoryBitsForCommands(cmd->subcommands_dict, selector, on, off, cflag);
if (cmd->subcommands_set) {
ACLCountCategoryBitsForCommands(cmd->subcommands_set, selector, on, off, cflag);
}
}
dictReleaseIterator(di);
hashsetResetIterator(&iter);
}

/* Return the number of commands allowed (on) and denied (off) for the user 'u'
Expand Down Expand Up @@ -1163,7 +1163,7 @@ int ACLSetSelector(aclSelector *selector, const char *op, size_t oplen) {
return C_ERR;
}

if (cmd->subcommands_dict) {
if (cmd->subcommands_set) {
/* If user is trying to allow a valid subcommand we can just add its unique ID */
cmd = ACLLookupCommand(op + 1);
if (cmd == NULL) {
Expand Down Expand Up @@ -2754,22 +2754,22 @@ sds getAclErrorMessage(int acl_res, user *user, struct serverCommand *cmd, sds e
* ==========================================================================*/

/* ACL CAT category */
void aclCatWithFlags(client *c, dict *commands, uint64_t cflag, int *arraylen) {
dictEntry *de;
dictIterator *di = dictGetIterator(commands);
void aclCatWithFlags(client *c, hashset *commands, uint64_t cflag, int *arraylen) {
hashsetIterator iter;
hashsetInitIterator(&iter, commands);

while ((de = dictNext(di)) != NULL) {
struct serverCommand *cmd = dictGetVal(de);
struct serverCommand *cmd;
while (hashsetNext(&iter, (void **)&cmd)) {
if (cmd->acl_categories & cflag) {
addReplyBulkCBuffer(c, cmd->fullname, sdslen(cmd->fullname));
(*arraylen)++;
}

if (cmd->subcommands_dict) {
aclCatWithFlags(c, cmd->subcommands_dict, cflag, arraylen);
if (cmd->subcommands_set) {
aclCatWithFlags(c, cmd->subcommands_set, cflag, arraylen);
}
}
dictReleaseIterator(di);
hashsetResetIterator(&iter);
}

/* Add the formatted response from a single selector to the ACL GETUSER
Expand Down
12 changes: 4 additions & 8 deletions src/config.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -532,7 +532,6 @@ void loadServerConfigFromString(char *config) {
loadServerConfig(argv[1], 0, NULL);
} else if (!strcasecmp(argv[0], "rename-command") && argc == 3) {
struct serverCommand *cmd = lookupCommandBySds(argv[1]);
int retval;

if (!cmd) {
err = "No such command in rename-command";
Expand All @@ -541,16 +540,13 @@ void loadServerConfigFromString(char *config) {

/* If the target command name is the empty string we just
* remove it from the command table. */
retval = dictDelete(server.commands, argv[1]);
serverAssert(retval == DICT_OK);
serverAssert(hashsetDelete(server.commands, argv[1]));

/* Otherwise we re-add the command under a different name. */
if (sdslen(argv[2]) != 0) {
sds copy = sdsdup(argv[2]);

retval = dictAdd(server.commands, copy, cmd);
if (retval != DICT_OK) {
sdsfree(copy);
sdsfree(cmd->fullname);
cmd->fullname = sdsdup(argv[2]);
if (!hashsetAdd(server.commands, cmd)) {
err = "Target command name already exists";
goto loaderr;
}
Expand Down
25 changes: 12 additions & 13 deletions src/latency.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -527,24 +527,23 @@ void fillCommandCDF(client *c, struct hdr_histogram *histogram) {

/* latencyCommand() helper to produce for all commands,
* a per command cumulative distribution of latencies. */
void latencyAllCommandsFillCDF(client *c, dict *commands, int *command_with_data) {
dictIterator *di = dictGetSafeIterator(commands);
dictEntry *de;
void latencyAllCommandsFillCDF(client *c, hashset *commands, int *command_with_data) {
hashsetIterator iter;
hashsetInitSafeIterator(&iter, commands);
struct serverCommand *cmd;

while ((de = dictNext(di)) != NULL) {
cmd = (struct serverCommand *)dictGetVal(de);
while (hashsetNext(&iter, (void **)&cmd)) {
if (cmd->latency_histogram) {
addReplyBulkCBuffer(c, cmd->fullname, sdslen(cmd->fullname));
fillCommandCDF(c, cmd->latency_histogram);
(*command_with_data)++;
}

if (cmd->subcommands) {
latencyAllCommandsFillCDF(c, cmd->subcommands_dict, command_with_data);
latencyAllCommandsFillCDF(c, cmd->subcommands_set, command_with_data);
}
}
dictReleaseIterator(di);
hashsetResetIterator(&iter);
}

/* latencyCommand() helper to produce for a specific command set,
Expand All @@ -565,19 +564,19 @@ void latencySpecificCommandsFillCDF(client *c) {
command_with_data++;
}

if (cmd->subcommands_dict) {
dictEntry *de;
dictIterator *di = dictGetSafeIterator(cmd->subcommands_dict);
if (cmd->subcommands_set) {
hashsetIterator iter;
hashsetInitSafeIterator(&iter, cmd->subcommands_set);

while ((de = dictNext(di)) != NULL) {
struct serverCommand *sub = dictGetVal(de);
struct serverCommand *sub;
while (hashsetNext(&iter, (void **)&sub)) {
if (sub->latency_histogram) {
addReplyBulkCBuffer(c, sub->fullname, sdslen(sub->fullname));
fillCommandCDF(c, sub->latency_histogram);
command_with_data++;
}
}
dictReleaseIterator(di);
hashsetResetIterator(&iter);
}
}
setDeferredMapLen(c, replylen, command_with_data);
Expand Down
38 changes: 19 additions & 19 deletions src/module.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1297,8 +1297,8 @@ int VM_CreateCommand(ValkeyModuleCtx *ctx,
cp->serverCmd->arity = cmdfunc ? -1 : -2; /* Default value, can be changed later via dedicated API */
/* Drain IO queue before modifying commands dictionary to prevent concurrent access while modifying it. */
drainIOThreadsQueue();
serverAssert(dictAdd(server.commands, sdsdup(declared_name), cp->serverCmd) == DICT_OK);
serverAssert(dictAdd(server.orig_commands, sdsdup(declared_name), cp->serverCmd) == DICT_OK);
serverAssert(hashsetAdd(server.commands, cp->serverCmd));
serverAssert(hashsetAdd(server.orig_commands, cp->serverCmd));
cp->serverCmd->id = ACLGetCommandID(declared_name); /* ID used for ACL. */
return VALKEYMODULE_OK;
}
Expand Down Expand Up @@ -1430,7 +1430,7 @@ int VM_CreateSubcommand(ValkeyModuleCommand *parent,

/* Check if the command name is busy within the parent command. */
sds declared_name = sdsnew(name);
if (parent_cmd->subcommands_dict && lookupSubcommand(parent_cmd, declared_name) != NULL) {
if (parent_cmd->subcommands_set && lookupSubcommand(parent_cmd, declared_name) != NULL) {
sdsfree(declared_name);
return VALKEYMODULE_ERR;
}
Expand All @@ -1440,7 +1440,7 @@ int VM_CreateSubcommand(ValkeyModuleCommand *parent,
moduleCreateCommandProxy(parent->module, declared_name, fullname, cmdfunc, flags, firstkey, lastkey, keystep);
cp->serverCmd->arity = -2;

commandAddSubcommand(parent_cmd, cp->serverCmd, name);
commandAddSubcommand(parent_cmd, cp->serverCmd);
return VALKEYMODULE_OK;
}

Expand Down Expand Up @@ -12059,20 +12059,20 @@ int moduleFreeCommand(struct ValkeyModule *module, struct serverCommand *cmd) {
moduleFreeArgs(cmd->args, cmd->num_args);
zfree(cp);

if (cmd->subcommands_dict) {
dictEntry *de;
dictIterator *di = dictGetSafeIterator(cmd->subcommands_dict);
while ((de = dictNext(di)) != NULL) {
struct serverCommand *sub = dictGetVal(de);
if (cmd->subcommands_set) {
hashsetIterator iter;
hashsetInitSafeIterator(&iter, cmd->subcommands_set);
struct serverCommand *sub;
while (hashsetNext(&iter, (void **)&sub)) {
if (moduleFreeCommand(module, sub) != C_OK) continue;

serverAssert(dictDelete(cmd->subcommands_dict, sub->declared_name) == DICT_OK);
serverAssert(hashsetDelete(cmd->subcommands_set, sub->declared_name));
sdsfree((sds)sub->declared_name);
sdsfree(sub->fullname);
zfree(sub);
}
dictReleaseIterator(di);
dictRelease(cmd->subcommands_dict);
hashsetResetIterator(&iter);
hashsetRelease(cmd->subcommands_set);
}

return C_OK;
Expand All @@ -12082,19 +12082,19 @@ void moduleUnregisterCommands(struct ValkeyModule *module) {
/* Drain IO queue before modifying commands dictionary to prevent concurrent access while modifying it. */
drainIOThreadsQueue();
/* Unregister all the commands registered by this module. */
dictIterator *di = dictGetSafeIterator(server.commands);
dictEntry *de;
while ((de = dictNext(di)) != NULL) {
struct serverCommand *cmd = dictGetVal(de);
hashsetIterator iter;
hashsetInitSafeIterator(&iter, server.commands);
struct serverCommand *cmd;
while (hashsetNext(&iter, (void **)&cmd)) {
if (moduleFreeCommand(module, cmd) != C_OK) continue;

serverAssert(dictDelete(server.commands, cmd->fullname) == DICT_OK);
serverAssert(dictDelete(server.orig_commands, cmd->fullname) == DICT_OK);
serverAssert(hashsetDelete(server.commands, cmd->fullname));
serverAssert(hashsetDelete(server.orig_commands, cmd->fullname));
sdsfree((sds)cmd->declared_name);
sdsfree(cmd->fullname);
zfree(cmd);
}
dictReleaseIterator(di);
hashsetResetIterator(&iter);
}

/* We parse argv to add sds "NAME VALUE" pairs to the server.module_configs_queue list of configs.
Expand Down
Loading

0 comments on commit 3038293

Please sign in to comment.