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

yairgott · 2025-08-12T04:57:26Z

Overview

Sharing memory between the module and engine reduces memory overhead by eliminating redundant copies of stored records in the module. This is particularly beneficial for search workloads that require indexing large volumes of documents.

Vectors

Vector similarity search requires storing large volumes of high-cardinality vectors. For example, a single vector with 512 dimensions consumes 2048 bytes, and typical workloads often involve millions of vectors. Due to the lack of a memory-sharing mechanism between the module and the engine, valkey-search currently doubles memory consumption when indexing vectors, significantly increasing operational costs. This limitation introduces adoption friction and reduces valkey-search's competitiveness.

Memory Allocation Strategy

At a fundamental level, there are two primary allocation strategies:

[Chosen] Module-allocated memory shared with the engine.
Engine-allocated memory shared with the module.

For valkey-search, it is crucial that vectors reside in cache-aligned memory to maximize SIMD optimizations. Allowing the module to allocate memory provides greater flexibility for different use cases, though it introduces slightly higher implementation complexity.

Old Implementation

The old implementation was based on ref-counting and introduced a new SDS type. After further discussion, we agreed to simplify the design by removing ref-counting and avoiding the introduction of a new SDS type.

New Implementation - Key Points

The engine exposes a new interface, VM_HashSetViewValue, which set value as a view of a buffer which is owned by the module. The function accepts the hash key, hash field, and a buffer along with its length.
ViewValue is a new data type that captures the externalized buffer and its length.

valkey-search Usage

Insertion

Upon receiving a key space notification for a new hash or JSON key with an indexed vector attribute, valkey-search allocates cache-aligned memory and deep-copies the vector value.
valkey-search then calls VM_HashSetViewValue to avoid keeping two copies of the vector.

Deletion

When receiving a key space notification for a deleted hash key or hash field that was indexed as a vector, valkey-search deletes the corresponding entry from the index.

Update

Handled similarly to insertion.

Signed-off-by: yairgott <[email protected]>

ranshid

@yairgott placing some high level comments first:

Not sure I like the ViewValue naming. I think the intention is to keep a "string" pointer and a length right? in that case maybe we should simply do that and call it a StringValue ?
I did not understand why we have to change the entryGetValue API? I would prefer to keep a separate API to get the "string" value like entryGetStringValue (or in your case entryGetViewValue)

src/entry.c

Signed-off-by: yairgott <[email protected]>

yairgott · 2025-08-15T17:31:36Z

Not sure I like the ViewValue naming. I think the intention is to keep a "string" pointer and a length right? in that case maybe we should simply do that and call it a StringValue ?

Renamed it to bufferView.

I did not understand why we have to change the entryGetValue API? I would prefer to keep a separate API to get the "string" value like entryGetStringValue (or in your case entryGetViewValue)

entryGetValue is called in many different places, including outside of entry.c and t_hash.c. Implementing a dedicated API to get the view value, would lead to wrapping each entryGetValue call with a check wether the value is a view or not.

codecov · 2025-08-15T17:31:47Z

Codecov Report

❌ Patch coverage is 67.14976% with 68 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.18%. Comparing base (23112fa) to head (bf6d7c7).
⚠️ Report is 100 commits behind head on unstable.

Files with missing lines	Patch %	Lines
src/entry.c	62.50%	45 Missing ⚠️
src/t_hash.c	61.53%	20 Missing ⚠️
src/module.c	0.00%	2 Missing ⚠️
src/ziplist.c	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable    #2472      +/-   ##
============================================
+ Coverage     72.03%   72.18%   +0.15%     
============================================
  Files           126      127       +1     
  Lines         70490    70914     +424     
============================================
+ Hits          50774    51187     +413     
- Misses        19716    19727      +11

Files with missing lines	Coverage Δ
src/aof.c	`81.13% <100.00%> (-0.06%)`	⬇️
src/db.c	`93.19% <100.00%> (+3.18%)`	⬆️
src/rdb.c	`76.75% <100.00%> (-0.25%)`	⬇️
src/server.h	`100.00% <ø> (ø)`
src/ziplist.c	`15.18% <0.00%> (ø)`
src/module.c	`9.81% <0.00%> (+0.27%)`	⬆️
src/t_hash.c	`94.65% <61.53%> (-1.58%)`	⬇️
src/entry.c	`80.00% <62.50%> (-15.41%)`	⬇️

... and 32 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

ranshid · 2025-08-17T12:20:28Z

I am still providing high level comments (although I do have detailed comments) since IMO we should solve the highlevel first. I think we should have a clear definition of the entry API.

Currently the entry is defined as a storage for sds type field and sds type value. In your suggestion this is changed and an entry can be provided with a value which is either an sds or a pointer to a native string and will INTERNALLY encode it the way it would like to.

Lets list the reason for this change IIUC:

The current entry API only supports sds value types. For different cases (e.g VSS module) it might be needed to have hashes keep string references which are NOT sds types.
The entry is taking ownership of the value sds. In some cases it is needed that the entry will only keep a reference of the value and will NOT own the value (i.e not free it when deleted and not account for the memory as part of the object)

