Stiffstream / restinio

Cross-platform, efficient, customizable, and robust asynchronous HTTP(S)/WebSocket server C++ library with the right balance between performance and ease of use
Other
1.15k stars 93 forks source link

needed_storage_max_size is smaller than a fmt::basic_memory_buffer<char, 1> #66

Closed ned14 closed 4 years ago

ned14 commented 4 years ago

If one is using {fmt} to format a response to be sent by restinio, it would be great if needed_storage_max_size were big enough to accommodate a fmt::basic_memory_buffer<char, 1> (at least 40 bytes on x64), and then we could send the {fmt} buffer directly without memory copying.

eao197 commented 4 years ago

Thanks for the suggestion!

I'll try to address it after a release of v.0.6.2 (that release is almost ready).

eao197 commented 4 years ago

It seems that the simplest way to solve this issue is to define needed_storage_max_size that way:

constexpr std::size_t needed_storage_max_size =
    std::max< std::size_t >( {
        sizeof( empty_buf_t ),
        sizeof( const_buf_t ),
        sizeof( string_buf_t ),
        sizeof( shared_datasizeable_buf_t< std::string > ),
        sizeof( sendfile_write_operation_t ),
        sizeof( fmt::basic_memory_buffer<char,1> ) } );

But I don't understand how this helps to "send the {fmt} buffer directly without memory copying" :( Maybe some support for fmt::basic_memory_buffer should be added to restinio::writable_item_t too...

ned14 commented 4 years ago

What I'm doing right now:

        std::shared_ptr<fmt::memory_buffer> reply = std::make_shared<fmt::memory_buffer>();
...
            fmt::format_to(
               *reply,
               R"({{"time":"{:#x}","type":"add","price":"{:#x}","quantity":"{:#x}","side":"{:s}","rank":"{:#x}","id":"{:#x}"}})",  //
               u->timestamp,            //
               u->price,                //
               u->quantity,             //
               u->buy ? "bid" : "ask",  //
               u->position,             //
               u->order_id);
...
      req->create_response()                                                                 //
         .append_header(restinio::http_field::server, MDX_SERVER_STRING)                     //
         .append_header(restinio::http_field::content_type, update_printer.response_type())  //
         .append_header_date_field()                                                         //
         .set_body(std::move(reply))                                          //
         .done();
      return restinio::request_accepted();

What I'd like is the need for the shared_ptr to go away. The entire response path is dynamic memory allocation free, except for that and the fmt::memory_buffer.

I think that your suggestion that support for sending fmt::memory_buffer as part of the response would work well. Some method of being notified when restinio is done with the memory buffer would also be useful, so I can recycle them.

eao197 commented 4 years ago

I think that your suggestion that support for sending fmt::memory_buffer as part of the response would work well.

Actually fmt::memory_buffer is fmt::basic_memory_buffer<char, 500, std::allocator<char>>. Where 500 means the size of the internal store inside basic_memory_buffer instance. I'm afraid we can't afford to store such large objects inside our writable_item_t.

But if we do support fmt::basic_memory_buffer<char, 1> then the internal buffer inside basic_memory_buffer will be just one byte long and basic_memory_buffer will allocate memory. So there won't be an allocation for fmt::basic_memory_buffer itself and you can write something like:

        fmt::basic_memory_buffer<char, 1> reply;
...
            fmt::format_to(
               reply,
               R"({{"time":"{:#x}","type":"add","price":"{:#x}","quantity":"{:#x}","side":"{:s}","rank":"{:#x}","id":"{:#x}"}})",  //
               u->timestamp,            //
               u->price,                //
               u->quantity,             //
               u->buy ? "bid" : "ask",  //
               u->position,             //
               u->order_id);
...
      req->create_response()                                                                 //
         .append_header(restinio::http_field::server, MDX_SERVER_STRING)                     //
         .append_header(restinio::http_field::content_type, update_printer.response_type())  //
         .append_header_date_field()                                                         //
         .set_body(std::move(reply))                                          //
         .done();
      return restinio::request_accepted();

but there will be memory allocations inside call to fmt::format_to.

Some method of being notified when restinio is done with the memory buffer would also be useful, so I can recycle them.

There are after-write notificators. They can be used for such purposes.

ned14 commented 4 years ago

fmt::basic_memory_buffer<char, 1> would work for me. You may wish to typedef that, or ask Victor to add a typedef for fmt::basic_memory_buffer<char, 1> e.g. fmt::dynamic_memory_buffer.

eao197 commented 4 years ago

But in the case of hypothetical fmt::dynamic_memory_buffer there almost won't be a difference between shared_ptr<fmt::memory_buffer> and fmt::dynamic_memory_buffer in sense of dynamic memory allocation. You either allocate shared_ptr<fmt::memory_buffer> and use the internal buffer of that memory_buffer instance or fmt::dynamic_memory_buffer will allocate memory on a call to fmt::format_to.

So I still don't get the idea :(

Maybe your response bodies longer than 500 bytes, in that the case usage of fmt::dynamic_memory_buffer can have sense. But in that case you can't reuse the buffer after moving it into restinio::writable_item_t :(

ned14 commented 4 years ago

I'm mainly looking for simplification. shared ptrs have other costs than just the extra memory allocation, they actively inhibit the optimiser and the ability of the CPU to parallelise opcode dispatch.

Our code base almost never calls malloc, incidentally. We almost exclusively use page faulting to allocate memory, that way the compiler can optimise much more aggressively. fmt::basic_memory_buffer works for us because we can expand them up real large, then reset their pages using LLFIO, that way we can keep a pool of them around and they don't cost anything except address space.

eao197 commented 4 years ago

Ok, now I see the point. I'll add support of fmt::basic_memory_buffer<char,1> to restinio::writable_item_t.

eao197 commented 4 years ago

The support for fmt::basic_memory_buffer<char,1> is in 0.6-dev branch. An example is here.

It will be merged into the master branch at the release of v.0.6.3. I hope at the end of the week.

ned14 commented 4 years ago

Cool, thanks for the fix