Replace dict with new hashtable: hash datatype #1502

SoftlyRaining · 2025-01-02T06:19:21Z

This PR replaces dict with the new hashtable data structure in the HASH datatype. There is a new struct for hashtable items which contains a pointer to value sds string and the embedded key sds string. These values were previously stored in dictEntry. This structure is kept opaque so we can easily add small value embedding or other optimizations in the future.

closes #1095

codecov · 2025-01-02T06:33:58Z

Codecov Report

Attention: Patch coverage is 91.40625% with 22 lines in your changes missing coverage. Please review.

Project coverage is 70.91%. Comparing base (9e02049) to head (bc7d761).
Report is 5 commits behind head on unstable.

Files with missing lines	Patch %	Lines
src/defrag.c	73.91%	6 Missing ⚠️
src/module.c	0.00%	6 Missing ⚠️
src/rdb.c	83.33%	5 Missing ⚠️
src/t_hash.c	97.97%	3 Missing ⚠️
src/db.c	92.30%	1 Missing ⚠️
src/debug.c	50.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable    #1502      +/-   ##
============================================
- Coverage     70.93%   70.91%   -0.03%     
============================================
  Files           120      120              
  Lines         64981    64965      -16     
============================================
- Hits          46095    46068      -27     
- Misses        18886    18897      +11

Files with missing lines	Coverage Δ
src/aof.c	`80.23% <100.00%> (ø)`
src/lazyfree.c	`86.11% <100.00%> (ø)`
src/object.c	`82.04% <100.00%> (-0.11%)`	⬇️
src/server.c	`87.60% <100.00%> (+0.02%)`	⬆️
src/server.h	`100.00% <ø> (ø)`
src/db.c	`89.56% <92.30%> (+0.07%)`	⬆️
src/debug.c	`52.40% <50.00%> (+0.27%)`	⬆️
src/t_hash.c	`96.14% <97.97%> (+<0.01%)`	⬆️
src/rdb.c	`76.90% <83.33%> (-0.04%)`	⬇️
src/defrag.c	`91.92% <73.91%> (+1.95%)`	⬆️
... and 1 more

... and 13 files with indirect coverage changes

zuiderkwast

I skimmed through it. I looks strait-forward. The only material thing to focus on seems to be the hashTypeEntry abstraction.

It would be good to have a clear abstraction and separation, even within t_hash.c, so we don't have too many direct accesses into the hashTypeEntry internals. It will help us later when we want to add things like TTL and embedded value.

src/server.h

src/defrag.c

Signed-off-by: Rain Valentine <[email protected]>

…eld/value pairs Signed-off-by: Rain Valentine <[email protected]>

zuiderkwast

Awesome! Just a few nits.

Introducing the sds_const type can preferably be a follow-up. It might trigger a long discussion.

zuiderkwast · 2025-01-09T10:46:30Z

src/t_hash.c

