rest: don't copy data when sending binary response #30321

romanz · 2024-06-22T08:51:32Z

Also, change HTTPRequest::WriteReply to accept std::span.

DrahtBot · 2024-06-22T08:51:34Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	laanwj, stickies-v
Stale ACK	maflcko, BrandonOdiwuor, tdb3

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

maflcko · 2024-06-22T09:53:09Z

lgtm ACK 07770609afddaa1c730b431c207015f34b3a5f5f

A benchmark would be nice (for fun), but this looks good either way.

laanwj · 2024-06-22T14:06:15Z

LGTM ACK 07770609afddaa1c730b431c207015f34b3a5f5f
Nice, simple optimization.

laanwj · 2024-06-22T14:07:54Z

src/httpserver.cpp

Not sure if a Span<byte> or string_view is best suited here. In principle we're dealing with binary data here, not necessarily a string.

Added 9c9375c with a few more fixes.

BrandonOdiwuor

Code Review ACK 07770609afddaa1c730b431c207015f34b3a5f5f

tdb3

ACK 07770609afddaa1c730b431c207015f34b3a5f5f

Simple and efficient. Looks good.

Tested with REST calls on two different nodes with signet block 201185 (one running PR 30321 and one running master (5383637 at the time)). Output from both branches matched (hash checked).

PR 30321:

curl -v http://127.0.0.1:38332/rest/block/0000005d9cd746eeae8078c9bdb40866e02ddde50680c4c2d9d396dfdb6d3351.bin --output pr30321.bin
sha256sum pr30321.bin
0f88a3cda3b44b2f0fc66006f5ac1349cc8e6a2e7a70f7d5f756355c61d58c76  pr30321.bin

master:

curl -v http://127.0.0.1:38332/rest/block/0000005d9cd746eeae8078c9bdb40866e02ddde50680c4c2d9d396dfdb6d3351.bin --output master.bin
sha256sum master.bin
0f88a3cda3b44b2f0fc66006f5ac1349cc8e6a2e7a70f7d5f756355c61d58c76  master.bin

Also took a quick look at adjacent code and at the other methods in HTTPRequest to see if there might be other spots benefitting from copy avoidance with string_view. Nothing popped out that wouldn't induce more significant refactoring (i.e. riskier).

romanz · 2024-06-24T20:36:19Z

Added 9c9375c with a few more fixes, following the suggestion above.

tdb3

re ACK 9c9375cf3b581c31f047b87a2c05507dc7b31804

Re-ran the tests in #30321 (review)

Also performed similar PR branch / master (5383637) comparison tests with tx, headers, blockhashbyheight, and getutxos endpoints. All matched.

curl -v http://127.0.0.1:38332/rest/tx/7382c61e570cbc882a3d32b272318d353386981a475f58341fb4268971a6f1ec.bin
curl -v http://127.0.0.1:38332/rest/headers/0000005d9cd746eeae8078c9bdb40866e02ddde50680c4c2d9d396dfdb6d3351.bin
curl -v http://127.0.0.1:38332/rest/blockhashbyheight/201185.bin
curl -v http://127.0.0.1:38332/rest/getutxos/7382c61e570cbc882a3d32b272318d353386981a475f58341fb4268971a6f1ec-0.bin

stickies-v · 2024-06-25T10:38:22Z

Concept ACK. Can you tidy up the fixup commit please?

stickies-v

Code LGTM 9c9375cf3b581c31f047b87a2c05507dc7b31804 - just needs squashing.

nit: could use std::span to avoid merge conflict with #29119?

git diff

diff --git a/src/httpserver.cpp b/src/httpserver.cpp
index adc6040d08..b6c6db8b35 100644
--- a/src/httpserver.cpp
+++ b/src/httpserver.cpp
@@ -26,6 +26,7 @@
 #include <deque>
 #include <memory>
 #include <optional>
+#include <span>
 #include <string>
 #include <unordered_map>
 
@@ -634,7 +635,7 @@ void HTTPRequest::WriteHeader(const std::string& hdr, const std::string& value)
  * Replies must be sent in the main loop in the main http thread,
  * this cannot be done from worker threads.
  */
