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

pull stickies-v wants to merge 3 commits into bitcoin:master from stickies-v:rpc-convert-string-view changing 4 files +31 −19

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

Type	Reviewers
ACK	ryanofsky, MarcoFalke
Approach ACK	pablomartin4btc

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:

        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 📻

<details><summary>Show signature</summary>

Signature:

</details>

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

Contributors

Labels

Linked (view graph)

#26506 refactor: rpc: use convenience fn to auto parse non-string parameters #27256 refactor: rpc: Remove unnecessary uses of ParseNonRFCJSONValue() and rename it

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

Questions

Reviews

Conflicts