Your suggestion is to change the entry API so:

It will be able to accept both sds type values AND string-references.
When a string reference is provided the entry will NOT take ownership over it.
string-references will NOT be embedded (following (2)) so it implies these entries will always have the FIELD_SDS_AUX_BIT_ENTRY_HAS_VALUE_PTR set.
string-references will NOT be accounted as the entry memory usage in entryMemUsage (I did not notice it is handled, and actually maybe there is a bug there)
value getters will not allow access to the value in the encoding it was provided. This means that although entry allowed providing the input value as sds and is INTERNALLY encoding it as sds, there is no way to retrieve a reference to the sds value.
Entry which is set with string-reference value, is ALWAYS expected to provide string reference value (?)

I think that following this suggestion I would handle the following cases:

switch between string-reference and sds value encodings - Not sure how this is currently handled correctly. I see that entrySetValuePtr, is assuming (6) ?. I think that the entryCreate and entryUpdate APIs should allow providing value as either sds or string reference. this can be done either by providing different functions prototypes for each (which is complicated) or by adding some API options like:

/* In this case the API will validate that either the value is provided OR the vref and assert otherwise */
entry *entryCreate(const_sds field, sds value, char* vref, size_t vlen, long long expiry);
entry *entryUpdate(entry *e, sds value, char* vref, size_t vlen, long long expiry);

OR

