The current RESP parsing logic is very fragile as it assumes a fixed response structure for GET operations. It doesn't handle Redis error responses (e.g., -ERR ...) or nil responses for non-existent keys ($-1\r\n). If Redis returns an unexpected response, the client will likely desynchronize from the TCP stream, leading to cascading failures for all subsequent operations on that connection.

To make this more robust, the parsing logic should inspect the first byte of the response to determine its type (e.g., + for simple string, $ for bulk string, - for error, : for integer) and parse the rest of the response accordingly. This is crucial for handling cases like a key not being found during a GET operation.

A similar issue exists in do_set_from and do_exists.

I think this suggestion is reasonable. The no parsing optimization might be a little aggressive. Parsing only the first byte avoid O(n) equality check.

In RESPClient, the submit_* methods call the native client and then register a future using the returned future_id. There is a race condition where the native worker thread could complete the operation and signal the event_fd before the future is registered in self._pending. If this happens, _on_ready will ignore the completion because the future_id is not yet in self._pending, and the future will never be resolved, leading to a hang.

Recommendation: The future_id should be known or reserved before the task is submitted to the native worker, allowing the future to be registered before the worker can possibly complete the task.

The make_key_header function uses std::ostringstream to construct the key header string for each request. While convenient, this can introduce performance overhead due to repeated dynamic memory allocations in a performance-critical path.

For a high-throughput client, consider optimizing this by using a pre-allocated buffer (e.g., thread-local) with snprintf, or a more efficient string building mechanism to avoid allocations on each call.

The make_key_header function uses std::ostringstream to construct the key header string for each request. While convenient, this can introduce performance overhead due to repeated dynamic memory allocations in a performance-critical path.

For a high-throughput client, consider optimizing this by using a pre-allocated buffer (e.g., thread-local) with snprintf, or a more efficient string building mechanism to avoid allocations on each call.

I agree with Gemini's opinion here. (But only when we can have an upper-bound on the key's size)

The RESPClient class in this example file is a duplicate of the one defined in lmcache/v1/storage_backend/connector/redis_connector.py. This code duplication makes maintenance harder. The example should import the RESPClient from the library itself rather than redefining it.

done

follow up: whether to have "pin"/"lock" semantics

doesn't seem to be in RESP right now

double check whether std::move and auto move constructor can work on all the fields inside Request struct (especially py::object)

Why do we have this in this PR?

needed to check the throughput. this has been broken for a while now

I can separate a new PR for this but this PR needs it as well to test so it will create redundant changes

Can we rename it as "full_chunk_size_bytes"? It's clearer because full_chunk_bytes sounds like a data chunk of bytes.

Btw, this name change can be separated to another PR to keep this PR clean.

Is it possible to avoid having py-related stuff in resp.h and resp.cpp? Let's try to put all the python-related types and operations into pybind.cpp.

FYI, py::list can be directly (and automatically) converted to std::vector if we include pybind11/stl.h (https://pybind11.readthedocs.io/en/stable/advanced/cast/stl.html)

We can also manually convert py::memoryview to std::vector as well (or maybe there are better practices).

By doing this, we can also have more freedom to release GIL. See: https://pybind11.readthedocs.io/en/stable/advanced/misc.html#global-interpreter-lock-gil

The Python C API dictates that the Global Interpreter Lock (GIL) must always be held by the current thread to safely access Python objects. As a result, when Python calls into C++ via pybind11 the GIL must be held, and pybind11 will never implicitly release the GIL.

Yes GIL only needed for py::object and py::handle so this will clarify where GIL can be released (do scoped release ASAP in every cpp function)

If we removed all the py-related stuff from resp.h and resp.cpp, we could have a few very simple lambda expressions to do the type conversion here.

done

We can force all the operations to be "batched" and remove these three functions. It can simplify the cpp implementation.

done

Generally, the code here can be put into the constructor.

Also, the get_prefix, set_prefix, and exists_prefix can be constexpr std::string_view IIUC?

done

The make_key_header function uses std::ostringstream to construct the key header string for each request. While convenient, this can introduce performance overhead due to repeated dynamic memory allocations in a performance-critical path.

For a high-throughput client, consider optimizing this by using a pre-allocated buffer (e.g., thread-local) with snprintf, or a more efficient string building mechanism to avoid allocations on each call.

I agree with Gemini's opinion here. (But only when we can have an upper-bound on the key's size)

What's the reason to have std::string host and host_(std::move(host)) instead of using const std::string &host) with host_(host)?

Since host will always be stored in the object (it's a "sink"), this pattern lets the compiler choose the optimal path based on what the caller provides, without needing overloads.
https://stackoverflow.com/questions/21035417/is-the-pass-by-value-and-then-move-construct-a-bad-idiom

I'm not an expert here, but I do want to get rid of buf_owners (and buf_owner) here.

From pybind11's doc, I do see we can do inc_ref and dec_ref on the python objects, including py::memoryview (see https://pybind11.readthedocs.io/en/stable/reference.html#without-reference-counting)

Not sure what the best practice is, but let's investigate and learn.

One thing we need to be careful about is that we need to do dec_ref when the failure happens. Since the tasks are executed asynchronously, I'm not sure what's the best place to do the dec_ref.

IMO, the best place is the destructor.

I think we can get rid of buf_owners since we can make the assumption in LMCache use case that the memory objects will not be garbage collected

we can probably do requests_.emplace(req) here (see https://en.cppreference.com/w/cpp/container/queue/emplace.html)

the req struct already exists (and is populated dynamically) so this does nothing? unless the cpp compiler is a super genius and can somehow figure this out.

@ApostaC From my understanding,
vector is [b0,b1,b2 ...,b7] in 1 byte.
and vector can save like [b0],[b1],[b2].
So, we can not guarantee the atomic byte access for 8 continuous bool value.

Is this what you mean? @sammshen

IMO, the best place is the destructor.

Is this for preventing the python memory gc?

this is to find the python future to which a completion belongs to from the cpp side.

cpp wakes up python via eventfd and outputs (future_id, ok, result_bool, error, result_bools) and python side needs to populate the future indexed by the future_id

Okay then it does prevent GC of python future object not memoryview.

Is there any possibility that completion drained before _pending insert?
If then we need another types of structure that Tuple[pending_future structure, str, Tuple[memoryview, ...]] types of thing for safe resolving. but It seems this case didnt happened in my test case.
So, just left as a concern in here.

this will not happen because the callback is not preemptive at the python level and the submission is non-blocking!

Okay then it does prevent GC of python future object not memoryview.

Is there any possibility that completion drained before _pending insert?
If then we need another types of structure that Tuple[pending_future structure, str, Tuple[memoryview, ...]] types of thing for safe resolving. but It seems this case didnt happened in my test case.
So, just left as a concern in here.