Provide a generic_flat_response

[mzimbres]

For latency sensitive users such as this one it might be useful to use a flat data structure to store reponses where buffers can be reused and thereby removing the need for dynamic memory allocations.

A generic_flag_response would keep all its data into std::string buffer and refer to it with string_views, for exampl

struct generic_flat_response {
    std::string buffer;
    adapter::result<std::vector<resp3::basic_node<std::string_view>> response;
};

[D0zee]

Hi Marcelo, I would like to try solving this issue. Can you please point out what should I learn/read that can help to implement generic_flat_response? My initial plan is to look at use cases of generic_response across all repo and understand how to add a new response type in an elegant way. I would appreciate any sources if you provide them.

[mzimbres]

Hi @D0zee thanks, you are welcome. I will try to come up with some explanation this week. The flat response type should have the same interface as the generic_reponse. It will also likely use the same adapter of the generic_response.

[mzimbres]

Hi, here are some details. The code is already generic enough so that if I define the generic_flag_response like

using generic_response = adapter::result<std::vector<resp3::node>>;
using generic_flat_response = adapter::result<std::vector<resp3::node_view>>;

and use it in the cpp20_subscriber.cpp example replacing the generic_response (and commenting the consume_one function) the code will compile. But of course that won't work because the data the string_views points to will have been erase by the time it is used, therefore, what we have to do is to defined the flat response as

using generic_flat_response = adapter::result<flat_response_imp>;

where

class flat_response_impl {
public:
   // This API has to be implemented: at(), reserve, begin, end, operator[] etc.

   // And to push data into the container push_back().

private:
   std::string data_;
   std::vector<resp3::node_view> view_;
};

Once we have that you can write an specialization almost identical to what I do for general_aggregate but that copies the data to the data_ member, which should happen before pushing data into the container here.

Also, care should taken when pushing data to the string causes it to reallocate and therefore invalidate string_view's. Perhaps it is going to be necessary to work with offsets into the buffer. I am not sure if node_view will be suitable for that or a new node type will have to be introduced.

[anarthal]

In case it helps somehow, Boost.MySQL does something somehow similar here: https://github.com/boostorg/mysql/blob/develop/include/boost/mysql/detail/row_impl.hpp

Strings are stored as offsets while parsing, then converted to string_views after no more reallocations may happen.

[D0zee]

thank you for explanations, will dig into them

[D0zee]

May be a silly question, but how properly to build the repo? Tried to use tools/ci.py but failed. Can you please help me in that?

[mzimbres]

Hi @D0zee, you can clone boost

$ git clone --recurse-submodules https://github.com/boostorg/boost

# I am not sure if the step above will checkout the develop branch so you might have to run
$ git submodule foreach --recursive git checkout develop

then create a directory to build it somewhere and run

$ cmake <path>/boost/libs/redis -DCMAKE_BUILD_TYPE=Debug
$ make

[anarthal]

You will probably need to add a BUILD_TESTING definition to your CMake invocation for it to build tests:

cmake <path>/boost/libs/redis -DCMAKE_BUILD_TYPE=Debug -DBUILD_TESTING=ON

To run the tests, you need a properly configured Redis server. Under Linux, you can use Docker Compose with the file in tools/docker-compose.yml:

IMAGE=ubuntu:22.04 docker compose -f <path>/boost/libs/redis/tools/docker-compose.yml up -d

This will spawn a listening server with adequate TLS configuration and a listening UNIX socket for the UNIX socket tests. You can also have a look at this file and run the redis-server command manually, emulating what this file does.

[D0zee]

Thank you for comments! I was able to set up build environment

[D0zee]

Hi @mzimbres, I'm thinking about ergonomic API access and wanted to discuss it with you. Right now I really think about implementation of new type of node - offset_node derived from node_view. This node will expose two additional fields: size and offset for reconstruction of std::string_view from data_. I have several questions:

• Should user be able to change the internal state of flat_response_impl? In my opinion the state of this structure should be frozen after parser completed its operation.

• Should we provide the opportunity to preallocate the size of response to the user? I assume that yes, but in this case there is a question how to determine the average / median node value size? Also, there is an option to give this responsibility to determine the desired size of each string to the user.

• Do we need to tweak/generalize/write a specialization for consume_one function?

[D0zee]

By the way you can have a look at draft implementation of this issue #278. Please do not judge strictly!

[mzimbres]

Thanks, I will have a look at the PR tomorrow.

Right now I really think about implementation of new type of node - offset_node derived from node_view. This node will expose two additional fields: size and offset for reconstruction of std::string_view from data_.

I am not sure we really need a new node type. For example, we could introduce an offset_string_view

struct offset_string {
   std::size_t offset;
   std::size_t size;

