refactor: RPC: pass named argument value as string

stickies-v commented at 6:20 pm on November 30, 2022: contributor

Inspired by MarcoFalke’s comment. Main purpose of this PR is to minimize copying (potentially large) RPC named arguments when calling .substr() by using std::string_view instead of std::string. Furthermore, cleans up the code by removing unnecessary complexity in ParseNonRFCJSONValue() (done first to avoid refactoring required to concatenate string and string_view), updates some naming and adds a few test cases. Should not introduce any behaviour change.

Questions

~Was there actually any merit to ParseNonRFCJSONValue() surrounding the value with brackets and then parsing it as an array? I don’t see it, and the new approach doesn’t fail any tests. Still a bit suspicious about it though.~
- Cleared up by #26506#pullrequestreview-1211984059
- If there are no objections to 7727603e44f8f674e0fc8389e78047e2b56e6052, I think we should follow up with a PR to rename ParseNonRFCJSONValue() to a local Parse() helper function (that throws if invalid), remove it from client.h and merge the test coverage we currently have on ParseNonRFCJSONValue() with the coverage we have on UniValue::read().

DrahtBot commented at 6:20 pm on November 30, 2022: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	ryanofsky, MarcoFalke
Approach ACK	pablomartin4btc

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

Conflicts

No conflicts as of last run.

DrahtBot added the label Needs rebase on Dec 13, 2022

in src/rpc/client.cpp:233 in d53953f990 outdated

227@@ -225,11 +228,14 @@ class CRPCConvertTable
228 public:
229     CRPCConvertTable();
230 
231-    bool convert(const std::string& method, int idx) {
232-        return (members.count(std::make_pair(method, idx)) > 0);
233+    /* Return value as UniValue, and first parse it if it is a non-string parameter */
234+    UniValue ParseIfNonString(std::string_view value, const std::string& method, int idx) {
235+        return (members.count(std::make_pair(method, idx))) > 0 ? ParseNonRFCJSONValue(value) : value;

maflcko commented at 12:22 pm on January 18, 2023:

0        return (members.count({method, idx})) > 0 ? ParseNonRFCJSONValue(value) : value;

nit

test: add cases to JSON parsing 1d02e59901

refactor: reduce unnecessary complexity in ParseNonRFCJSONValue

Since https://github.com/jgarzik/univalue/pull/31, UniValue::read() can now
parse raw literals directly, so there is no more need to wrap them into an
array first.

7727603e44

refactor: use string_view for RPC named argument values

Minimize copying RPC named argument values when calling .substr() by
using std::string_view instead of std::string.

545ff924ab

stickies-v force-pushed on Jan 18, 2023

stickies-v renamed this:
~~[WIP] refactor: RPC: pass named argument as string_view~~
refactor: RPC: pass named argument value as string_view
on Jan 18, 2023

stickies-v marked this as ready for review on Jan 18, 2023

stickies-v commented at 5:18 pm on January 18, 2023: contributor

Rebased now that #26506 is merged, ready for review!

Also added to PR description:

If there are no objections to https://github.com/bitcoin/bitcoin/commit/7727603e44f8f674e0fc8389e78047e2b56e6052, I think we should follow up with a PR to rename ParseNonRFCJSONValue() to a local Parse() function, remove it from client.h and merge the test coverage we currently have on ParseNonRFCJSONValue() with the coverage we have on UniValue::read().

DrahtBot removed the label Needs rebase on Jan 18, 2023

DrahtBot added the label Refactoring on Jan 18, 2023

ryanofsky approved

ryanofsky commented at 6:55 pm on March 2, 2023: contributor

Code review ACK 545ff924ab6303ffabd91fdfc4f0a4962daf133c

I think the followup ideas in the PR description sound very good too

pablomartin4btc commented at 7:03 pm on March 2, 2023: member

Approach ACK.

I’ve investigated why (if?) we would need to declare const std::string_view. I see that while std::string_view is already read-only by design, declaring it as const can provide additional benefits in terms of communication, const correctness, and optimization but if we expect large values as @MarcoFalke mentioned on his comment perhaps we need to consider it. This is a very interesting article on reasons to pass std::string_view by value, there are links to godbolt snippets where it compares machine code differences on using one or the other approach.

Is there any benchmark that you think I could run in order to get some stats in terms of gains in performance improvement?

maflcko approved

maflcko commented at 10:36 am on March 3, 2023: member

nice

review ACK 545ff924ab6303ffabd91fdfc4f0a4962daf133c 📻

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3review ACK 545ff924ab6303ffabd91fdfc4f0a4962daf133c 📻
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUhxDQwAnkUGV9vWw6I02mle/ZFfeIWLL4UhIJjNmcc0UH+0DwtK7IM9V74ST0QZ
 88iv5AOTv43xJ9Ltye54nIbJnjIhW1WmZ/nhwTHDqJtQHP2GqF4ao2uQo2nGMEt/G
 9O3BixrcDyYHQDOR92bou9pzc7BOJpQ3Imhbq12FmQrVFSDsREF9wyMU0sPm7OypZ
10ZIBSZc8/7n3KzGFd4qwBwQHKP0Do4vngE/PnuW+OF5u8k8+xe1kny4KBSadccpNl
11eeO/VTeH2hip8pgBweKG7iuWRvHNtsEnrRdHIKLpTQh13l1L4VaRiz/GzkeGDEGf
12YdRDOI9f5CT3slSC93hEKODOakmWVxQ/7dvprogcefNpZSq1jYIrK1uK/z1ZSJ1u
13FuvPBv6WZ4AhJkdFA9jzLKEZ/Fr6Bur0qeXJNQdoa7CpHK3tsEJ5KqhhFbjhrxlF
14vDlABZ6iVD2S4hKF7keu7et+cxMSxsB6VvTd/3tI46TxZqGlyv7bE2VuPBGyiNst
15pioPNtzT
16=pTHK
17-----END PGP SIGNATURE-----

fanquake merged this on Mar 3, 2023

fanquake closed this on Mar 3, 2023

fanquake commented at 2:35 pm on March 3, 2023: member

I think the followup ideas in the PR description sound very good too @stickies-v will you follow up here?

stickies-v deleted the branch on Mar 3, 2023

sidhujag referenced this in commit 04dac02b35 on Mar 3, 2023

stickies-v commented at 3:00 pm on March 14, 2023: contributor

@stickies-v will you follow up here?

Done in #27256 (cc @ryanofsky)

fanquake referenced this in commit 436c185b05 on Jun 2, 2023

bitcoin locked this on Mar 13, 2024

refactor: RPC: pass named argument value as string_view #26612

Questions

Reviews

Conflicts