Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

websocket: Reduce number of re-allocation of string parameters #535

Merged
merged 6 commits into from
Sep 18, 2022

Conversation

kang-sw
Copy link
Contributor

@kang-sw kang-sw commented Sep 11, 2022

Removed multiple payload copy operations during sending payload

  1. Capturing 'msg' as value.
    https://github.com/kang-sw/Crow/blob/6f832f82fafea2c831f154869888041cd0a6a3e4/include/crow/websocket.h#L177
  2. Pushing back msg by copy
    https://github.com/kang-sw/Crow/blob/6f832f82fafea2c831f154869888041cd0a6a3e4/include/crow/websocket.h#L180
  3. Copying lambda function to executor
    https://github.com/kang-sw/Crow/blob/6f832f82fafea2c831f154869888041cd0a6a3e4/include/crow/websocket.h#L141

On this PR, all send_* parameter series will be refactored to take 'std::string' as value, and moved to lambda capture when dispatched to executor. This removes first copy. Lambda function will be declared as mutable, which enables modifying(moving) captured variable on invocation. This makes removing second copy available, by moving captured 'msg' instance to write_buffers_.
Lambda function itself is also copied to executor when this->dispatch() invocation, so it changed to use perfect forwarding to prevent redundant copy of captured variables, which removes third copy.

Copy link
Member

@dranikpg dranikpg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great improvements! Especially useful when sending large amounts of data over websockets.

Just a small issue with C++11 support.

Comment on lines 141 to 142
dispatch([this, msg = std::move(msg)]() mutable {
auto header = build_header(0x9, msg.size());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Crow still supports C++11. Unfortunately, move capture was added only in c++14.

We use #ifdef CROW_CAN_USE_CPP14 to handle such cases. Take a look at the routing.h as an example

Copy link
Contributor Author

@kang-sw kang-sw Sep 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops. I didn't know that was c++14 feature ... well, I'll make small update for this concept to work.


Resolved using asio::post instead of using io_service::post

And some other question ... How io_service().dispatch() handles received event instance, by moving, or copying? If it copies received handler, than we may remove that copy by using shared_ptr for every send_data request.

This makes copy operation of newly created lambda object relatively trivial, however, it would introduce unnecessary allocation if given string is smaller than SBO size. What do you think about this idea?

                auto p_msg = std::make_shared<std::string>(std::move(msg));
                dispatch([this, p_msg]() mutable {
                    auto header = build_header(0x9, p_msg->size());
                    write_buffers_.emplace_back(std::move(header));
                    write_buffers_.emplace_back(std::move(*p_msg));
                    do_write();
                });

If msg payload is larger than SBO, than it can reduce number of total allocations. Maybe simple 'new std::string' can be an option, but it can easily be error-prone, thus I don't prefer that option. As make_shared allocates control block at once, I believe above change does not introduce such a big overhead, compared to current 'always copy' implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For new commit 99217d8, I have extracted lambda capture as explicit structure, and took advantage of move sementics by moving given struct. It still takes all advantages of originally proposed, however, It seems SendMessageType as move-only type makes program un-compillable. It seems still there's copy operation exists.

If we use asio::post() instead of get_io_executor().post(), move-only object become compilable(in commit b6ec1bc). In this way, we don't have to create shared_ptr everytime to avoid copy.

Copy link
Member

@dranikpg dranikpg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sure it compiles with c++11 (warning: lambda capture initializers only available with ‘-std=c++14’ or ‘-std=gnu++14’ counts) and it'll be ready to merge

include/crow/websocket.h Outdated Show resolved Hide resolved
Comment on lines 641 to 683
struct SendMessageType
{
std::string payload;
Connection* self;
int opcode;

void operator()()
{
self->send_data_impl(this);
}

SendMessageType() noexcept = default;

SendMessageType(SendMessageType const&) = delete;
SendMessageType& operator=(SendMessageType const&) = delete;

SendMessageType(SendMessageType&&) noexcept = default;
SendMessageType& operator=(SendMessageType&&) noexcept = default;
};

static_assert(
std::is_nothrow_move_assignable<SendMessageType>::value &&
std::is_nothrow_move_constructible<SendMessageType>::value,
"SendMessageType must be nothrow movable!");

void send_data_impl(SendMessageType* s)
{
auto header = build_header(s->opcode, s->payload.size());
write_buffers_.emplace_back(std::move(header));
write_buffers_.emplace_back(std::move(s->payload));
do_write();
}

void send_data(int opcode, std::string&& msg)
{
SendMessageType event_arg{
std::move(msg),
this,
opcode};

post(std::move(event_arg));
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, 😨 This looks like a total overkill... I'll accept it anyway, but please note more code = more maintenance

Copy link
Contributor Author

@kang-sw kang-sw Sep 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I have applied changes for removing lambda capture. Since close() routine does not share any routine with other 4(send_text, binary, ping, pong) and much less frequently called, I just roll-backed close() function's refactor.

Additionally, I removed unnecessary assertions & operator declartions for SendMessageType struct on commit 02c0f8d (Those were just compile time check whether move operation is properly handled)

@dranikpg dranikpg merged commit 1e47bc6 into CrowCpp:master Sep 18, 2022
@dranikpg
Copy link
Member

@kang-sw Thanks! 🎊

Please keep it more simple next time 😅

We're currently collecting changes for 1.1, so it'll make it into this version in the nearest future

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants