[hashset feature] Replace dict with hashset for keys and expire #1178

zuiderkwast · 2024-10-16T08:47:01Z

Keys and expire are embedded in robj.

More info to come.

keys                                      expire
hashset            robj                   hashset
+---+         +-------------------+        +---+
| 1 --------->| type, enc, lru    |<-------- 1 |
| 2 |         | refcount, flags   |        | 2 |
| 3 |         | ptr               |        | 3 |
| . |         | expire (optional) |        | . |
| . |         | embedded key      |        | . |
+---+         +-------------------+        +---+

Signed-off-by: Viktor Söderqvist <[email protected]>

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]> Co-authored-by: Rain Valentine <[email protected]>

Signed-off-by: Viktor Söderqvist <[email protected]>

* Add hashsetRehashMicroseconds * Change hashsetTwoPhasePopDelete to *not* call the element destructor * Change a field name in the hashset iterator * Add missing prototype for hashsetIsRehashingPaused Signed-off-by: Viktor Söderqvist <[email protected]>

Signed-off-by: Viktor Söderqvist <[email protected]>

…ty buckets" Signed-off-by: Viktor Söderqvist <[email protected]>

Signed-off-by: Viktor Söderqvist <[email protected]>

madolson · 2024-10-16T17:48:57Z

src/kvstore.h


-#include "dict.h"
+#include "hashset.h"


I think you forgot to include this file

This PR currently targets the hashset feature branch, where this file exists.

Do you think I should target the unstable branch instead? I'm trying to find a way to split it into smaller PRs...

How about we first create a PR to unstable with just the hashset implementation and replace dict with hashet in the command lookup table?

Oh, I missed this, I thought this was the PR for the hashset and I was interested. You can ignore me :D

No, seriously, I think it can be very hard to get a huge feature branch merged. We should split it up in some way.

The implementation of the hashset is in the hashset branch in this repo. PTAL!

SoftlyRaining

Didn't look at db.c yet

src/server.h

src/evict.c

SoftlyRaining · 2024-10-16T19:44:51Z

src/object.c

+    }
+    if (val->hasembkey) {
+        uint8_t hdr_size = *(uint8_t *)data;
+        data += 1 + hdr_size;


what if we made hdr_size into key_offset and have the +1 included in the value?

So the value we store in key_offset byte includes its own size?

Yeah, that would work. I copied this code from how keys are currently embedded in dict entry.

SoftlyRaining · 2024-10-16T19:56:25Z

src/object.c

+    sds oldkey = *(sds *)data;
+    if (oldkey != NULL) sdsfree(oldkey);


This doesn't quite make sense to me with the assert at the top of the function. If valkeyGetKey(val) == NULL then there should be nothing here - not embedded or a pointer.

You're right. This code isn't ready yet. I want to rewrite it to avoid code duplication and improve it in general.

SoftlyRaining · 2024-10-16T20:29:14Z

src/object.c

+        /* Size of embedded value (EMBSTR) including \0 term. */
+        min_size += sizeof(struct sdshdr8) + val_len + 1;


Why not call sdscopytobuffer() again? How do we know it always uses sdshdr8?

EMBSTR was always hard-coded to using sdshdr8. This is old code, refactored. I wanted to avoid rewriting it.

In the future, a good thing would be to get rid of the ptr field in robj and instead compute it using a function. Then we can save another 8 bytes for embedded strings.

SoftlyRaining · 2024-10-16T20:34:46Z

src/object.c

+        sdscopytobuffer(data + 1, key_sds_size, key, data);
+        data += 1 + key_sds_size;


What's the +1? Is that for the sds header size? Is there a reason it's not assigned?

The +1 is the byte where we store the sds header size.

Assigned? Not sure what you mean.

I mean, in this function where we allocate and initialize a brand new valkey object, when do we initialize this embedded header size value? What value is it going to have when this function ends?

The byte at data is assigned by sdscopytobuffer by the last parameter we pass to it.

SoftlyRaining · 2024-10-16T20:47:08Z

src/object.c

+        struct sdshdr8 *sh = (void *)data;
+        sh->flags = SDS_TYPE_8;
+        sh->len = val_len;
+        size_t capacity = bufsize - (min_size - val_len);


This is correct but I found it rather confusing.

Yes. It's old code, just moved/copied around. You're welcome to suggest simplification, or do it as a follow-up. I wanted to focus on getting things to work first and to avoid changing more than necessary to keep the diff less huge.

Do you have a suggestion for a code comment or different variable names?

SoftlyRaining · 2024-10-17T00:22:22Z

src/memory_prefetch.c

Think we'll implement something, or will we end up completely removing prefetch?

Yes... Help wanted. :)

