rest: don't copy data when sending binary response

romanz commented at 8:51 am on June 22, 2024: contributor

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

DrahtBot commented at 8:51 am on June 22, 2024: contributor

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.

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

maflcko commented at 9:53 am on June 22, 2024: member

lgtm ACK 07770609afddaa1c730b431c207015f34b3a5f5f

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

laanwj commented at 2:06 pm on June 22, 2024: member

LGTM ACK 07770609afddaa1c730b431c207015f34b3a5f5f Nice, simple optimization.

in src/httpserver.cpp:637 in 07770609af outdated

633@@ -634,7 +634,7 @@ void HTTPRequest::WriteHeader(const std::string& hdr, const std::string& value)
634  * Replies must be sent in the main loop in the main http thread,
635  * this cannot be done from worker threads.
636  */
637-void HTTPRequest::WriteReply(int nStatus, const std::string& strReply)
638+void HTTPRequest::WriteReply(int nStatus, const std::string_view strReply)

laanwj commented at 2:07 pm on June 22, 2024:

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.

romanz commented at 4:48 am on June 24, 2024:

Added https://github.com/bitcoin/bitcoin/commit/9c9375cf3b581c31f047b87a2c05507dc7b31804 with a few more fixes.

BrandonOdiwuor approved

BrandonOdiwuor commented at 6:18 pm on June 22, 2024: contributor

Code Review ACK 07770609afddaa1c730b431c207015f34b3a5f5f

tdb3 approved

tdb3 commented at 2:20 am on June 23, 2024: contributor

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 (538363738e9e30813cf3e76ca4f71c1aaff349e7 at the time)). Output from both branches matched (hash checked).

PR 30321:

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

master:

0curl -v http://127.0.0.1:38332/rest/block/0000005d9cd746eeae8078c9bdb40866e02ddde50680c4c2d9d396dfdb6d3351.bin --output master.bin
1sha256sum master.bin
20f88a3cda3b44b2f0fc66006f5ac1349cc8e6a2e7a70f7d5f756355c61d58c76  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 renamed this:
~~rest: don't copy block data when sending binary response~~
rest: don't copy data when sending binary response
on Jun 24, 2024

romanz commented at 8:36 pm on June 24, 2024: contributor

Added https://github.com/bitcoin/bitcoin/commit/9c9375cf3b581c31f047b87a2c05507dc7b31804 with a few more fixes, following the suggestion above.

tdb3 approved

tdb3 commented at 10:47 pm on June 24, 2024: contributor

re ACK 9c9375cf3b581c31f047b87a2c05507dc7b31804

Re-ran the tests in #30321#pullrequestreview-2133999354

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

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

DrahtBot requested review from maflcko on Jun 24, 2024

DrahtBot requested review from laanwj on Jun 24, 2024

DrahtBot requested review from BrandonOdiwuor on Jun 24, 2024

stickies-v commented at 10:38 am on June 25, 2024: contributor

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

in src/httpserver.h:12 in 9c9375cf3b outdated

 8@@ -9,6 +9,8 @@
 9 #include <optional>
10 #include <string>
11 
12+#include <span.h>

stickies-v commented at 12:06 pm on June 25, 2024:

nit: we typically include our own headers first, and include in httpserver.cpp is missing

 0diff --git a/src/httpserver.cpp b/src/httpserver.cpp
 1index adc6040d08..6ba8754485 100644
 2--- a/src/httpserver.cpp
 3+++ b/src/httpserver.cpp
 4@@ -13,6 +13,7 @@
 5 #include <netbase.h>
 6 #include <node/interface_ui.h>
 7 #include <rpc/protocol.h> // For HTTP status codes
 8+#include <span.h>
 9 #include <sync.h>