/* In this case the user will always have to provide the string value pointer and size but indicate explicitly that the value is sds.
entry *entryCreate(const_sds field, char* value, size_t vlen, bool, value_is_sds, long long expiry);
entry *entryUpdate(entry *e, char* value, size_t vlen, bool, value_is_sds, long long expiry);

OR

/* In this case the user will always have to provide the string value pointer and size but indicate explicitly that the value is sds by passing vlen zero (0). I think this is little bit prone to bugs, and an explicit way to  indicate the inout is sds or not might be better.
entry *entryCreate(const_sds field, char* value, size_t vlen, long long expiry);
entry *entryUpdate(entry *e, char* value, size_t vlen, long long expiry);

the entryGetValue change is generally fine, but it should be clear and reflected though the entire entry documentation.
I also think that we should provide a way to access the value as sds if it was encoded this way.
value/bufferView - IMO the view is not a great word for wht it is. personally stringRef reflects the real usage and provides a fine indication of what and why the entry will use it the way it does.

I also think the top comment might be missing some more alternatives that we consider(?):
for example:

add reference count option to entry. (probably not a good fit for VSS module case... but can we explain exactly why?)
create a new reference count sds type (you did provide some link and explanation to why we did not go this way)
more options if we have them...

yairgott · 2025-08-18T16:20:16Z

string-references will NOT be accounted as the entry memory usage in entryMemUsage (I did not notice it is handled, and actually maybe there is a bug there)

Fixed entryMemUsage

there is no way to retrieve a reference to the sds value.

entryGetValueRef is still available, and remains static to entry.c.

Entry which is set with string-reference value, is ALWAYS expected to provide string reference value

I'm not 100% clear but I'll note that outside of entry.c, one should still be using the public interface entryGetValue.

I think that the entryCreate and entryUpdate APIs should allow providing value as either sds or string reference.

entryCreate - creating an entry with a value view is not supported. An entry may switch to store a value view by calling t_hash.c, hashTypeSetValueView. For safety, I've incorporated a debug mode verification that the provided view buffer matches the entry SDS.

entryUpdate is modified to support updating view value just with expiration field. FWIW, entrySetValue was primary changed to improved code reuse and avoid code duplication in entryUpdate, see entrySetValuePtr.

value/bufferView - IMO the view is not a great word for wht it is. personally stringRef reflects the real usage and provides a fine indication of what and why the entry will use it the way it does.

Naming is hard :) The name bufferView is inspired by the C++ std::string_view.Also, IMO:

I think that buffer, rather than string, makes a better fit in this case.
The name reference could be confusing due to it connotation with C++.

ranshid · 2025-08-18T18:07:51Z

string-references will NOT be accounted as the entry memory usage in entryMemUsage (I did not notice it is handled, and actually maybe there is a bug there)

Fixed entryMemUsage

there is no way to retrieve a reference to the sds value.

entryGetValueRef is still available, and remains static to entry.c.

entryGetValueRef, is meant for internal use and will not work in all cases, so it is not fit for a public API. if, for example I am a user of entry and I provided an entry with sds and I need to continue using an sds from that entry, I have no way of doing so aside for creating a new sds.
like:

sds my_private_data.
entry *e = entryCreate(field, my_private_data, expiry);
...
// when I need to use the value sds for some cases, I am forced to create a new sds:
sds my_private_data = entryGetValue(e, &len); // X - will not work
sds my_private_data = sdsnewlen(entryGetValue(e, &len), len); //  will work, but will require allocating and making sure to deallocate after use.

Entry which is set with string-reference value, is ALWAYS expected to provide string reference value

I'm not 100% clear but I'll note that outside of entry.c, one should still be using the public interface entryGetValue.

I think that the entryCreate and entryUpdate APIs should allow providing value as either sds or string reference.

entryCreate - creating an entry with a value view is not supported. An entry may switch to store a value view by calling t_hash.c, hashTypeSetValueView. For safety, I've incorporated a debug mode verification that the provided view buffer matches the entry SDS.

entryUpdate is modified to support updating view value just with expiration field. FWIW, entrySetValue was primary changed to improved code reuse and avoid code duplication in entryUpdate, see entrySetValuePtr.

From reading the code I see that:

calling entryUpdate with a value which is not a "view" will just assert when the entry has a view already
entrySetValue will just assert as well because of (1)

I think this needs to be clear. If the entry is allowing to change an entry which is created with an sds value to an entry which has a "view", why we cannot do the opposite? and why do we expose a public API that is simply asserting instead of preventing this in some way? I think that as the entry is a stand-alone module, it should be generic and flexible, or the API should be restrictive and not allow what is simply not supported.

value/bufferView - IMO the view is not a great word for wht it is. personally stringRef reflects the real usage and provides a fine indication of what and why the entry will use it the way it does.

Naming is hard :) The name bufferView is inspired by the C++ std::string_view.Also, IMO:

I think that buffer, rather than string, makes a better fit in this case.

The name reference could be confusing due to it connotation with C++.

Well we are not doing c++ unfortunately, and pointers here are treated as references IMO. So I think it is a better name, but will not make this the main point of the review. I just think that the way to distinguish a "native c style string" to what you have which requires to ALWAYS keep the provided reference is to use a name like stringref.

To conclude. I would like to have a complete API which is both generic and standalone together with supportive to the existing usecases we have. As I mentioned before, we could allow entry to accept both sds value and "native c style strings" and encode it internally which is subject to the internal implementation which should prefer memory efficiency and performance (avoid large copies and better cache line locality).

For first thing we should solve the part of the API which accepts values if we plan to NOT allow accepting . How do we handle the HSET or HINCRBYcommands? do we intercept it from the module in case of the VSS? so how should other modules handle it?

yairgott · 2025-08-18T19:03:53Z

From reading the code I see that:

calling entryUpdate with a value which is not a "view" will just assert when the entry has a view already

entrySetValue will just assert as well because of (1)

Right, I'll fix this. The idea is to handle adding expiry to a view value. Otherwise, the existing code, with slight changes, will handle updating a view value.

value/bufferView - IMO the view is not a great word for wht it is. personally stringRef reflects the real usage and provides a fine indication of what and why the entry will use it the way it does.

Naming is hard :) The name bufferView is inspired by the C++ std::string_view.Also, IMO:

I think that buffer, rather than string, makes a better fit in this case.

The name reference could be confusing due to it connotation with C++.

Well we are not doing c++ unfortunately, and pointers here are treated as references IMO. So I think it is a better name, but will not make this the main point of the review. I just think that the way to distinguish a "native c style string" to what you have which requires to ALWAYS keep the provided reference is to use a name like stringref.

I'm not religious about the name, if you feel strongly about the stringref, I'll just change it.

To conclude. I would like to have a complete API which is both generic and standalone together with supportive to the existing usecases we have. As I mentioned before, we could allow entry to accept both sds value and "native c style strings" and encode it internally which is subject to the internal implementation which should prefer memory efficiency and performance (avoid large copies and better cache line locality).

I think that making the API complete is not really an objective but rather supporting the use-cases. Take for example entryCreate where there is no use-case which requires creation of a stringref value entry, but rather a stringref value entry is always triggered by the module on an existing entry. Adding support for a "complete" API would add another layer of complexity without providing any value. entryCreate, entryUpdate APIs receive value as SDS, which makes it clear that a stringref object is not supported.

For first thing we should solve the part of the API which accepts values if we plan to NOT allow accepting . How do we handle the HSET or HINCRBYcommands? do we intercept it from the module in case of the VSS? so how should other modules handle it?

Any update to the entry is handled by a call to entryUpdate. The module registers event keyspace notification which is triggered by commands like HSET or HINCRBY. The module event handling logic involves reading the entry and handling the mutation accordingly.

ranshid · 2025-08-18T19:20:00Z

From reading the code I see that:

calling entryUpdate with a value which is not a "view" will just assert when the entry has a view already

entrySetValue will just assert as well because of (1)

Right, I'll fix this. The idea is to handle adding expiry to a view value. Otherwise, the existing code, with slight changes, will handle updating a view value.

value/bufferView - IMO the view is not a great word for wht it is. personally stringRef reflects the real usage and provides a fine indication of what and why the entry will use it the way it does.

Naming is hard :) The name bufferView is inspired by the C++ std::string_view.Also, IMO:

I think that buffer, rather than string, makes a better fit in this case.

The name reference could be confusing due to it connotation with C++.

Well we are not doing c++ unfortunately, and pointers here are treated as references IMO. So I think it is a better name, but will not make this the main point of the review. I just think that the way to distinguish a "native c style string" to what you have which requires to ALWAYS keep the provided reference is to use a name like stringref.

I'm not religious about the name, if you feel strongly about the stringref, I'll just change it.

To conclude. I would like to have a complete API which is both generic and standalone together with supportive to the existing usecases we have. As I mentioned before, we could allow entry to accept both sds value and "native c style strings" and encode it internally which is subject to the internal implementation which should prefer memory efficiency and performance (avoid large copies and better cache line locality).

I think that making the API complete is not really an objective but rather supporting the use-cases. Take for example entryCreate where there is no use-case which requires creation of a stringref value entry, but rather a stringref value entry is always triggered by the module on an existing entry. Adding support for a "complete" API would add another layer of complexity without providing any value. entryCreate, entryUpdate APIs receive value as SDS, which makes it clear that a stringref object is not supported.

I think that the entry should keep a clear and concrete API. this API is not going to be used ONLY by the search module, but potentially by other future parts of the application as well as future modules, and it would be great if we could make the API complete. But let me try and track it better in the detailed review.

For first thing we should solve the part of the API which accepts values if we plan to NOT allow accepting . How do we handle the HSET or HINCRBYcommands? do we intercept it from the module in case of the VSS? so how should other modules handle it?

Any update to the entry is handled by a call to entryUpdate. The module registers event keyspace notification which is triggered by commands like HSET or HINCRBY. The module event handling logic involves reading the entry and handling the mutation accordingly.

So that would require to handle entryUpdate correctly. I see that you suggest you will fix that, so I will followup on that.

ranshid · 2025-08-18T19:20:58Z

@yairgott also please make a run through the current documentation and structure ascii and change them accordingly.

Signed-off-by: yairgott <[email protected]>

yairgott · 2025-08-19T11:46:44Z

So that would require to handle entryUpdate correctly. I see that you suggest you will fix that, so I will followup on that.

entryUpdate already works correctly. Let me know if you find any issues. I've also added unittests.

also please make a run through the current documentation and structure ascii and change them accordingly.

Sure.

Also, noting that I've done the renaming to stringRef.

Signed-off-by: yairgott <[email protected]>

ranshid

went though some more stuff. will continue tomorrow

src/entry.c

src/entry.h

src/entry.c

ranshid · 2025-08-20T15:10:59Z

src/entry.c

    return entryWrite(buf, buf_size, field, value, expiry, embed_value, embedded_field_sds_type, embedded_field_sds_size, embedded_value_sds_size, expiry_size);
 }

+entry *entrySetStringRef(entry *entry, const char *buf, size_t len, long long expiry) {


To me it sounds like we are duplicating some stuff form entryUpdate. The way I imagined it is that the entry can be encoded either like:

1. field | embedded value 2. value ptr | field 3. ttl | field | embedded value 4. ttl | value ptr | field 5. vstring | vlen | field 6. ttl | vstring | vlen | field

So entryUpdate should be able to navigate between ALL these cases and as such can be used inside entrySetStringRef.

I also still not a big fan of the fact that there is no real way to create an entry with a stringRef. Maybe it is not needed right now, but the API seems strange that way IMO.

Decoupling with a dedicated function, rather than extending entryUpdate to support string ref, is much more straight forward both to implement and to maintain. Extending entryUpdate would involve:

Supporting both types of input values, sds and [char * buf, size_t len].

Extending the logic to properly handle all the possible combinations.

In general, we should strive for code which is easy to maintain and resilient. IMO entryUpdate is already too complicated and it already handles too many different cases which adds complexity to follow and to reason about.

In term of code reuse, I will try to explore how to improve the code section in lines 324-342.

Edited!

I would like to suggest the following changes to improve the code quality:

Starting with entryUpdate:

entry *entryUpdate(entry *e, sds value, long long expiry) { if (entryChangeLayout(e, value, expiry)) { entry *new_entry = entryCreate((sds)e, value, expiry); entryFree(e); return new_entry; } // Layout was not changed, just apply the value and the expiry entryReplaceValue(e, value); if (expiry != EXPIRY_NONE) entryReplaceExpiry(e, expiry); return e; }

Now that entryReqSize is called just by entryCreate and therefor its logic can be embedded inside entryCreate and refactored to greatly simplified.

If agreed with the above, I can drive this changes but I prefer to drive this after this PR is landed. WDYT?

src/module.c

src/t_hash.c

zuiderkwast · 2025-08-25T15:30:15Z

We discussed this in the core team meeting today.

If we've closed on the design and it's been reviewed by next Monday, we can merge it to 9.0 RC 2, but otherwise we can postpone it to 9.1.

madolson · 2025-09-01T15:15:58Z

If we've closed on the design and it's been reviewed by next Monday, we can merge it to 9.0 RC 2, but otherwise we can postpone it to 9.1.

It's next monday. I guess we'll move this to 9.1.

yairgott · 2025-09-03T05:44:38Z

If we've closed on the design and it's been reviewed by next Monday, we can merge it to 9.0 RC 2, but otherwise we can postpone it to 9.1.

It's next monday. I guess we'll move this to 9.1.

Are there any reservations about the design? please correct me if I misunderstood but based on the comments, my understanding is that there are a couple of implementation details which are needed to be sort out but there are no push backs on the design itself.

Signed-off-by: Yair Gottdenker <[email protected]>

src/t_hash.c

src/server.h

src/entry.c

src/t_hash.c

src/entry.c

Co-authored-by: Ran Shidlansik <[email protected]> Signed-off-by: Yair Gottdenker <[email protected]>

Signed-off-by: Yair Gottdenker <[email protected]>

src/t_hash.c

Signed-off-by: Yair Gottdenker <[email protected]>

ranshid · 2025-10-08T08:08:02Z

Overall the code changes LGTM.

NOTE: this might have future conflicts with #2618

@valkey-io/core-team how can we progress the major-decision-pending checkout? does any other maintainer wish to go over and check this?

madolson · 2025-10-27T14:05:32Z

@ranshid Hey, what specifically do you want the core team to address?

madolson · 2025-10-27T14:18:22Z

We reviewed the APIs, they seem minimal but there are no concerns with it. @JimB123 since Ran asked for another pair of eyes, PTAL. Also Ran, please approve if you are happy with the PR.

JimB123 · 2025-10-28T16:51:59Z

@JimB123 since Ran asked for another pair of eyes, PTAL. Also Ran, please approve if you are happy with the PR.

Ack. Reviewing.

JimB123 · 2025-10-28T17:26:13Z

src/module.c

+/* Sets a reference to the value.
+ * The function takes the hash key, hash field, and a buffer along with its length. */


