as string_view is optimized to be trivially copiable, whereas the current code creates a std::string copy at each call.
These utility methods are called by quite a few RPCs and tests, as well as by each other.
$ git grep "ParseHashV\|ParseHashO\|ParseHexV\|ParseHexO" | wc -l
61
Also remove an out-of-date external link.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For detailed information about the code coverage, see the test coverage report.
<!--021abf342d371248e50ceaed478a90ca-->
See 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.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
72 | @@ -73,7 +73,7 @@ CAmount AmountFromValue(const UniValue& value, int decimals) 73 | return amount; 74 | } 75 | 76 | -uint256 ParseHashV(const UniValue& v, std::string strName) 77 | +uint256 ParseHashV(const UniValue& v, const std::string& strName)
The names are short, so a copy or reference should make no difference. Also, a copy is still done when the input is a c-string, which is the case here for all call sites.
It looks like passing std::strings by reference to const (as done in ParseHashStr() in core_io.h) would be much better than passing string literals (for which by-value or by-reference-to-const doesn't matter, as you rightly point out).
<details><summary>test</summary><p>
// string-passing.cpp
//
// $ clang++ -std=c++17 string-passing.cpp && ./a.out
// foo(string) took 436096 cycles
// bar(string) took 156324 cycles
// foo("string literal") took 1464669 cycles
// bar("string literal") took 1463397 cycles
#include <ctime>
#include <iostream>
#include <string>
void foo(std::string s) { return; }
void bar(const std::string& s) { return; }
int main() {
std::string s = "blockhash";
auto start = clock();
for (int it = 0; it < 100000000; ++it) {
foo(s);
}
std::cout << "foo(string) took " << (clock() - start) << " cycles" << std::endl;
start = clock();
for (int it = 0; it < 100000000; ++it) {
bar(s);
}
std::cout << "bar(string) took " << (clock() - start) << " cycles" << std::endl;
start = clock();
for (int it = 0; it < 100000000; ++it) {
foo("blockhash");
}
std::cout << "foo(\"string literal\") took " << (clock() - start) << " cycles" << std::endl;
start = clock();
for (int it = 0; it < 100000000; ++it) {
bar("blockhash");
}
std::cout << "bar(\"string literal\") took " << (clock() - start) << " cycles" << std::endl;
return 0;
}
</p></details>
Replacing the blockhash, hash, and txid string literals by a constant would do it (and the parameter 1 ones look like they should be txid as well).
Pushed an update to use string_view instead.
Not sure. This has no effect on the compiled binary and is only changing the style of the code.
Also unsure. Can you better-explain the changes here/respond to the review comment?
Updated to pass string literals to the Parse{Hash,Hex} functions by value as string_view rather than std::string. Thank you for the helpful feedback!
<details><summary>microbench</summary><p>
// passing-string-literals.cpp
//
// $ g++ -std=c++17 passing-string-literals.cpp && ./a.out
// StringByValue("string literal") took 1426 cycles
// StringByConstRef("string literal") took 1412 cycles
// StringViewByValue("string literal") took 612 cycles
#include <ctime>
#include <iostream>
#include <string>
#include <string_view>
void StringByValue(std::string s) { return; }
void StringByConstRef(const std::string& s) { return; }
void StringViewByValue(std::string_view s) { return; }
int main() {
const int iters = 100000; // try also with `iters` values of, say, 1 or 100
auto start = clock();
for (int it = 0; it < iters; ++it) {
StringByValue("blockhash");
}
std::cout << "StringByValue(\"string literal\") took " << (clock() - start) << " cycles" << std::endl;
start = clock();
for (int it = 0; it < iters; ++it) {
StringByConstRef("blockhash");
}
std::cout << "StringByConstRef(\"string literal\") took " << (clock() - start) << " cycles" << std::endl;
start = clock();
for (int it = 0; it < iters; ++it) {
StringViewByValue("blockhash");
}
std::cout << "StringViewByValue(\"string literal\") took " << (clock() - start) << " cycles" << std::endl;
return 0;
}
</p></details>
73 | @@ -73,29 +74,29 @@ CAmount AmountFromValue(const UniValue& value, int decimals) 74 | return amount; 75 | } 76 | 77 | -uint256 ParseHashV(const UniValue& v, std::string strName) 78 | +uint256 ParseHashV(const UniValue& v, std::string_view strName)
uint256 ParseHashV(const UniValue& v, std::string_view name)
nit, while touching all lines where this is used
done
89 | +uint256 ParseHashO(const UniValue& o, std::string_view strKey) 90 | { 91 | return ParseHashV(o.find_value(strKey), strKey); 92 | } 93 | -std::vector<unsigned char> ParseHexV(const UniValue& v, std::string strName) 94 | +std::vector<unsigned char> ParseHexV(const UniValue& v, std::string_view strName)
same
done
string_view is dangerous, because it can lead to use-after-free, but here it seems fine. lgtm.
I think you can remove the 4 links to random blog posts, because everyone is able to type std::string_view into a search engine, if they want to. Also, I don't think the benchmark is useful in this context and can be removed. If we cared about an RPC call taking a few femtoseconds less, there are much lower hanging fruits to pick. A benchmark would be useful if this showed an end-to-end improvement, but it seems more likely that the compiler already optimized all of this down to the same number of cycles when the functions are called in a complex RPC method.
Updated the pull description and repushed to take @MarcoFalke's review feedback (thanks!)
lgtm ACK b94581a8acecafe5ffff15692485a5572d1db57c
as string_view is optimized to be trivially copiable, and in these use cases we
only perform read operations on the passed object.
These utility methods are called by quite a few RPCs and tests, as well as by each other.
$ git grep "ParseHashV\|ParseHashO\|ParseHexV\|ParseHexO" | wc -l
61
lgtm ACK bb91131d545d986aab81c4bb13676c4520169259
74 | @@ -74,29 +75,29 @@ CAmount AmountFromValue(const UniValue& value, int decimals) 75 | return amount; 76 | } 77 | 78 | -uint256 ParseHashV(const UniValue& v, std::string strName) 79 | +uint256 ParseHashV(const UniValue& v, std::string_view name)
What is the difference of changing std::string to std::string_view in comparision of changing it to const std::string& name? The overhead of a reference to the entire object?
const std::string& requires that the referred-to memory exists inside a std::string in the memory somewhere.
std::string_view does not have this requirement, because it allows to refer to anything char-span-like.
Cool, thanks!
@brunoerg If relevant to your question, check out the microbench results above in #28172 (comment).
// StringByValue("string literal") took 1426 cycles
// StringByConstRef("string literal") took 1412 cycles
// StringViewByValue("string literal") took 612 cycles
crACK bb91131d545d986aab81c4bb13676c4520169259
Tested locally and reviewed the code changes, looks good.
ACK https://github.com/bitcoin/bitcoin/commit/bb91131d545d986aab81c4bb13676c4520169259
ACK bb91131d545d986aab81c4bb13676c4520169259