Support chunked_vector and other json/httpd changes #2647

travisdowns · 2025-02-17T21:12:27Z

This series came out of an effort to squash large allocations in a json httpd endpoint in redpanda: the basic idea is to optionally use chunked_fifo instead of std::vector as the underlying type in the json2code generation.

Some other bugs I ran into along the the way are also fixed, and even code that doesn't use chunked_fifo should get some benefits from the addition of move support on the json response hot path.

Test cases added where I thought it made sense.

In the exception handler we accessed exception.message which doesn't exist in general, so this would throw a new exception obscuring the true error. Fix it by just using str(e).

Factor out some common code for constructing the URL and doing the GET into _do_query.

tchaikov

lgtm.

tchaikov · 2025-02-18T03:03:22Z

tests/unit/api.json

+              "description": "Whether to return the response as a stream_object",
+              "required": true,
+              "allowMultiple": false,
+              "type": "string",


i was about to suggest to use "boolean". but ironically, it turns out that the "string" type with enum contraints is indeed simpler.

Happy to use boolean if you prefer. I actually used enum since I did a quick check and dind't see any existing uses of boolean but looking at seastar-json2code.py a boolean type is indeed supported.

I think it's less surprising to have a boolean here.

tchaikov · 2025-02-18T03:06:02Z

@amnonh hi Amnon, could you please take a look as well?

nyh · 2025-02-18T14:56:44Z

include/seastar/core/chunked_fifo.hh