src/defrag.c

Signed-off-by: Viktor Söderqvist <[email protected]>

zuiderkwast and others added 30 commits September 21, 2024 14:51

WIP new hashtable

e44f048

Signed-off-by: Viktor Söderqvist <[email protected]>

More implementation

a9178ad

Lots of changes (resize, scan, etc)

f3af277

Signed-off-by: Viktor Söderqvist <[email protected]>

Add release, delete, pop + rename and reorder stuff

0ec3218

Signed-off-by: Viktor Söderqvist <[email protected]>

Spelling

29a9422

Signed-off-by: Viktor Söderqvist <[email protected]>

clang-format (with manual rewrites to avoid superlong lines)

b34094a

Signed-off-by: Viktor Söderqvist <[email protected]>

Implement iterators, fix scan end condition

c89cd6b

Signed-off-by: Viktor Söderqvist <[email protected]>

update todo in .h file

5db23c9

Signed-off-by: Viktor Söderqvist <[email protected]>

Some updates

5a6c775

Signed-off-by: Viktor Söderqvist <[email protected]>

Add two-phase insert

cc7a25e

Signed-off-by: Viktor Söderqvist <[email protected]>

Add random element, some refac

bafce7d

Signed-off-by: Viktor Söderqvist <[email protected]>

Tuning, cleanup

e59548c

Signed-off-by: Viktor Söderqvist <[email protected]>

empty, with progress callback

e67b148

Signed-off-by: Viktor Söderqvist <[email protected]>

Add stats collection functions, untested

3580b38

Signed-off-by: Viktor Söderqvist <[email protected]>

Add two-phase pop

2d997f6

Signed-off-by: Viktor Söderqvist <[email protected]>

Add 'instant_rehashing' type flag (non-incremental)

7b00ece

Signed-off-by: Viktor Söderqvist <[email protected]>

Spellcheck

dc0ae98

Signed-off-by: Viktor Söderqvist <[email protected]>

Clang-format

638d222

Signed-off-by: Viktor Söderqvist <[email protected]>

clang-format .h file

0cc2b99

Signed-off-by: Viktor Söderqvist <[email protected]>

Build errors

a62a16f

Signed-off-by: Viktor Söderqvist <[email protected]>

add hashtabFindRef

e82b69b

Signed-off-by: Viktor Söderqvist <[email protected]>

Rename to hashset

fbccb4b

Signed-off-by: Viktor Söderqvist <[email protected]>

Add hashsetBuckets() and HASHET_BUCKET_SIZE

9c139bf

Signed-off-by: Viktor Söderqvist <[email protected]>

Try to fix 32-bit warnings

75f054a

Signed-off-by: Viktor Söderqvist <[email protected]>

Merge remote-tracking branch 'valkey/unstable' into hashset

52649ca

Add hashsetDefragInternals (defrag helper)

36e7c8a

Signed-off-by: Viktor Söderqvist <[email protected]>

Some hashset functions

9c34fe7

* Add hashsetRehashMicroseconds * Change hashsetTwoPhasePopDelete to *not* call the element destructor * Change a field name in the hashset iterator * Add missing prototype for hashsetIsRehashingPaused Signed-off-by: Viktor Söderqvist <[email protected]>

Merge remote-tracking branch 'valkey/unstable' into hashset

2b77a02

Fix wrong indexes in hashtabScan while rehashing

2c76792

Signed-off-by: Viktor Söderqvist <[email protected]>

zuiderkwast added 25 commits October 14, 2024 14:15

Fix some things including MOVE

8e045a7

Signed-off-by: Viktor Söderqvist <[email protected]>

Fix (delete before add) in RENAME command

7d47457

Signed-off-by: Viktor Söderqvist <[email protected]>

Fix memory leak when overwriting a key

b1a77cc

Signed-off-by: Viktor Söderqvist <[email protected]>

Optimizations in db.c

5907a1d

Signed-off-by: Viktor Söderqvist <[email protected]>

Optimization in dbGenericDelete for expires

1b1fb67

Signed-off-by: Viktor Söderqvist <[email protected]>

Fix leak when freeing non-string obj with key

ba298c6

Signed-off-by: Viktor Söderqvist <[email protected]>

Minor stuff in kvstore and hashset

