refactor: RPC: pass named argument value as string_view #26612
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsNo conflicts as of last run. |
Since jgarzik/univalue#31, UniValue::read() can now parse raw literals directly, so there is no more need to wrap them into an array first.
Minimize copying RPC named argument values when calling .substr() by using std::string_view instead of std::string.
d53953f to
545ff92
Compare
|
Rebased now that #26506 is merged, ready for review! Also added to PR description:
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
nice
review ACK 545ff92 📻
Show signature
Signature:
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
review ACK 545ff924ab6303ffabd91fdfc4f0a4962daf133c 📻
-----BEGIN PGP SIGNATURE-----
iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUhxDQwAnkUGV9vWw6I02mle/ZFfeIWLL4UhIJjNmcc0UH+0DwtK7IM9V74ST0QZ
8iv5AOTv43xJ9Ltye54nIbJnjIhW1WmZ/nhwTHDqJtQHP2GqF4ao2uQo2nGMEt/G
O3BixrcDyYHQDOR92bou9pzc7BOJpQ3Imhbq12FmQrVFSDsREF9wyMU0sPm7OypZ
ZIBSZc8/7n3KzGFd4qwBwQHKP0Do4vngE/PnuW+OF5u8k8+xe1kny4KBSadccpNl
eeO/VTeH2hip8pgBweKG7iuWRvHNtsEnrRdHIKLpTQh13l1L4VaRiz/GzkeGDEGf
YdRDOI9f5CT3slSC93hEKODOakmWVxQ/7dvprogcefNpZSq1jYIrK1uK/z1ZSJ1u
FuvPBv6WZ4AhJkdFA9jzLKEZ/Fr6Bur0qeXJNQdoa7CpHK3tsEJ5KqhhFbjhrxlF
vDlABZ6iVD2S4hKF7keu7et+cxMSxsB6VvTd/3tI46TxZqGlyv7bE2VuPBGyiNst
pioPNtzT
=pTHK
-----END PGP SIGNATURE-----
@stickies-v will you follow up here? |
Done in #27256 (cc @ryanofsky) |
…f ParseNonRFCJSONValue() and rename it cfbc8a6 refactor: rpc: hide and rename ParseNonRFCJSONValue() (stickies-v) 6c8bde6 test: move coverage on ParseNonRFCJSONValue() to UniValue::read() (stickies-v) Pull request description: Follow-up to bitcoin/bitcoin#26612 (comment). As per bitcoin/bitcoin#26506 (review), `ParseNonRFCJSONValue()` is no longer necessary and we can use `UniValue::read()` directly: > IIRC before that PR UniValue::read could only parse JSON object and array values, but now it will parse string/number/boolean/null values as well. laanwj pointed this out in bitcoin/bitcoin#9028 (comment) The implementation of `ParseNonRFCJSONValue()` was already [simplified in #26612](https://github.com/bitcoin/bitcoin/pull/26612/files#diff-84c7a7f36362b9724c31e5dec9879b2f81eae0d0addbc9c0933c3558c577de65R259-R263) and [test coverage updated](https://github.com/bitcoin/bitcoin/pull/26612/files#diff-fc0f86b6c3bb23b0e983bcf79d7546d1c9eaa15d6e4d8a7b03b5b85955f585abR292-R312) to ensure behaviour didn't change. To avoid code duplication, we keep the function to throw on invalid input data but rename it to `Parse()` and remove it from the header. The existing test coverage we had on `ParseNonRFCJSONValue()` is moved over to `UniValue::read()`. ACKs for top commit: ryanofsky: Code review ACK cfbc8a6. Only change since last review is adding a new test Tree-SHA512: 89be959d2353af7ace0c1348ba1600b9ac1d3c7b3cf7f0b59f6e005b7fb9d695ce3e8720e1be3cf77fe7e318a4017c880df108928e7179ec50447583d13bc781
Inspired by MarcoFalke's comment. Main purpose of this PR is to minimize copying (potentially large) RPC named arguments when calling
.substr()by usingstd::string_viewinstead ofstd::string. Furthermore, cleans up the code by removing unnecessary complexity inParseNonRFCJSONValue()(done first to avoid refactoring required to concatenatestringandstring_view), updates some naming and adds a few test cases. Should not introduce any behaviour change.Questions
Was there actually any merit toParseNonRFCJSONValue()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.ParseNonRFCJSONValue()to a localParse()helper function (that throws if invalid), remove it fromclient.hand merge the test coverage we currently have onParseNonRFCJSONValue()with the coverage we have onUniValue::read().