+    if (&rhs != this) {
+        clear();
+        std::copy_n(rhs.begin(), rhs.size(), std::back_inserter(*this));
+        shrink_to_fit();


In copy() you decided to use reserve() and here shrink_to_fit(). Is there a reason?
By the way, maybe these two functions can call each other instead of having two versions of the copying algorithm?

By the way, I'm pretty sure that bulk copy can be done more efficiently than copying the items one by one, but I don't know if it matters.

Thanks @nyh for your feedback!

In copy() you decided to use reserve() and here shrink_to_fit(). Is there a reason?

In the copy() case, we know the destination object (ret) is empty and we know the target size, so we can just reserve that size, it's the easy case.

In the assignment operation the LHS may already have allocated memory and there are sort of three primary cases as I see it:

The LHS has less capacity than the RHS (including LHS empty).

The LHS has "equal or slightly more" capacity than RHS.

The LHS has much higher capacity than RHS (including RHS empty).

The goal is not not reallocate memory if not necessary (case 2) and not leave the container with a capacity totally out of line with its contents (case 3). The chosen approach does that. reserve() does not do that: it never shrinks the container. Arguably it is possible to be more efficient in case 2, currently we destroy all objects and then copy the new ones in, but it would also be possible to move the source objects into the LHS directly without destruction (then destroy any LHS objects in the suffix).

By the way, maybe these two functions can call each other instead of having two versions of the copying algorithm?

The reason for the two versions is as above, so it seems like the common code is really only the copy_n line (if you accept the validity of the argument above, that is)?

By the way, I'm pretty sure that bulk copy can be done more efficiently than copying the items one by one, but I don't know if it matters.

Yes, and it is easiest in case 2, where the LHS is already big enough, then we can simply use std::copy with iterators and internally this does the right thing (e.g., lowered to memcpy depending on the characteristics of the involved objects). For the other cases it is not so simple: certainly it can be done but it requires working with unitialized memory, or assuming the type T has a default constructor and then default constructing the LHS to the right size and doing a copy, which is definitely cheaper for primitive types, but puts additional requirements on the type.

In general it doesn't seem like existing routines in chunked_fifo are optimized to that level, but I'm happy to go this route if you think that's what's required here.

I see. No, I don't think that super-optimizing this code is very important. By definition, making a copy of a whole vector is already non-optimal...

By definition, making a copy of a whole vector is already non-optimal...

Indeed, and here we are adding the copy assignment operator only because that's a requirement of the json elements in seastar: we auto-generate a copy constructor (which internally uses assignment) so we require all types used as json elements to have at least an assignment operator.

avikivity · 2025-02-18T17:45:35Z

include/seastar/core/chunked_fifo.hh

@@ -178,8 +179,9 @@ public:
    chunked_fifo(chunked_fifo&& x) noexcept;
    chunked_fifo(const chunked_fifo& X) = delete;
    ~chunked_fifo();
-    chunked_fifo& operator=(const chunked_fifo&) = delete;
+    chunked_fifo& operator=(const chunked_fifo&);


Would be more symmetrical to require x = y.copy(), no?

I don't mind allowing both the copy constructor and assignment operator. C++ is a copyful language, and pretending it isn't usually just makes life harder.

Would be more symmetrical to require x = y.copy(), no?

Yeah, but ... the main problem I'm trying to solve is that json2code generates a "copy constructor" which expects all the elements of the object to be copy-assignable, here's an example from api.json in this repo:

my_object(const my_object& e) { register_params(); var1 = e.var1; var2 = e.var2; enum_var = e.enum_var; }

As shown, this copy constructor does element-wise assignment (not sure why it doesn't use member init in order to use copy-ctor instead), so that's why I'm adding the assignment operator: to make the above compile with chunked_fifo as an element.

Actually copy() is not needed here at all, it is vestigial from a different approach I tried first, though we do use this copy() pattern in Redpanda, exactly as you wanted: the container class declares move-assignment, but not move-copy, so you'd use x = y.copy() if you wanted to "force" a copy-assignment.

I'm happy to whatever here:

This change as is

This change but remove any traces of copy()

Add full copy support to chunked_fifo and update the code generation to use the copy-ctor

Remove copy-assignment, but leave copy() and update the code generation to call copy() explicitly (I had a change along these lines originally)

Please advise.

I agree that containers without copy are kind of anti-C++ though I know having them as saved us many unecessary copies in Redpanda as it forces you to get move working everywhere (with copy() as an escape hatch).

There is also 5. change json2code to call a helper template function, which defaults to a regular copy, but which we can override to do something else for specific types.

But I think supporting the copy constructor is the path of least friction (though it opens the door to bad surprises).

We handle unexpected copies by having our small-scale performance tests monitor allocation count (and task count, and instruction count) per op and watching for changes.

But I think supporting the copy constructor is the path of least friction (though it opens the door to bad surprises).

Sounds good to me.

Push 44e087a adds full copy support to chunked_fifo, and removes copy(). This actually simplifies the fix to the dangling code that @nvartolomei pointed out: currently it's hard to fully support move-only types all the way down the serialization hierarchy as we have only a virtual write(ostream) const method on json elements: this is not suitable for move-only types, which need a && overload but then can't even compile the const method, so it all gets very messy (e.g., the code generator would need to track whether the current object was "tainted" anywhere by a move-only type and then stub out the const write method to throw an exception, or implement a different interface or something.

We handle unexpected copies by having our small-scale performance tests monitor allocation count (and task count, and instruction count) per op and watching for changes.

I love this idea. Is this source-available so I can peek at it?

avikivity · 2025-02-18T17:47:09Z

include/seastar/core/chunked_fifo.hh

@@ -190,6 +192,9 @@ public:
    inline void pop_front() noexcept;
    inline bool empty() const noexcept;
    inline size_t size() const noexcept;
+    // Return a new chunked_fifo which is a copy of this one, which
+    // is useful as this class does not allow copy creation or assignment.


Previous change allows assignment.

This comment is toast as copy() is removed now.

avikivity · 2025-02-18T17:51:12Z

include/seastar/json/formatter.hh

-    static future<> write(output_stream<char>& s, const Range& range) {
-        return do_with(std::move(range), [&s] (const auto& range) {
+    static future<> write(output_stream<char>& s, Range&& range) {
+        return do_with(std::forward<Range>(range), [&s] (const auto& range) {


Ah, the original code pretends to move but actually copies.

The new code moves when it's safe.

avikivity · 2025-02-18T17:58:33Z

I'm now regretting my decision not to allow random containers like chunked_vector into Seastar (or maybe, the decision to allow the json stuff in). It's an important building block for intermediate layers like this json stuff.

nvartolomei · 2025-02-19T19:37:27Z

include/seastar/json/json_elements.hh

        _elements.clear();
        for  (auto i : list) {
            push(i);
        }
        return *this;
    }
    virtual future<> write(output_stream<char>& s) const override {
-        return formatter::write(s, _elements);
+        auto t = const_cast<json_list_template<T, Container> *>(this);


I'm puzzled by this cast and the intention behind it.

Why are we moving data from a const object?

If an optimization is possible/desirable why not add a virtual future<> write(...) && overload?

Also https://en.cppreference.com/w/cpp/language/const_cast notes: "Modifying a const object through a non-const access path and referring to a volatile object through a non-volatile glvalue results in undefined behavior."

Good eye, this is definitely wrong and left over from my earlier experimentation: will fix.

This is fixed in 44e087a

This changes adds copy assignment, and construction to chunked_fifo. These methods were missing from chunked_fifo. Copy construction in particular is a possible performance footgun which is perhaps why it was not offered in the past, or perhaps it was simply that chunked_fifo is a self-described minimalist class and copy wasn't needed. This is a precursor to using chunked_fifo in the json autogenerated classes (seastar-json2code.py).

Recently the json::formatter support was improved, allowing any Range to be formatted using formatter::write(). However, this changed the old behavior which took std::vector by value and moved it into the write method to using a const Range&, which prevents moving (we did call std::move in the same place as before but given we pass a const ref this does nothing). Change to this take a forwarding reference Range&& and use std::forward to pass the range object to do_with, which enables write to both (a) work with move-only types, and (b) avoid copying where possible even for copyable types.

stream_object takes an object by value and returns a stream function which when called with an output stream, writes the captured value to the stream. This mechanism did not work for move only objects. Enhance it to do so by moving the captured object into formatter::write.

There are a variety of http function handler types, e.g., returning string or json, async or sync. Response objects may be streaming or not, and we didn't handle the streaming case for several handler permutations. To fix, we need to check if body_writer is set (the streaming case) and use that if so for the streaming case. Do this in a utility method common to all the cases where it applies.

Stream responses were broken for a few handler types (see previous fix) so add test coverage for the streaming case: in every json2code test case do a streaming and non-streaming call to the endpoint and ensure their results are identical.

Remove unused <time.h> and <sstream> headers.

tchaikov · 2025-02-20T06:48:25Z

tests/unit/rest_api_httpd.cc

        // This demonstrate enum conversion
        obj.enum_var = v;
-        return obj;
+        stream_enum is_streaming =str2stream_enum(req.query_parameters.at("stream_enum"));


might want to add spaces around =.

Eagle eyes 🦅 !

Fixed in 9f22206.

Currently json2code only supports std::vector as a list type, but this results in unavoidable large allocations for even modest response sizes, e.g., is is not uncommon for sizeof(elem_type) for these vectors to be 100 - 1000 bytes for simple to moderately complex response types, so then a mere ~1200 to ~120 objects in the vector are enough to result in allocations > 128K, a problem for seastar applications which must avoid large allocations (because of fragmentation). To avoid this problem, support chunked_fifo as a second type for lists in json2code. Use the type "chunked_array" instead of "array" to use it and the generated code will use chunked_fifo instead of vector. Also adds tests for the new use case in the json2code test.

travisdowns added 2 commits January 9, 2025 12:37

seastar-json2code: fix error handling

d668345

In the exception handler we accessed exception.message which doesn't exist in general, so this would throw a new exception obscuring the true error. Fix it by just using str(e).

json2code_test: factor out query method

051f94f

Factor out some common code for constructing the URL and doing the GET into _do_query.

travisdowns changed the title ~~Support chunked_vector and other https changes~~ Support chunked_vector and other json/httpd changes Feb 17, 2025

tchaikov approved these changes Feb 18, 2025

View reviewed changes

tchaikov reviewed Feb 18, 2025

View reviewed changes

tchaikov requested a review from amnonh February 18, 2025 03:05

nyh reviewed Feb 18, 2025

View reviewed changes

avikivity reviewed Feb 18, 2025

View reviewed changes

travisdowns mentioned this pull request Feb 19, 2025

chunked_map in seastar #2133

Open

nvartolomei reviewed Feb 19, 2025

View reviewed changes

travisdowns added 6 commits February 19, 2025 22:12

httpd: test cases for streaming

df6953f

Stream responses were broken for a few handler types (see previous fix) so add test coverage for the streaming case: in every json2code test case do a streaming and non-streaming call to the endpoint and ensure their results are identical.

json: remove unused headers

5117252

Remove unused <time.h> and <sstream> headers.

travisdowns force-pushed the td-chunked-vector-json branch from 542ac71 to 44e087a Compare February 20, 2025 01:25

tchaikov reviewed Feb 20, 2025

View reviewed changes

travisdowns force-pushed the td-chunked-vector-json branch from 44e087a to 9f22206 Compare February 20, 2025 12:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support chunked_vector and other json/httpd changes #2647

Support chunked_vector and other json/httpd changes #2647

travisdowns commented Feb 17, 2025

tchaikov left a comment

tchaikov Feb 18, 2025

travisdowns Feb 18, 2025

avikivity Feb 18, 2025

tchaikov commented Feb 18, 2025

nyh Feb 18, 2025

nyh Feb 18, 2025

travisdowns Feb 18, 2025

nyh Feb 18, 2025

travisdowns Feb 18, 2025

avikivity Feb 18, 2025

avikivity Feb 18, 2025

travisdowns Feb 19, 2025 •

edited

Loading

avikivity Feb 19, 2025

travisdowns Feb 20, 2025

avikivity Feb 18, 2025

travisdowns Feb 20, 2025 •

edited

Loading

avikivity Feb 18, 2025

avikivity commented Feb 18, 2025

nvartolomei Feb 19, 2025

nvartolomei Feb 19, 2025

travisdowns Feb 19, 2025

travisdowns Feb 20, 2025

tchaikov Feb 20, 2025

travisdowns Feb 20, 2025

Support chunked_vector and other json/httpd changes #2647

Are you sure you want to change the base?

Support chunked_vector and other json/httpd changes #2647

Conversation

travisdowns commented Feb 17, 2025

tchaikov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tchaikov commented Feb 18, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

travisdowns Feb 19, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

travisdowns Feb 20, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avikivity commented Feb 18, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

travisdowns Feb 19, 2025 •

edited

Loading

travisdowns Feb 20, 2025 •

edited

Loading