10 #include <util/check.h>
11 #include <util/signalinterrupt.h>
12diff --git a/src/httpserver.h b/src/httpserver.h
13index c193cf2a7f..ffba4864aa 100644
14--- a/src/httpserver.h
15+++ b/src/httpserver.h
16@@ -5,12 +5,12 @@
17 #ifndef BITCOIN_HTTPSERVER_H
18 #define BITCOIN_HTTPSERVER_H
19 
20+#include <span.h>
21+
22 #include <functional>
23 #include <optional>
24 #include <string>
25 
26-#include <span.h>
27-
28 namespace util {
29 class SignalInterrupt;
30 } // namespace util

romanz commented at 6:52 pm on June 25, 2024:

Thanks - fixed in 24e7925abb.

stickies-v commented at 12:57 pm on June 25, 2024: contributor

Code LGTM 9c9375cf3b581c31f047b87a2c05507dc7b31804 - just needs squashing.

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

 0diff --git a/src/httpserver.cpp b/src/httpserver.cpp
 1index adc6040d08..b6c6db8b35 100644
 2--- a/src/httpserver.cpp
 3+++ b/src/httpserver.cpp
 4@@ -26,6 +26,7 @@
 5 #include <deque>
 6 #include <memory>
 7 #include <optional>
 8+#include <span>
 9 #include <string>
10 #include <unordered_map>
11 
12@@ -634,7 +635,7 @@ void HTTPRequest::WriteHeader(const std::string& hdr, const std::string& value)
13  * Replies must be sent in the main loop in the main http thread,
14  * this cannot be done from worker threads.
15  */
16-void HTTPRequest::WriteReply(int nStatus, Span<const std::byte> reply)
17+void HTTPRequest::WriteReply(int nStatus, std::span<const std::byte> reply)
18 {
19     assert(!replySent && req);
20     if (m_interrupt) {
21diff --git a/src/httpserver.h b/src/httpserver.h
22index c193cf2a7f..907ed1a72b 100644
23--- a/src/httpserver.h
24+++ b/src/httpserver.h
25@@ -7,10 +7,9 @@
26 
27 #include <functional>
28 #include <optional>
29+#include <span>
30 #include <string>
31 
32-#include <span.h>
33-
34 namespace util {
35 class SignalInterrupt;
36 } // namespace util
37@@ -132,9 +131,9 @@ public:
38      */
39     void WriteReply(int nStatus, std::string_view reply = "")
40     {
41-        WriteReply(nStatus, AsBytes(Span{reply}));
42+        WriteReply(nStatus, std::as_bytes(std::span{reply}));
43     }
44-    void WriteReply(int nStatus, Span<const std::byte> reply);
45+    void WriteReply(int nStatus, std::span<const std::byte> reply);
46 };
47 
48 /** Get the query parameter value from request uri for a specified key, or std::nullopt if the key

laanwj commented at 1:12 pm on June 25, 2024: member

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 commented at 1:18 pm on June 25, 2024: contributor

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 commented at 1:29 pm on June 25, 2024: member

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

romanz force-pushed on Jun 25, 2024

romanz requested review from stickies-v on Jun 25, 2024

DrahtBot commented at 10:48 pm on June 25, 2024: contributor

🚧 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

DrahtBot added the label CI failed on Jun 25, 2024

rest: don't copy data when sending binary response

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

1556d21599

romanz force-pushed on Jun 26, 2024

DrahtBot removed the label CI failed on Jun 26, 2024

laanwj commented at 8:48 am on June 26, 2024: member

re-ACK 1556d21599a250297d5f20e5249c970340ab08bc

DrahtBot requested review from tdb3 on Jun 26, 2024

stickies-v approved

stickies-v commented at 10:08 am on June 26, 2024: contributor

ACK 1556d21599a250297d5f20e5249c970340ab08bc

fanquake merged this on Jun 26, 2024

fanquake closed this on Jun 26, 2024

romanz deleted the branch on Jun 26, 2024

romanz commented at 11:33 am on June 26, 2024: contributor

Many thanks!

luke-jr referenced this in commit 8a42c1a519 on Aug 1, 2024

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

Code Coverage

Reviews