Adding support for sharing memory between the module and the engine

[yairgott]

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

1. 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.
2. ViewValue is a new data type that captures the externalized buffer and its length.

valkey-search Usage

Insertion

1. 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.
2. 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.

[yairgott]

• 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[bot]]

Codecov Report

:x: Patch coverage is 66.07143% with 76 lines in your changes missing coverage. Please review. :white_check_mark: Project coverage is 72.40%. Comparing base (51f871a) to head (509a414). :warning: Report is 3 commits behind head on unstable.

Files with missing lines	Patch %	Lines
src/entry.c	64.56%	45 Missing :warning:
src/t_hash.c	60.37%	21 Missing :warning:
src/module.c	18.18%	9 Missing :warning:
src/ziplist.c	0.00%	1 Missing :warning:

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable    #2472      +/-   ##
============================================
- Coverage     72.46%   72.40%   -0.07%     
============================================
  Files           129      129              
  Lines         70530    70629      +99     
============================================
+ Hits          51108    51136      +28     
- Misses        19422    19493      +71     

Files with missing lines	Coverage Δ
src/aof.c	81.10% <100.00%> (+0.02%)	:arrow_up:
src/db.c	93.07% <100.00%> (-0.06%)	:arrow_down:
src/rdb.c	76.75% <100.00%> (-0.22%)	:arrow_down:
src/server.h	100.00% <ø> (ø)
src/ziplist.c	15.16% <0.00%> (ø)
src/module.c	9.79% <18.18%> (+0.02%)	:arrow_up:
src/t_hash.c	94.56% <60.37%> (-2.04%)	:arrow_down:
src/entry.c	80.08% <64.56%> (-15.29%)	:arrow_down:

... and 14 files with indirect coverage changes

:rocket: New features to boost your workflow:

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

[ranshid]

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:

1. 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.
2. 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:

1. It will be able to accept both sds type values AND string-references.
2. When a string reference is provided the entry will NOT take ownership over it.
3. 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.
4. 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)
5. 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.
6. 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:

1. 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);

2. 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.
3. 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:

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

[yairgott]

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

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

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

6. 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]

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

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

6. 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:

1. calling entryUpdate with a value which is not a "view" will just assert when the entry has a view already
2. 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]

From reading the code I see that:

1. calling entryUpdate with a value which is not a "view" will just assert when the entry has a view already
2. 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]

From reading the code I see that:

1. calling entryUpdate with a value which is not a "view" will just assert when the entry has a view already
2. 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]

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

[yairgott]

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.

[zuiderkwast]

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]

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]

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.

[ranshid]

Overall the code changes LGTM.

NOTE: this might have future conflicts with https://github.com/valkey-io/valkey/issues/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]

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

[madolson]

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]

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

Ack. Reviewing.

[madolson]

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

[JimB123]

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 I agree that more significant code refactors (like redesign of entryConstruct) can be addressed separately from this PR. However, I think that documentation issues are critical and should be updated now. This feature introduces some pretty fundamental changes, and we need to make sure that everyone is clear regarding usage of this API and associated memory management considerations.

Of the items you listed as (non-trivial) code refactors, I think we should do this one now:

• A separate, typed, stringref getter - https://github.com/valkey-io/valkey/pull/2472/files#r2471108477 This is important for maintainability and for limiting unintended void creep. It's needed to clarify which functions are actually working on stringRefs and which are working on sds. As is, this change makes the code less maintainable as it removes the existing type checking.

[ranshid]

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 I agree that more significant code refactors (like redesign of entryConstruct) can be addressed separately from this PR. However, I think that documentation issues are critical and should be updated now. This feature introduces some pretty fundamental changes, and we need to make sure that everyone is clear regarding usage of this API and associated memory management considerations.

Of the items you listed as (non-trivial) code refactors, I think we should do this one now:

• A separate, typed, stringref getter - https://github.com/valkey-io/valkey/pull/2472/files#r2471108477 This is important for maintainability and for limiting unintended void creep. It's needed to clarify which functions are actually working on stringRefs and which are working on sds. As is, this change makes the code less maintainable as it removes the existing type checking.

I agree about the documentation. just would like @yairgott to be aligned

[ranshid]

@yairgott lets just complete the last mile:

1. address @JimB123 last comments
2. lets sync with upstream and resolve the merge conflicts.
3. lets merge the thing!

[ranshid]

@JimB123 can you PTAL and qpprove/comment?