Skip to content

Adding support for sharing memory between the module and the engine #2472

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 24 commits into
base: unstable
Choose a base branch
from
+466 −196
Open

Adding support for sharing memory between the module and the engine #2472

Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
9ee29c2
fixing test issue
yairgott Aug 12, 2025
a9fc017
fixing test issue
yairgott Aug 12, 2025
7dae1e6
addressing unittest issues
yairgott Aug 12, 2025
5d4c16a
addressing unittest issues
yairgott Aug 12, 2025
f826960
addressing lint
yairgott Aug 12, 2025
78b90bb
addressing comments
yairgott Aug 15, 2025
6f7f990
renaming, bug fixing and adding unittests
yairgott Aug 19, 2025
848fd6d
unittest fix
yairgott Aug 19, 2025
a7a1537
cosmetic change
yairgott Aug 19, 2025
06b6cfc
fixing module tests
yairgott Aug 19, 2025
c8a7aac
Merge remote-tracking branch 'upstream/unstable' into hash_shared
yairgott Aug 20, 2025
356891d
fixing comments
yairgott Aug 20, 2025
6913e21
addressing code review comments
Sep 5, 2025
82ef7bd
addressing lint
Sep 5, 2025
48193e8
addressing unit test failure
Sep 5, 2025
38aae4a
Update src/t_hash.c
yairgott Sep 9, 2025
5f65be7
Update src/t_hash.c
yairgott Sep 9, 2025
8224ea6
Update src/server.h
yairgott Sep 9, 2025
ed86fa0
Update src/entry.c
yairgott Sep 9, 2025
6d5a45e
addressing comments
Sep 9, 2025
2aea44d
addressing code review comments
Sep 9, 2025
37a006c
fixing module test
Sep 10, 2025
b97667a
addressing code review comments
Sep 16, 2025
bf6d7c7
addressing code review comments
Sep 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions src/aof.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1943,8 +1943,9 @@ static int rioWriteHashIteratorCursor(rio *r, hashTypeIterator *hi, int what) {
else
return rioWriteBulkLongLong(r, vll);
} else if (hi->encoding == OBJ_ENCODING_HASHTABLE) {
sds value = hashTypeCurrentFromHashTable(hi, what);
return rioWriteBulkString(r, value, sdslen(value));
size_t len;
char *value = hashTypeCurrentFromHashTable(hi, what, &len);
return rioWriteBulkString(r, value, len);
}

