Concludes the work on the generic_flat_response started by Nikolai Vladimirov

[mzimbres]

@anarthal, let us start the review for generic_flat_response. I need the async_receive2 so I committed my own implementation and will rebase on top of yours if you find time to implement it.

[anarthal]

Do we have any numbers comparing the performance of this approach (converting to string_view immediately after parsing) vs. the custom iterator approach (converting to string_view on iteration)?

I will have a look at this tomorrow

[mzimbres]

Do we have any numbers comparing the performance of this approach (converting to string_view immediately after parsing) vs. the custom iterator approach (converting to string_view on iteration)?

IMO this is not worth the effort. Both cases must contruct construct the string_view but the custom iterator approach must copy more data i.e. node& vs node so it can only be slower.

Performance however is not my main concern because it is probably in sub microsend difference, which is hard to measure. In essence I regard the T at(std::size_t); api to be inferior to T const& at(std::size_t) since the latter requires reconstructing the node on each call (imagine needing to call it into a loop). Changing the container is probably never going to be needed, but I at least the current API does not make it impossible.

I will have a look at this tomorrow

Thanks. Please, take into account I hurried a bit to ovoid delaying discussion another week, specially on what we should do with async_receive2.

[anarthal]

The only thing I'm not convinced of is exposing this new offset_string type. It's more API surface, and it's not "A std::string-like type.", which is what basic_node<String> requires from String.

I'm writing some benchmarks and thinking to see if there is any chance of using basic_node<std::string_view> with the current approach.

Testing and docs could use a couple of improvements, but I don't think there's a need to delay merging this because of these two - I can make some improvements later.

[anarthal]

Concluded the benchmarks. TL;DR: let's use the separate vector<offset_and_size> for now.

Benchmark results (explanation follows):

	clang-20	clang-20 libc++	gcc-13
BM_push_no_reuse<original_impl>	8331 ns	9274 ns	10633 ns
BM_push_no_reuse<custom_buffer>	5451 ns	7011 ns	5066 ns
BM_push_no_reuse<offset_vector>	8781 ns	10816 ns	8551 ns
BM_push_reuse<original_impl>	4999 ns	6533 ns	7731 ns
BM_push_reuse<custom_buffer>	2987 ns	5453 ns	2767 ns
BM_push_reuse<offset_vector>	5040 ns	7461 ns	4928 ns
BM_big_no_reuse<original_impl>	2668 ns	2902 ns	3126 ns
BM_big_no_reuse<custom_buffer>	1927 ns	2342 ns	1893 ns
BM_big_no_reuse<offset_vector>	2835 ns	3332 ns	2761 ns
BM_big_reuse<original_impl>	1308 ns	1837 ns	2039 ns
BM_big_reuse<custom_buffer>	838 ns	1417 ns	748 ns
BM_big_reuse<offset_vector>	1335 ns	1900 ns	1425 ns

Full code: https://gist.github.com/anarthal/73a8e081dc9de3f390e6d0ec1d5e2b34

Implementations:

• generic_flat_response_value (matching original_impl in the table above): what is in the PR.
• generic_flat_response_value_v2: never used, it's a proof of concept but may be UB.
• generic_flat_response_value_v3 (matching custom_buffer in the table above): we store only string views. Every time the buffer reallocates, we use the old data pointer to compute the offset and update the string view. Doesn't use any adapter hooks.
• generic_flat_response_value_v4 (matching offset_vector in the table above): we use regular node_view nodes, and a separate vector for sizes/offsets, as you proposed.

The benchmarks:

• Push: simulates receiving 100 pushes.
• Big: simulates receiving a response with 100 nodes.
• Reuse: the same response object is reused over and over.
• No reuse: a new response object is created every time.

The custom buffer approach seems consistently faster in all benchmarks. It's however much more implementation work, so I'd discourage from taking this way at least for now. The other two options seem similar. offset_vector would allow us to get rid of offset_string, so I suggest taking this path.

[anarthal]

We shouldn't need this or -Wno-deprecated-declarations. We already define BOOST_ALLOW_DEPRECATED in the tests, and examples shouldn't use deprecated features.

[anarthal]

Unused include

[anarthal]

Better use explicit return type here

[anarthal]

You probably need -Wno-deprecated-declarations and friends because impl_.async_receive and impl_.receive are deprecated. This means that the user is going to see these warnings when they build src.hpp, and we don't want this.

The way I usually solve this is by moving the function from basic_connection to connection_impl, then calling this second function from both basic_connection and connection.

[anarthal]

These should be made private with a detail accessor. But this is not urgent, I can do it afterwards.

[anarthal]

We should retain at least one test for the deprecated async_receive and receive so we make sure we don't break them by accident. I suggest duplicating this test to get a minimum of coverage.

[anarthal]

There seems to be a MrDocs bug with copydoc and this seems to fail. It doesn't support Doxygen's overload resolution either (cppalliance/mrdocs#842). Since this is deprecated, I suggest keeping the old docs stating that the function is deprecated and go.

[anarthal]

New classes should be added to the reference page. If you'd like, please open an issue and I'll add it after you merge it.

[anarthal]

I completely forgot - but this class copy semantics are wrong. You will get string_views pointing to the old class. You will need to implement a custom copy constructor and copy assignment. You might also mark these operations as deleted for now and implement them later on.