a8ef616

Signed-off-by: Viktor Söderqvist <[email protected]>

Fix test case "expire scan should skip dictionaries with lot's of emp…

b47df73

…ty buckets" Signed-off-by: Viktor Söderqvist <[email protected]>

Fix resize allowed maxmemory check

9c9bf21

Signed-off-by: Viktor Söderqvist <[email protected]>

Fix kvstore unit tests

0538d28

Signed-off-by: Viktor Söderqvist <[email protected]>

Fix maxmemory test (increase size)

6402519

Signed-off-by: Viktor Söderqvist <[email protected]>

Delete test cases about shared integers

02450c3

Signed-off-by: Viktor Söderqvist <[email protected]>

Scan single step in active expire

50075ea

Signed-off-by: Viktor Söderqvist <[email protected]>

Seed hashset hash function on startup

6ce144b

Signed-off-by: Viktor Söderqvist <[email protected]>

Set resize policy when a fork is active, and fix test case

3a9f35b

Signed-off-by: Viktor Söderqvist <[email protected]>

Fix rehashing test case in unit/info suite

680887e

Signed-off-by: Viktor Söderqvist <[email protected]>

Fix more resize test cases in unit/other

2d0023e

Signed-off-by: Viktor Söderqvist <[email protected]>

Fix scan testcases not expecting duplicates

6a06e53

Signed-off-by: Viktor Söderqvist <[email protected]>

Another run at the unit/expire test suite

b848bf4

Signed-off-by: Viktor Söderqvist <[email protected]>

Fix typo in defrag pubsub channels

4001406

Signed-off-by: Viktor Söderqvist <[email protected]>

Make dbAddRDBLoad return the (maybe reallocated) object

dc8f62d

Signed-off-by: Viktor Söderqvist <[email protected]>

Fix LRU issue when overwriting existing key

8dd9658

Signed-off-by: Viktor Söderqvist <[email protected]>

Fix module string-DMA issue

5911b9c

Signed-off-by: Viktor Söderqvist <[email protected]>

Add missing unit test file test_object.c

7a16526

Signed-off-by: Viktor Söderqvist <[email protected]>

Account for scan duplicates in valkey-cli test

364e1f7

Signed-off-by: Viktor Söderqvist <[email protected]>

madolson reviewed Oct 16, 2024

View reviewed changes

SoftlyRaining reviewed Oct 17, 2024

View reviewed changes

Fix refcount bits in robj

cc16018

Signed-off-by: Viktor Söderqvist <[email protected]>

zuiderkwast force-pushed the hashset branch 2 times, most recently from 8fe59b3 to 3038293 Compare October 18, 2024 07:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[hashset feature] Replace dict with hashset for keys and expire #1178

[hashset feature] Replace dict with hashset for keys and expire #1178

zuiderkwast commented Oct 16, 2024 •

edited

Loading

madolson Oct 16, 2024

zuiderkwast Oct 16, 2024

zuiderkwast Oct 16, 2024

madolson Oct 16, 2024

zuiderkwast Oct 16, 2024

SoftlyRaining left a comment

SoftlyRaining Oct 16, 2024

zuiderkwast Oct 17, 2024

SoftlyRaining Oct 16, 2024

zuiderkwast Oct 17, 2024

SoftlyRaining Oct 16, 2024

zuiderkwast Oct 17, 2024

SoftlyRaining Oct 16, 2024

zuiderkwast Oct 17, 2024

SoftlyRaining Oct 17, 2024

zuiderkwast Oct 19, 2024

SoftlyRaining Oct 16, 2024

zuiderkwast Oct 17, 2024

zuiderkwast Oct 18, 2024

SoftlyRaining Oct 17, 2024

zuiderkwast Oct 17, 2024

		sds oldkey = (sds )data;
		if (oldkey != NULL) sdsfree(oldkey);

		/* Size of embedded value (EMBSTR) including \0 term. */
		min_size += sizeof(struct sdshdr8) + val_len + 1;

		sdscopytobuffer(data + 1, key_sds_size, key, data);
		data += 1 + key_sds_size;


		#include "dict.h"
		#include "hashset.h"

[hashset feature] Replace dict with hashset for keys and expire #1178

Are you sure you want to change the base?

[hashset feature] Replace dict with hashset for keys and expire #1178

Conversation

zuiderkwast commented Oct 16, 2024 • edited Loading

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 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

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

zuiderkwast commented Oct 16, 2024 •

edited

Loading