This is insufficient documentation to understand the purpose and use of these APIs. We need to explain when/why this function should be used and what are the implications of using it.

I think it's assumed that the value should remain fundamentally the same in content, right? This isn't documented or validated.

How is this undone? What happens at deletion time? or when the value is modified by an HSET?

JimB123 · 2025-10-28T18:06:23Z

tests/modules/hash_stringref.c

@@ -0,0 +1,77 @@
+#include "valkeymodule.h"


There's no documentation regarding the purpose of this test/example.

JimB123 · 2025-10-28T18:07:56Z

tests/modules/hash_stringref.c

+    VALKEYMODULE_NOT_USED(argc);
+    if (ValkeyModule_Init(ctx, "hash.stringref", 1, VALKEYMODULE_APIVER_1) ==
+        VALKEYMODULE_OK &&
+        ValkeyModule_CreateCommand(ctx, "hash.set_stringref", hashSetStringRef, "write",


There's no documentation regarding the format/parameters of this command. A reader has no way to know what the numbered parameters represent.

JimB123 · 2025-10-28T18:08:02Z

tests/modules/hash_stringref.c

+        VALKEYMODULE_OK &&
+        ValkeyModule_CreateCommand(ctx, "hash.set_stringref", hashSetStringRef, "write",
+                                  1, 1, 1) == VALKEYMODULE_OK &&
+        ValkeyModule_CreateCommand(ctx, "hash.has_stringref", hashHasStringRef, "write",


There's no documentation regarding the format/parameters of this command. A reader has no way to know what the numbered parameters represent.

Also, is it correct that a command called "has_stringref" should be marked as a "write" command?

JimB123 · 2025-10-28T21:33:20Z

src/entry.c

 /* Returns the location of a pointer to a separately allocated value. Only for
 * an entry without an embedded value. */
-static sds *entryGetValueRef(const entry *entry) {
+static inline void **entryGetValueRef(const entry *entry) {


There are times when void is necessary. Allocation routines, for example, have no idea of the type that's being allocated. But that's not really the case here.

In this case, we have 2 distinct possibilities. In most(*) of the cases where this function is being used, the caller must check which case is being used and then cast the void into the proper type.
(*) Note, there is one place in updateValue that simply nulls out the reference.

I'll suggest that you should use 2 separate routines, each properly typed, and avoid void. If you want, this also provides a place to assert if the referencing/casting is being done incorrectly.

static sds * entryGetSdsValueRef(const entry *entry); static stringRef * entryGetStringRefRef(const entry *entry);

Doing this will eliminate a lot of the casting, and provide additional safety with enhanced type checking. This change would invert some of the logic.

Current logic is something like:

void **value_ref = entryGetValueRef(entry); if (entryHasStringRef(entry)) { stringRef *string_ref = (stringRef *)*value_ref; ... } else { sds sds = (sds)*value_ref; ... }

This can easily be converted to something like:

if (entryHasStringRef(entry)) { stringRef *string_ref = *entryGetStringRefRef(entry); ... } else { sds sds = *entryGetSdsRef(entry); ... }

This eliminates the casting and the use of void. This also helps clarify (and typecheck) that sds IS a pointer, while stringRef IS a struct.

JimB123 · 2025-10-30T18:52:50Z

src/t_hash.c

+        return sdsfromlonglong(vll);
+    }
+    if (hi->encoding == OBJ_ENCODING_HASHTABLE) {
+        size_t vlen = 0;


Avoid unnecessary initializations, it can fake out static analysis.

JimB123 · 2025-10-30T18:58:48Z

src/t_hash.c

+            size_t len;
+            sds field = entryGetField(hi.next);
+            char *value_str = entryGetValue(hi.next, &len);
            long long expiry = entryGetExpiry(hi.next);
            /* Add a field-value pair to a new hash object. */
-            entry *entry = entryCreate(field, sdsdup(value), expiry);
+            sds value = sdsnewlen(value_str, len);
+            entry *entry = entryCreate(field, value, expiry);


Shouldn't duplicating an entry be the responsibility of entry.c? Is there a reason that this is done here in hash.c?

The fact that the entry lacks some explicit API for duplicate does not mean it needs to be implemented only for a single case. The code here is not doing anything intrusive to the entry, just using the offered API.
We can introduce a duplicate API but IMO it is negligible

JimB123 · 2025-10-30T19:02:22Z

src/t_hash.c

    } else if (hi->encoding == OBJ_ENCODING_HASHTABLE) {
-        sds value = hashTypeCurrentFromHashTable(hi, what);
-        addWritePreparedReplyBulkCBuffer(wpc, value, sdslen(value));
+        size_t len = 0;


Avoid meaningless initializations, it can fake out static analysis.

JimB123 · 2025-10-30T19:38:20Z

src/db.c

        free_callback = sdsfree;
    }
-    vectorInit(&result, SCAN_VECTOR_INITIAL_ALLOC, sizeof(sds));
+    vectorInit(&result, SCAN_VECTOR_INITIAL_ALLOC, sizeof(stringRef));


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.

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.

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.

The alternative is to create here a local data-type which is similar to stringRef.

JimB123 · 2025-10-30T19:47:03Z

src/server.h

+/* Structure representing a non-owning view of a buffer.
+ * A stringRef struct does not manage the underlying memory, so its destruction
+ * will not free the buffer. */
+typedef struct stringRef {
+    const char *buf; /* Pointer to the externalized buffer */
+    size_t len;      /* Length of the buffer */
+} stringRef;
+


Please move this type into entry.c. The only other usage is completely unrelated in db.c and suggested to be an independent type. Note this this does not need to be exposed in entry.h, it can be completely internal.

we kept it here for other occasions we might want to use it (eg scan).
I think I am fine with repeat internal re-implemet this, but why is it so bad to have this type?

madolson · 2025-11-10T16:43:35Z

@ranshid @JimB123 @yairgott What of the comments that are blocking this PR from getting merged? We have plenty of time before 9.1, but I just don't want to end up in a situation where we don't merge it again.

ranshid

Let me try and sum-up my thoughts about the status:

Documentation improvements

https://github.com/valkey-io/valkey/pull/2472/files#r2473588727
https://github.com/valkey-io/valkey/pull/2472/files#r2471218383
https://github.com/valkey-io/valkey/pull/2472/files#r2473736680
https://github.com/valkey-io/valkey/pull/2472/files#r2470424309
https://github.com/valkey-io/valkey/pull/2472/files#r2470568137
https://github.com/valkey-io/valkey/pull/2472/files#r2470572782
https://github.com/valkey-io/valkey/pull/2472/files#r2470572981

code refactor

create a dedicated scan type: https://github.com/valkey-io/valkey/pull/2472/files#r2479301622

add a dedicated stringRef getter: https://github.com/valkey-io/valkey/pull/2472/files#r2471108477

refactor entryGetValue: https://github.com/valkey-io/valkey/pull/2472/files#r2471145583

entryUpdate refactor: https://github.com/valkey-io/valkey/pull/2472/files#r2474627473 + https://github.com/valkey-io/valkey/pull/2472/files#r2478426332
https://github.com/valkey-io/valkey/pull/2472/files#r2474655831

entryUpdateAsStringRef: https://github.com/valkey-io/valkey/pull/2472/files#r2474037834 + https://github.com/valkey-io/valkey/pull/2472/files#r2478563045 + https://github.com/valkey-io/valkey/pull/2472/files#r2473574601 + https://github.com/valkey-io/valkey/pull/2472/files#r2479164080+ https://github.com/valkey-io/valkey/pull/2472/files#r2479155661

refactor entryConstruct: https://github.com/valkey-io/valkey/pull/2472/files#r2475553245

trivial code refactor

https://github.com/valkey-io/valkey/pull/2472/files#r2471200320
https://github.com/valkey-io/valkey/pull/2472/files#r2471192263
https://github.com/valkey-io/valkey/pull/2472/files#r2479118890
https://github.com/valkey-io/valkey/pull/2472/files#r2479166951
https://github.com/valkey-io/valkey/pull/2472/files#r2479190818
https://github.com/valkey-io/valkey/pull/2472/files#r2479214355

correctness

https://github.com/valkey-io/valkey/pull/2472/files#r2471130746
https://github.com/valkey-io/valkey/pull/2472/files#r2478894678
https://github.com/valkey-io/valkey/pull/2472/files#r2478794808
https://github.com/valkey-io/valkey/pull/2472/files#r2478831245
https://github.com/valkey-io/valkey/pull/2472/files#r2478899287

open issues

https://github.com/valkey-io/valkey/pull/2472/files#r2473620702
https://github.com/valkey-io/valkey/pull/2472/files#r2475548331 - should be handled when we rebase unstable to solve conflict
https://github.com/valkey-io/valkey/pull/2472/files#r2479323821

IMO the open issues + correctness + trivial code refactor can be completed in order to merge it.
We can consider followup in a different PR about the documentation and major code refactor
@JimB123 @yairgott WDYT?

ranshid · 2025-11-10T17:12:36Z

src/entry.c

-
 /* Returns the address of the entry allocation. */
-void *entryGetAllocPtr(const entry *entry) {
+static void *entryGetAllocPtr(const entry *entry) {


@JimB123 I am not sure exactly how much it would simplify existing cases. I might agree that having another function which is fetching the expiration ref would probably improve maintainability and clarity.

ranshid · 2025-11-10T17:15:17Z

src/entry.c

+    debugServerAssert((((uintptr_t)buf & 0x7) == 0)); /* Test that the allocation is indeed 8 bytes aligned
+                                                       * This is needed since we access the expiry as with pointer casting
+                                                       * which require the access to be 8 bytes aligned. */


@JimB123 IIRC this followed your review comment when we introduced HFE pr. But leave aside the past. I agree this can be dropped.

ranshid · 2025-11-10T17:45:56Z

src/entry.c

+    size_t field_size = sdsReqSize(sdslen(field), SDS_TYPE_8);
+    size_t alloc_size = field_size + sizeof(void *);
+    alloc_size += (expiry == EXPIRY_NONE) ? 0 : sizeof(expiry);
+
+    size_t expiry_size = 0;
+    if (expiry != EXPIRY_NONE) expiry_size = sizeof(expiry);
+    sds new_entry = entryConstruct(alloc_size, field, value, expiry, false, SDS_TYPE_8, expiry_size, sizeof(value), field_size);


I think this already changed with #2794 so we might just fix this over the rebase.

ranshid · 2025-11-10T17:51:48Z

src/entry.c

-        if (entryHasExpiry(entry)) mem += sizeof(long long);
    }
-    mem += sdsAllocSize(entryGetValue(entry));
+    mem += sdsAllocSize((sds)entryGetValue(entry, NULL));


yes. we should probably separate into 2 cases:
when we have stringRef (ie take the len) and when we have sds value ref, we should use the sdsAllocSize

ranshid · 2025-11-10T17:54:46Z

src/server.h

+/* Structure representing a non-owning view of a buffer.
+ * A stringRef struct does not manage the underlying memory, so its destruction
+ * will not free the buffer. */
+typedef struct stringRef {
+    const char *buf; /* Pointer to the externalized buffer */
+    size_t len;      /* Length of the buffer */
+} stringRef;
+


we kept it here for other occasions we might want to use it (eg scan).
I think I am fine with repeat internal re-implemet this, but why is it so bad to have this type?

ranshid · 2025-11-10T17:57:45Z

src/t_hash.c

+    entry *entry = *entry_ref;
+    long long expiry = entryGetExpiry(entry);
+    void *new_entry = entryUpdateAsStringRef(entry, buf, len, expiry);
+    serverAssert(hashtableReplaceReallocatedEntry(ht, entry, new_entry));


I think it might be true, but we already fail apply your suggestion through many places in the code.
However since this is a small refactor, lets do it.

ranshid · 2025-11-10T18:02:41Z

src/t_hash.c

+            size_t len;
+            sds field = entryGetField(hi.next);
+            char *value_str = entryGetValue(hi.next, &len);
            long long expiry = entryGetExpiry(hi.next);
            /* Add a field-value pair to a new hash object. */
-            entry *entry = entryCreate(field, sdsdup(value), expiry);
+            sds value = sdsnewlen(value_str, len);
+            entry *entry = entryCreate(field, value, expiry);


The fact that the entry lacks some explicit API for duplicate does not mean it needs to be implemented only for a single case. The code here is not doing anything intrusive to the entry, just using the offered API.
We can introduce a duplicate API but IMO it is negligible

yairgott added 2 commits August 12, 2025 03:03

fixing test issue

9ee29c2

Signed-off-by: yairgott <[email protected]>

fixing test issue

a9fc017

Signed-off-by: yairgott <[email protected]>

yairgott mentioned this pull request Aug 12, 2025

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

Closed

yairgott added 3 commits August 12, 2025 14:57

addressing unittest issues

7dae1e6

Signed-off-by: yairgott <[email protected]>

addressing unittest issues

5d4c16a

Signed-off-by: yairgott <[email protected]>

addressing lint

f826960

Signed-off-by: yairgott <[email protected]>

yairgott marked this pull request as ready for review August 12, 2025 15:21

madolson requested a review from ranshid August 12, 2025 18:05

madolson added this to Valkey 9.0 Aug 12, 2025

madolson moved this to In Progress in Valkey 9.0 Aug 12, 2025

ranshid reviewed Aug 13, 2025

View reviewed changes

src/entry.c Show resolved Hide resolved

src/entry.c Outdated Show resolved Hide resolved

src/entry.c Outdated Show resolved Hide resolved

src/entry.c Show resolved Hide resolved

src/entry.c Outdated Show resolved Hide resolved

addressing comments

78b90bb

Signed-off-by: yairgott <[email protected]>

yairgott added 3 commits August 19, 2025 03:03

renaming, bug fixing and adding unittests

6f7f990

Signed-off-by: yairgott <[email protected]>

unittest fix

848fd6d

Signed-off-by: yairgott <[email protected]>

cosmetic change

a7a1537

Signed-off-by: yairgott <[email protected]>

yairgott added 3 commits August 19, 2025 13:31

fixing module tests

06b6cfc

Signed-off-by: yairgott <[email protected]>

Merge remote-tracking branch 'upstream/unstable' into hash_shared

c8a7aac

fixing comments

356891d

Signed-off-by: yairgott <[email protected]>

ranshid reviewed Aug 20, 2025

View reviewed changes

zuiderkwast added the major-decision-pending Major decision pending by TSC team label Aug 25, 2025

madolson removed this from Valkey 9.0 Sep 1, 2025

madolson added this to Valkey 9.1 Sep 1, 2025

madolson moved this to Todo in Valkey 9.1 Sep 1, 2025

Yair Gottdenker added 3 commits September 5, 2025 04:41

addressing code review comments

6913e21

Signed-off-by: Yair Gottdenker <[email protected]>

addressing lint

82ef7bd

Signed-off-by: Yair Gottdenker <[email protected]>

addressing unit test failure

48193e8

Signed-off-by: Yair Gottdenker <[email protected]>

ranshid reviewed Sep 9, 2025

View reviewed changes

yairgott and others added 7 commits September 9, 2025 14:55

Update src/t_hash.c

38aae4a

Co-authored-by: Ran Shidlansik <[email protected]> Signed-off-by: Yair Gottdenker <[email protected]>

Update src/t_hash.c

5f65be7

Co-authored-by: Ran Shidlansik <[email protected]> Signed-off-by: Yair Gottdenker <[email protected]>

Update src/server.h

8224ea6

Co-authored-by: Ran Shidlansik <[email protected]> Signed-off-by: Yair Gottdenker <[email protected]>

Update src/entry.c

ed86fa0

Co-authored-by: Ran Shidlansik <[email protected]> Signed-off-by: Yair Gottdenker <[email protected]>

addressing comments

6d5a45e

Signed-off-by: Yair Gottdenker <[email protected]>

addressing code review comments

2aea44d

Signed-off-by: Yair Gottdenker <[email protected]>

fixing module test

37a006c

Signed-off-by: Yair Gottdenker <[email protected]>

ranshid reviewed Sep 14, 2025

View reviewed changes

src/t_hash.c Show resolved Hide resolved

src/t_hash.c Outdated Show resolved Hide resolved

src/t_hash.c Outdated Show resolved Hide resolved

Yair Gottdenker added 2 commits September 16, 2025 19:28

addressing code review comments

b97667a

Signed-off-by: Yair Gottdenker <[email protected]>

addressing code review comments

bf6d7c7

Signed-off-by: Yair Gottdenker <[email protected]>

madolson requested a review from JimB123 October 27, 2025 14:10

madolson added major-decision-approved Major decision approved by TSC team and removed major-decision-pending Major decision pending by TSC team labels Oct 27, 2025

JimB123 reviewed Oct 30, 2025

View reviewed changes

allenss-amazon mentioned this pull request Nov 10, 2025

[NEW] Combined list of Issues for Search Module #2559

Open

ranshid reviewed Nov 10, 2025

View reviewed changes

		/* Sets a reference to the value.
		* The function takes the hash key, hash field, and a buffer along with its length. */

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

Are you sure you want to change the base?

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

Uh oh!

Conversation

yairgott commented Aug 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview

Vectors

Memory Allocation Strategy

Old Implementation

New Implementation - Key Points

valkey-search Usage

Insertion

Deletion

Update

Uh oh!

ranshid left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yairgott commented Aug 15, 2025

Uh oh!

codecov bot commented Aug 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ranshid commented Aug 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yairgott commented Aug 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ranshid commented Aug 18, 2025

Uh oh!

yairgott commented Aug 18, 2025

Uh oh!

ranshid commented Aug 18, 2025

Uh oh!

ranshid commented Aug 18, 2025

Uh oh!

yairgott commented Aug 19, 2025

Uh oh!

ranshid left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zuiderkwast commented Aug 25, 2025

Uh oh!

madolson commented Sep 1, 2025

Uh oh!

yairgott commented Sep 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yairgott commented Aug 12, 2025 •

edited

Loading

codecov bot commented Aug 15, 2025 •

edited

Loading

ranshid commented Aug 17, 2025 •

edited

Loading

yairgott commented Aug 18, 2025 •

edited

Loading

yairgott commented Sep 3, 2025 •

edited

Loading

ranshid commented Oct 8, 2025 •

edited

Loading

yairgott Nov 10, 2025 •

edited

Loading