+/* takes ownership of value, does not take ownership of field */
+hashTypeEntry *hashTypeCreateEntry(const sds field, sds value) {


const sds is meaningless. Is it time to introduce the const_sds typedef in sds.h now?

Take ownership of field but not value seems asymmetrical, but anything else is suboptimal, right? That's what we do for keys too IIRC. It's OK.

Shall we add a comment for the entry API, like the one below for the hash type API?

/*----------------------------------------------------------------------------- * Hash entry API *----------------------------------------------------------------------------*/

zuiderkwast · 2025-01-09T13:52:03Z

src/t_hash.c

+            hashTypeEntry *hash_entry = entry;
+            sds sds_val = hashTypeEntryGetValue(hash_entry);


The entry can be cast implicitly. You did that for sds sds_field = hashTypeEntryGetField(entry); a few lines above.

Suggested change

hashTypeEntry *hash_entry = entry;

sds sds_val = hashTypeEntryGetValue(hash_entry);

sds sds_val = hashTypeEntryGetValue(entry);

zuiderkwast · 2025-01-09T14:10:32Z

src/defrag.c

 static void scanLaterSet(robj *ob, unsigned long *cursor) {
    if (ob->type != OBJ_SET || ob->encoding != OBJ_ENCODING_HASHTABLE) return;
    hashtable *ht = ob->ptr;
    *cursor = hashtableScanDefrag(ht, *cursor, activeDefragSdsHashtableCallback, NULL, activeDefragAlloc, HASHTABLE_SCAN_EMIT_REF);
 }

+static void activeDefragHashTypeEntry(void *privdata, void *element_ref) {


Please add a comment saying it's used as a scan callback.

zuiderkwast · 2025-01-09T14:20:51Z

src/t_hash.c

+sds hashTypeEntryGetField(const hashTypeEntry *entry) {
+    const unsigned char *field = entry->field_data + entry->field_offset;
+    return (sds)field;


This (and the GetValue) should probably return const_sds if we add that type.

zuiderkwast · 2025-01-09T14:22:44Z

src/t_hash.c

+    return size;
+}
+
+hashTypeEntry *hashTypeEntryDefrag(hashTypeEntry *entry, void *(*defragfn)(void *), sds (*sdsdefragfn)(sds)) {


Please add a short comment above each of the functions, at least the non-trivial ones like this one.

zuiderkwast · 2025-01-09T14:29:04Z

src/t_hash.c

-            sdsfree(dictGetVal(existing));
-            dictSetVal(ht, existing, v);
+            /* exists: replace value */
+            hashTypeEntryReplaceValue(existing, v);


I think hashTypeEntryReplaceValue should return the entry, so we don't have to change the signature in the future. With embedded value, this function may need to reallocate the entry.

For now, we can just assert that it returns existing.

Suggested change

hashTypeEntryReplaceValue(existing, v);

serverAssert(hashTypeEntryReplaceValue(existing, v) == existing);

zuiderkwast · 2025-01-09T14:33:06Z

src/t_hash.c

            if (withvalues) value = hashTypeCurrentObjectNewSds(&hi, OBJ_HASH_VALUE);
-            ret = dictAdd(d, key, value);
+            ret = dictAdd(d, field, value);


Follow-up: Let's replace this sdsReplyDictType with a hashtable.

zuiderkwast · 2025-01-09T14:33:43Z

src/t_hash.c

@@ -1069,25 +1116,25 @@ void hrandfieldWithCountCommand(client *c, long l, int withvalues) {
    else {
        /* Hashtable encoding (generic implementation) */
        unsigned long added = 0;
-        listpackEntry key, value;
+        listpackEntry field, value;
        dict *d = dictCreate(&hashDictType);


Follow-up: Let's replace this dict with a hashtable. Then we can delete that dict type.

SoftlyRaining changed the title ~~[draft] Replace dict with new hashtable: sorted set datatype~~ [draft] Replace dict with new hashtable: hash datatype Jan 2, 2025

zuiderkwast self-requested a review January 2, 2025 09:45

zuiderkwast reviewed Jan 2, 2025

View reviewed changes

src/server.h Outdated Show resolved Hide resolved

src/defrag.c Outdated Show resolved Hide resolved

SoftlyRaining force-pushed the hash-datatype branch 2 times, most recently from 61772d3 to d357204 Compare January 4, 2025 00:22

SoftlyRaining added 2 commits January 8, 2025 21:24

Replace dict with new hashtable: hash datatype

8b8fe3b

Signed-off-by: Rain Valentine <[email protected]>

make hashTypeEntry opaque and consistent naming: hash keys contain fi…

bc7d761

…eld/value pairs Signed-off-by: Rain Valentine <[email protected]>

SoftlyRaining force-pushed the hash-datatype branch from d357204 to bc7d761 Compare January 8, 2025 21:33

SoftlyRaining marked this pull request as ready for review January 8, 2025 21:34

SoftlyRaining changed the title ~~[draft] Replace dict with new hashtable: hash datatype~~ Replace dict with new hashtable: hash datatype Jan 8, 2025

zuiderkwast added release-notes This issue should get a line item in the release notes run-extra-tests Run extra tests on this PR (Runs all tests from daily except valgrind and RESP) labels Jan 9, 2025

zuiderkwast reviewed Jan 9, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace dict with new hashtable: hash datatype #1502

Replace dict with new hashtable: hash datatype #1502

SoftlyRaining commented Jan 2, 2025 •

edited

Loading

codecov bot commented Jan 2, 2025 •

edited

Loading

zuiderkwast left a comment

zuiderkwast left a comment

zuiderkwast Jan 9, 2025

zuiderkwast Jan 9, 2025

zuiderkwast Jan 9, 2025

zuiderkwast Jan 9, 2025

zuiderkwast Jan 9, 2025

zuiderkwast Jan 9, 2025

zuiderkwast Jan 9, 2025

zuiderkwast Jan 9, 2025

		/* takes ownership of value, does not take ownership of field */
		hashTypeEntry *hashTypeCreateEntry(const sds field, sds value) {

		hashTypeEntry *hash_entry = entry;
		sds sds_val = hashTypeEntryGetValue(hash_entry);

	hashTypeEntry *hash_entry = entry;
	sds sds_val = hashTypeEntryGetValue(hash_entry);
	sds sds_val = hashTypeEntryGetValue(entry);

	hashTypeEntryReplaceValue(existing, v);
	serverAssert(hashTypeEntryReplaceValue(existing, v) == existing);

Replace dict with new hashtable: hash datatype #1502

Are you sure you want to change the base?

Replace dict with new hashtable: hash datatype #1502

Conversation

SoftlyRaining commented Jan 2, 2025 • edited Loading

codecov bot commented Jan 2, 2025 • edited Loading

Codecov Report

zuiderkwast left a comment

Choose a reason for hiding this comment

zuiderkwast left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SoftlyRaining commented Jan 2, 2025 •

edited

Loading

codecov bot commented Jan 2, 2025 •

edited

Loading