serverPanic("Unknown hash encoding");
Expand All @@ -1962,7 +1963,8 @@ int rewriteHashObject(rio *r, robj *key, robj *o) {
while (hashTypeNext(&hi) != C_ERR) {
long long expiry = entryGetExpiry(hi.next);
sds field = entryGetField(hi.next);
sds value = entryGetValue(hi.next);
size_t value_len;
char *value = entryGetValue(hi.next, &value_len);
if (rioWriteBulkCount(r, '*', 8) == 0) return 0;
if (rioWriteBulkString(r, "HSETEX", 6) == 0) return 0;
if (rioWriteBulkObject(r, key) == 0) return 0;
Expand All @@ -1971,7 +1973,7 @@ int rewriteHashObject(rio *r, robj *key, robj *o) {
if (rioWriteBulkString(r, "FIELDS", 6) == 0) return 0;
if (rioWriteBulkLongLong(r, 1) == 0) return 0;
if (rioWriteBulkString(r, field, sdslen(field)) == 0) return 0;
if (rioWriteBulkString(r, value, sdslen(value)) == 0) return 0;
if (rioWriteBulkString(r, value, value_len) == 0) return 0;
volatile_items++;
}
hashTypeResetIterator(&hi);
Expand Down
45 changes: 25 additions & 20 deletions src/db.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1004,6 +1004,12 @@ int objectTypeCompare(robj *o, long long target) {
return 1;
}

static void addScanDataItem(vector *result, const char *buf, size_t len) {
stringRef *item = vectorPush(result);
item->buf = buf;
item->len = len;
}

/* Hashtable scan callback used by scanCallback when scanning the keyspace. */
void keysScanCallback(void *privdata, void *entry, int didx) {
scanData *data = (scanData *)privdata;
Expand Down Expand Up @@ -1034,15 +1040,14 @@ void keysScanCallback(void *privdata, void *entry, int didx) {
}

/* Keep this key. */
sds *item = vectorPush(data->result);
*item = key;
addScanDataItem(data->result, (const char *)key, sdslen(key));
}

/* This callback is used by scanGenericCommand in order to collect elements
* returned by the dictionary iterator into a list. */
void hashtableScanCallback(void *privdata, void *entry) {
scanData *data = (scanData *)privdata;
sds val = NULL;
stringRef val = {NULL, 0};
sds key = NULL;

robj *o = data->o;
Expand All @@ -1062,7 +1067,7 @@ void hashtableScanCallback(void *privdata, void *entry) {
} else if (o->type == OBJ_HASH) {
key = entryGetField(entry);
if (!data->only_keys) {
val = entryGetValue(entry);
val.buf = entryGetValue(entry, &val.len);
}
} else {
serverPanic("Type not handled in hashtable SCAN callback.");
Expand All @@ -1084,15 +1089,15 @@ void hashtableScanCallback(void *privdata, void *entry) {
if (!data->only_keys) {
char buf[MAX_LONG_DOUBLE_CHARS];
int len = ld2string(buf, sizeof(buf), node->score, LD_STR_AUTO);
val = sdsnewlen(buf, len);
sds tmp = sdsnewlen(buf, len);
val.buf = (const char *)tmp;
val.len = sdslen(tmp);
}
}

sds *item = vectorPush(data->result);
*item = key;
if (val) {
item = vectorPush(data->result);
*item = val;
addScanDataItem(data->result, (const char *)key, sdslen(key));
if (val.buf) {
addScanDataItem(data->result, val.buf, val.len);
}
}

Expand Down Expand Up @@ -1247,7 +1252,7 @@ void scanGenericCommand(client *c, robj *o, unsigned long long cursor) {
/* scanning ZSET allocates temporary strings even though it's a dict */
free_callback = sdsfree;
}
vectorInit(&result, SCAN_VECTOR_INITIAL_ALLOC, sizeof(sds));
vectorInit(&result, SCAN_VECTOR_INITIAL_ALLOC, sizeof(stringRef));
Copy link
Member

@JimB123 JimB123 Oct 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This represents an overloading of the stringRef type for a different purpose. Should it have a different type?

In the original, it represents a reference to a non-owned string used in the t_hash entry. There are ownership considerations to maintain. Elsewhere in this review I suggested the possibility of adding a callback to that struct to avoid coupling with server event capabilities.

In this case, you just need to keep track of a pointer/length pair. I'd recommend a new type, local to db.c, rather than coupling to the type created for t_hash.

EDIT: after looking at the other code, it seems that the stringRef type is only used internally in entry.c. This should definitely be a separate type, and stringRef should be removed from server.h.

Copy link
Contributor Author

@yairgott yairgott Nov 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stringRef is a simple struct which holds a pointer to a string and a length. It's currently used by the hash entry and here but may be used in any other place instead of the pair <const char * str, size_t len>. In a way, stringRef is similar to std::string_view, which is widely used in c++.

here are ownership considerations to maintain.

Can you clarify this statement? The existing code and so the PR changes, do not take ownership of the hash field entries.

In this case, you just need to keep track of a pointer/length pair. I'd recommend a new type, local to db.c, rather than coupling to the type created for t_hash.

  1. To clarify, stringRef is not specific to t_hash and can be used in any scenario where the pair <const char * str, size_t len> is used.
  2. The alternative is to create here a local data-type which is similar to stringRef.

Copy link
Contributor Author

@yairgott yairgott Nov 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that Ran's comment is related.


/* For main hash table scan or scannable data structure. */
if (!o || ht) {
Expand Down Expand Up @@ -1308,8 +1313,8 @@ void scanGenericCommand(client *c, robj *o, unsigned long long cursor) {
if (use_pattern && !stringmatchlen(pat, sdslen(pat), key, len, 0)) {
continue;
}
sds *item = vectorPush(&result);
*item = sdsnewlen(key, len);
sds item = sdsnewlen(key, len);
addScanDataItem(&result, (const char *)item, sdslen(item));
}
setTypeReleaseIterator(si);
cursor = 0;
Expand All @@ -1329,13 +1334,13 @@ void scanGenericCommand(client *c, robj *o, unsigned long long cursor) {
continue;
}
/* add key object */
sds *item = vectorPush(&result);
*item = sdsnewlen(str, len);
sds item = sdsnewlen(str, len);
addScanDataItem(&result, (const char *)item, sdslen(item));
/* add value object */
if (!only_keys) {
str = lpGet(p, &len, intbuf);
item = vectorPush(&result);
*item = sdsnewlen(str, len);
item = sdsnewlen(str, len);
addScanDataItem(&result, (const char *)item, sdslen(item));
}
p = lpNext(o->ptr, p);
}
Expand All @@ -1350,10 +1355,10 @@ void scanGenericCommand(client *c, robj *o, unsigned long long cursor) {

addReplyArrayLen(c, vectorLen(&result));
for (uint32_t i = 0; i < vectorLen(&result); i++) {
sds *key = vectorGet(&result, i);
addReplyBulkCBuffer(c, *key, sdslen(*key));
stringRef *key = vectorGet(&result, i);
addReplyBulkCBuffer(c, key->buf, key->len);
if (free_callback) {
free_callback(*key);
free_callback((sds)(key->buf));
}
}

Expand Down
Loading
Loading