-void HTTPRequest::WriteReply(int nStatus, Span<const std::byte> reply)
+void HTTPRequest::WriteReply(int nStatus, std::span<const std::byte> reply)
 {
     assert(!replySent && req);
     if (m_interrupt) {
diff --git a/src/httpserver.h b/src/httpserver.h
index c193cf2a7f..907ed1a72b 100644
--- a/src/httpserver.h
+++ b/src/httpserver.h
@@ -7,10 +7,9 @@
 
 #include <functional>
 #include <optional>
+#include <span>
 #include <string>
 
-#include <span.h>
-
 namespace util {
 class SignalInterrupt;
 } // namespace util
@@ -132,9 +131,9 @@ public:
      */
     void WriteReply(int nStatus, std::string_view reply = "")
     {
-        WriteReply(nStatus, AsBytes(Span{reply}));
+        WriteReply(nStatus, std::as_bytes(std::span{reply}));
     }
-    void WriteReply(int nStatus, Span<const std::byte> reply);
+    void WriteReply(int nStatus, std::span<const std::byte> reply);
 };
 
 /** Get the query parameter value from request uri for a specified key, or std::nullopt if the key

src/httpserver.h

laanwj · 2024-06-25T13:12:59Z

nit: could use std::span to avoid merge conflict with #29119?

i think the problem with prematurely porting PRs to std::span is that utility functions (at least ToBytes) haven't been ported to std::span.

stickies-v · 2024-06-25T13:18:37Z

i think the problem with prematurely porting PRs to std::span is that utility functions (at least ToBytes) haven't been ported to std::span.

I think it's fine to just use std::as_bytes (as done in my diff)? No other utilities needed here. Anyway, it's a nit - either is fine for me. (And I'm not 100% sure if std::as_bytes is supported on all compilers we use)

laanwj · 2024-06-25T13:29:42Z

Oh, hadn't seen that diff, sorry, somehow expected it to be more involved.

DrahtBot · 2024-06-25T22:48:11Z

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the
documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

_{Debug: https://github.com/bitcoin/bitcoin/runs/26671061608}

Also, change `HTTPRequest::WriteReply` to accept `std::span`.

laanwj · 2024-06-26T08:48:17Z

re-ACK 1556d21

stickies-v

ACK 1556d21

romanz · 2024-06-26T11:33:55Z

Many thanks!

Also, change `HTTPRequest::WriteReply` to accept `std::span`. Github-Pull: bitcoin#30321 Rebased-From: 1556d21

DrahtBot added the RPC/REST/ZMQ label Jun 22, 2024

laanwj reviewed Jun 22, 2024

View reviewed changes

BrandonOdiwuor approved these changes Jun 22, 2024

View reviewed changes

tdb3 approved these changes Jun 23, 2024

View reviewed changes

romanz changed the title ~~rest: don't copy block data when sending binary response~~ rest: don't copy data when sending binary response Jun 24, 2024

tdb3 approved these changes Jun 24, 2024

View reviewed changes

DrahtBot requested review from BrandonOdiwuor, laanwj and maflcko June 24, 2024 22:47

stickies-v reviewed Jun 25, 2024

View reviewed changes

src/httpserver.h Outdated Show resolved Hide resolved

romanz force-pushed the http-string-view branch from 9c9375c to 24e7925 Compare June 25, 2024 18:48

romanz requested a review from stickies-v June 25, 2024 18:58

DrahtBot added the CI failed label Jun 25, 2024

rest: don't copy data when sending binary response

1556d21

Also, change `HTTPRequest::WriteReply` to accept `std::span`.

romanz force-pushed the http-string-view branch from 24e7925 to 1556d21 Compare June 26, 2024 03:49

DrahtBot removed the CI failed label Jun 26, 2024

DrahtBot requested a review from tdb3 June 26, 2024 08:48

stickies-v approved these changes Jun 26, 2024

View reviewed changes

fanquake merged commit b4b9854 into bitcoin:master Jun 26, 2024

romanz deleted the http-string-view branch June 26, 2024 11:33

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Aug 1, 2024

rest: don't copy data when sending binary response

8a42c1a

Also, change `HTTPRequest::WriteReply` to accept `std::span`. Github-Pull: bitcoin#30321 Rebased-From: 1556d21

pinheadmz mentioned this pull request Apr 29, 2025

Replace libevent with our own HTTP and socket-handling implementation #32061

Open

bitcoin locked and limited conversation to collaborators Sep 20, 2025

rest: don't copy data when sending binary response #30321

rest: don't copy data when sending binary response #30321

Uh oh!

Conversation

romanz commented Jun 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jun 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Uh oh!

maflcko commented Jun 22, 2024

Uh oh!

laanwj commented Jun 22, 2024

Uh oh!

laanwj Jun 22, 2024

Choose a reason for hiding this comment

Uh oh!

romanz Jun 24, 2024

Choose a reason for hiding this comment

Uh oh!

BrandonOdiwuor left a comment

Choose a reason for hiding this comment

Uh oh!

tdb3 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

romanz commented Jun 24, 2024

Uh oh!

tdb3 left a comment

Choose a reason for hiding this comment

Uh oh!

stickies-v commented Jun 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stickies-v left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

laanwj commented Jun 25, 2024

Uh oh!

stickies-v commented Jun 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Jun 25, 2024

Uh oh!

DrahtBot commented Jun 25, 2024

Uh oh!

laanwj commented Jun 26, 2024

Uh oh!

stickies-v left a comment

Choose a reason for hiding this comment

Uh oh!

romanz commented Jun 26, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

romanz commented Jun 22, 2024 •

edited

Loading

DrahtBot commented Jun 22, 2024 •

edited

Loading

tdb3 left a comment •

edited

Loading

stickies-v commented Jun 25, 2024 •

edited

Loading

stickies-v left a comment •

edited

Loading

stickies-v commented Jun 25, 2024 •

edited

Loading