Refactor dict restoring abstraction#3561
Conversation
| * the "htdict" prefix is used to avoid colliding with the "dict" in libvalkey */ | ||
| #define dictCreate(type) htdictCreate(type) | ||
| #define dictExpand(d, size) htdictExpand(d, size) | ||
| #define dictSetKey(d, de, key) htdictSetKey(d, de, key) |
There was a problem hiding this comment.
I want to eliminate this function. "set key" is an anti-pattern for a dict structure. The only place this is being used is for one use-case in expire.c. We could have fixed that with the key dup callback, but that callback was eliminated in the transition to hashtable.
Might just need a better API which addresses the use case in expire.
There was a problem hiding this comment.
Or would it be feasible to refactor that use case to use hashtable directly?
I got slightly distracted - turns out it was easy. I had a PR ready (#3566) before I remembered I was reviewing here 😓
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## unstable #3561 +/- ##
============================================
- Coverage 76.35% 76.21% -0.15%
============================================
Files 159 159
Lines 80054 80075 +21
============================================
- Hits 61125 61028 -97
- Misses 18929 19047 +118
🚀 New features to boost your workflow:
|
Signed-off-by: Jim Brunner <brunnerj@amazon.com>
There was a problem hiding this comment.
Looks good overall, though I think there are couple things to fix. I do think keeping the opaqueness of dict is the right move, and we don't have to give up the performance gain either!
I'm a little disappointed with the dict naming conflict from libvalkey, and all the #define boilerplate in dict.h. We didn't have to do this before this change (apparently?), so my (unresearched) hunch is that there should be some way to avoid the boilerplate. Could we remove dict.h from server.h and only include it where dict is used, or something like that? 🤔
| } dictEntry; | ||
|
|
||
| #define UNUSED(V) ((void)V) | ||
| typedef struct dictEntry dictEntry; // opaque |
There was a problem hiding this comment.
nit: /* opaque */ (Valkey style doesn't use // comments)
| * This file implements the dict API as a thin wrapper of the newer hashtable | ||
| * API. The dictEntry struct is used as the entry type in underlying hashtable. | ||
| * | ||
| * Copyright (c) 2006-2012, Redis Ltd. |
There was a problem hiding this comment.
This is a new file, so we should be giving Valkey credit, not Redis. Put this at the top instead and get rid of that redistribution/disclaimer cruft too:
/*
* Copyright Valkey Contributors.
* All rights reserved.
* SPDX-License-Identifier: BSD 3-Clause
*/|
|
||
| double htdictIncrDoubleVal(dictEntry *de, double val) { | ||
| de->v.d += val; | ||
| return de->v.d; |
There was a problem hiding this comment.
nit: imo these ultra short alphabet soup names are hard to read as a reviewer. I assume this is for compatibility with old dict-using code though? otherwise I'd kinda prefer entry->val.doubleval over de->v.d for example
| * Always returns 1 to indicate the key was consumed (either added or used | ||
| * to replace). The caller should not free the key after calling this. */ | ||
| int htdictReplace(dict *d, void *key, void *val) { | ||
| dictEntry *entry = (dictEntry *)zmalloc(sizeof(*entry)); |
There was a problem hiding this comment.
You can avoid the temporary allocation when the key already exists if you use the hashtableFindPositionForInsert pattern here. (Good comment in hashtable.c) You can pass just the key and find whether it exists, then only allocate and build the entry if actually needed, otherwise you replace the existing entry's value.
| * the "htdict" prefix is used to avoid colliding with the "dict" in libvalkey */ | ||
| #define dictCreate(type) htdictCreate(type) | ||
| #define dictExpand(d, size) htdictExpand(d, size) | ||
| #define dictSetKey(d, de, key) htdictSetKey(d, de, key) |
There was a problem hiding this comment.
Or would it be feasible to refactor that use case to use hashtable directly?
I got slightly distracted - turns out it was easy. I had a PR ready (#3566) before I remembered I was reviewing here 😓
2 participants
The recent update to
dict(#3366) improves performance by makingdicta thin wrapper on top of thehashtableimplementation.As part of that refactoring, we lost some of our
dictabstraction. ThedictEntrybecame public (again). The defrag code was diving into the entry directly.This update:
dictabstraction by makingdictEntryopaque (again)dict(out of defrag).cfile rather than the.hfile (allowing for opaqueness in the data structure and code)dictEntryGetKeyon everydict(it's essentially a required constant)Link time optimization (LTO) will result in the same inlining of functions, however it can now use configurable options to tune the level of inlining. This potentially reduces L1 cache bloat.