   std::string_view to_string_view(char const* base_ptr) const
   {
      return std::string_view{base_ptr + offset, size};
   }
};

Then the node definitions would be

using node = basic_node<std::string>;
using node_view = basic_node<std::string_view>;
using offset_node = basic_node<offset_string>;

I would like to avoid working with temporaries like here

resp3::node_view at(std::size_t) { ... }

and instead we would have

offset_node const& at(std::size_t) const { ... }

That is however not very ergonomic to the user as to get a string_view he would have to call

auto data = resp.at(i).to_string_view(resp.get_base_ptr());

We could also store the base pointer in the offset_string

struct offset_string {
   char const* base_ptr;
   std::size_t offset;
   std::size_t size;

   operator std::string_view() const
   {
      return std::string_view{base_ptr + offset, size};
   }
};

and set the base_ptr once done with the response, for example in the notify_done callback. To do that we would have to add a new adapter method that would be called here, so instead of

      info->set_done_callback([notifier]() {
         notifier->try_send(std::error_code{}, 0);
      });

we would have

      info->set_done_callback([notifier, adapter]() {
         adapter.prepare_done(); // New adapter method.
         notifier->try_send(std::error_code{}, 0);
      });

However, at the moment, the adapter is type erased here before we can pass it to the code snippet above

   template <class T>
   static auto create_impl(T& resp) -> impl_t
   {
      using namespace boost::redis::adapter;
      auto adapter = boost_redis_adapt(resp);
      std::size_t size = adapter.get_supported_response_size();
      return {std::move(adapter), size};
   }

Some kind of hopefully small refactoring would be required.

I have several questions:

Should user be able to change the internal state of
flat_response_impl? In my opinion the state of this structure should
be frozen after parser completed its operation.

I don't have use case to change the state of a response. As a first prototype we can indeed make the interface const.

Should we provide the opportunity to preallocate the size of
response to the user? I assume that yes, but in this case there is a
question how to determine the average / median node value size?
Also, there is an option to give this responsibility to determine
the desired size of each string to the user.

Not sure what you mean, only the user knows the expected size of a response, we can't guess it to preallocate memory.

[D0zee]

Thank you for the comment!

I am not sure we really need a new node type. For example, we could introduce an offset_string_view

However in this case the user lacks other meta information like depth/type/aggregate_size. That was a reason why I don't just only string-like structure. I thought this information is important for the end user. However if it is not needed in your opinion the implementation will be much easier.

I would like to avoid working with temporaries like here

Could you please elaborate on the reason? It's going to be light-weight operation or the performance is not only the reason? In my implementation it is going to be consistent, he will be able to work with the new generic_flat_response in the same way as he does with generic_response.

Not sure what you mean, only the user knows the expected size of a response, we can't guess it to preallocate memory.

You are right and I understood it later after I left the comment. If the user assumes about the size of the response and length of each response string he will pass parameters relying on this assumption and instead of X memory allocations during parsing operation (when std::string buffer grows) he would have one memory allocation. It is just opportunity to have less allocations if the user really cares about it.

[mzimbres]

However in this case the user lacks other meta information like depth/type/aggregate_size. That was a reason why I don't just only string-like structure. I thought this information is important for the end user. However if it is not needed in your opinion the implementation will be much easier.

In your PR you defined the offset_node like this

struct offset_node : resp3::node_view {
   std::size_t offset{};
   std::size_t size{};
};

I was wondering however if it is more sound to define an offset_string and use that as basic_node template parameter

struct offset_string {
   std::string_view data;
   std::size_t offset;
   std::size_t size;
};

using offset_node = basic_node<offset_string>;

I think this is conceptually more robust because the offset refers only to the string and not to node itself.

Could you please elaborate on the reason? It's going to be light-weight operation or the performance is not only the reason?

Let us take this usecase where the user is reading 109,000 hashes. If we traverse such a response with the function

resp3::node_view at(std::size_t index) { return make_node_view(view_.at(index)); }

we will endup coping 109,000 * sizeof(offset_node) bytes of data or ~6Mb according to my calculation, this is probably negligible but worth avoiding if possible. Low latency was one of the main design principles behind Boost.Redis.

Sidenote: In the future I would like to have the option to read large responses from the socket directly into the flat response buffer, reducing data copying to the very minimum.

In my implementation it is going to be consistent, he will be able to work with the new generic_flat_response in the same way as he does with generic_response.

Sure, thats is great. Above I was throing my ideas on the table so we can discuss but I am not sure all of that is reasonable. Tomorrow I will have more time and will have a closer look in your PR.

[D0zee]

I've taken into consideration all comments and suggestions, now the implementation is much cleaner. Please have a look when you find time. Thank you!