Reduce syscall volume, and/or TCP segment generation - #2541
Conversation
|
Thanks for the PR! The benchmark could not see this change at all: its only endpoint returns a 12-byte 1. Large TLS responses lose a third to a half.
Five alternating rounds each; every bold row separates at p = 0.008, and
Handing a span larger than the staging buffer straight to 2. A response can promise a body and send none. TEST(BorrowedContentTest, ReplacingViewWithEmptyBodyClearsContentLength) {
Server svr;
const std::string data = "0123456789abcdefghij"; // 20 bytes
svr.Get("/replaced", [&](const Request & /*req*/, Response &res) {
res.set_content(data.data(), data.size(), "text/plain", nullptr);
res.set_content("", "text/plain"); // drop it again
});
auto listen_thread = std::thread([&svr]() { svr.listen("localhost", PORT); });
auto se = detail::scope_exit([&] {
svr.stop();
listen_thread.join();
});
svr.wait_until_ready();
Client cli("localhost", PORT);
cli.set_read_timeout(1, 0);
auto res = cli.Get("/replaced");
ASSERT_TRUE(res) << "Error: " << to_string(res.error());
EXPECT_EQ("0", res->get_header_value("Content-Length"));
EXPECT_EQ("", res->body);
}On the branch this reports inline void Response::set_content(const char *s, size_t n,
const std::string &content_type) {
body.assign(s, n);
content_view_data_ = nullptr;
content_length_ = 0; // add
content_provider_ = nullptr; // add
is_chunked_content_provider_ = false; // addplus the same three lines in the 3. The releaser is not always called. TEST(BorrowedContentTest, ReplacingContentReleasesTheOldView) {
Server svr;
const std::string a = "AAAA";
const std::string b = "BBBB";
std::atomic<int> released_a{0};
svr.Get("/twice", [&](const Request & /*req*/, Response &res) {
res.set_content(a.data(), a.size(), "text/plain",
[&](bool) { released_a++; });
res.set_content(b.data(), b.size(), "text/plain", nullptr);
});
auto listen_thread = std::thread([&svr]() { svr.listen("localhost", PORT); });
auto se = detail::scope_exit([&] {
svr.stop();
listen_thread.join();
});
svr.wait_until_ready();
Client cli("localhost", PORT);
auto res = cli.Get("/twice");
ASSERT_TRUE(res) << "Error: " << to_string(res.error());
EXPECT_EQ("BBBB", res->body);
EXPECT_EQ(1, released_a.load()); // the discarded view is still released
}Setting content a second time overwrites the previous releaser instead of invoking it, so void clear_content() {
if (content_provider_resource_releaser_) { // add
content_provider_resource_releaser_(false); // add
content_provider_resource_releaser_ = nullptr; // add
}
body.clear();Nothing to do about abidiff, by the way. It failing just makes the next release a minor bump instead of a patch. |
|
Wow - thanks, that's a more detailed review than I expected, and benchmarking feels like you've done some of my homework for me. Thank you. I will rebase, take a good look, and report back. Overall: do you think the juice will be worth the squeeze? I'm primarily optimizing for small embedded hosts, and if this PR feels like a distraction from your primary use case, please say so. Thanks, |
c8afb41 to
e508d80
Compare
Address yhirose's review of yhirose#2541. Stream::writev(): - only flatten write calls into a local buffer when they are short. Otherwise, iterate over the individual buffers in the writev() and hand them one-by-one to a normal write(). The net result is a ~30% speedup over baseline on SSL, which is surprising - the unexpected gains come from an extraneous copy in the baseline code we dropped in the new code. Response: - the copying set_content() overloads now clear content_length_, content_provider_ and is_chunked_content_provider_, so replacing a borrowed view with an owned body can no longer emit a Content-Length describing bytes that are never sent. - The bare releaser is replaced with ContentResource, a shared-ownership holder whose state destructor fires the releaser via RAII. This moves clean-up into the type system so we don't have to worry about corner cases. This also removes the need for explicit constructors/destructor on Resource.
|
@yhirose - I have rebased to catch your changes to master, and pushed out a squashable commit to address your comments. If you prefer, I can squash this commit myself (leaving just the two logical commits you would want to merge.) I've also reproduced your benchmarks and think the regressions are addressed now. |
Address yhirose's review of yhirose#2541. Stream::writev(): - only flatten write calls into a local buffer when they are short. Otherwise, iterate over the individual buffers in the writev() and hand them one-by-one to a normal write(). The net result is a ~30% speedup over baseline on SSL, which is surprising - the unexpected gains come from an extraneous copy in the baseline code we dropped in the new code. Response: - the copying set_content() overloads now clear content_length_, content_provider_ and is_chunked_content_provider_, so replacing a borrowed view with an owned body can no longer emit a Content-Length describing bytes that are never sent. - The bare releaser is replaced with ContentResource, a shared-ownership holder whose state destructor fires the releaser via RAII. This moves clean-up into the type system so we don't have to worry about corner cases. This also removes the need for explicit constructors/destructor on Resource.
4980747 to
4158693
Compare
Normally, httplib generates one TCP segment for headers and at least one
more for body content. This performs badly without TCP_NODELAY ("It's
always TCP_NODELAY. Every damn time." [1]). With TCP_NODELAY, small
responses can become unnecessarily segmented and throughput can suffer
on deeply embedded hosts due to syscall overhead.
This patch adds a vectorized (gather) write to Stream and threads it
through httplib just enough to combine headers and body content into a
single, zero-copy syscall under "happy path" conditions (chiefly, no
ranged requests).
- SocketStream::writev() wraps sendmsg() (WSASend() under Win32),
allowing multiple buffers to leave in a single syscall. Stream
provides a default implementation for streams without native gather
support (notably TLS, where every write() is at least one record on
the wire): small totals are flattened into a staging buffer and leave
as a single write(), and large ones are written buffer-by-buffer so
malloc/memcpy overheads don't outweigh the syscall overheads we're
trying to duck.
- A new Response::set_content() overload allows callers to provide a
copy-free "borrowed view" into C arrays. The existing std::move-based
set_content() call isn't useful for non-std::string arguments, and
the existing Response::set_content(char*, size_t) call copied.
This new call requires some pointer-lifetime effort but allows
copy-free responses to come from non-strings (e.g. mmap'd files); both
static-mount and set_file_content() responses now use it.
- Server::write_response_core uses writev() and "borrowed view" content
to entirely combine most Responses into a single syscall. This avoids
excess TCP segment generation (a good thing, with or without NODELAY).
- A new Response::clear_content() drops pending content at the
error-reset sites (413/416/404 and handler exceptions), and every
content setter clears a previously set borrowed view, so stale
borrowed bytes can never trail a response whose headers no longer
describe them.
- Releaser bookkeeping moves from ~Response into a shared
detail::ContentResource that invokes the releaser exactly once, when
the last Response referencing it falls out of scope. This lets
clear_content() and the content setters safely drop or replace a
releaser-bearing body, and gives Response its defaulted special member
functions back.
[1]: https://brooker.co.za/blog/2024/05/09/nagle.html
4158693 to
6968237
Compare
|
(rebased off master and simplified to match recent changes.) |
2 participants
Normally, httplib generates one TCP segment for headers and at least one more for body content. This is awful without TCP_NODELAY ("It's always TCP_NODELAY. Every damn time." [1]). Even with TCP_NODELAY, small responses can become unnecessarily segmented and throughput can suffer on deeply embedded hosts due to syscall overhead.
This patch series threads a vectorized write syscall (writev() on Linux/MacOS, something else under Win32) through httplib just enough to combine headers and body content into a single, zero-copy syscall under "happy path" conditions (chiefly, no ranged requests).
New Response::set_content() allows callers to provide a copy-free "borrowed view" into C arrays. The existing std::move-based set_content() call isn't useful for non-std::string arguments, and the existing Response::set_content(char*, size_t) call required copying. This new call requires some pointer-lifetime effort but allows copy-free responses to come from non-strings (e.g. mmap'd files); both static-mount and set_file_content() responses now use it.
Server::write_response_core uses writev() and "borrowed view" content to entirely combine most Responses into a single syscall. This avoids excess TCP segment generation (a good thing with or without NODELAY), and on resource-constrained systems, the avoided syscall overhead itself can be meaningful.
A new Response::clear_content() drops all pending content representations at the error-reset sites and uncaught handler exceptions, and every content setter clears a previously set borrowed view, so stale borrowed bytes can never trail a response whose headers no longer describe them.
The abidiff test fails (because the ABI did change, and I haven't modified anything to match). I'm uncertain how you handle ABI changes but happy to follow your lead.