-
Notifications
You must be signed in to change notification settings - Fork 38.7k
refactor: use string_view for passing string literals to Parse{Hash,Hex} #28172
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor: use string_view for passing string literals to Parse{Hash,Hex} #28172
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. 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. ConflictsReviewers, 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. |
maflcko
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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? |
a42464e to
811b4de
Compare
811b4de to
be3a44a
Compare
|
Updated to pass string literals to the microbench
// 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;
} |
maflcko
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
be3a44a to
b94581a
Compare
|
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
b94581a to
bb91131
Compare
|
Rebased per |
|
lgtm ACK bb91131 |
brunoerg
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
crACK bb91131
|
Tested locally and reviewed the code changes, looks good. ACK bb91131 |
|
ACK bb91131 |
as
string_viewis optimized to be trivially copiable, whereas the current code creates astd::stringcopy at each call.These utility methods are called by quite a few RPCs and tests, as well as by each other.
Also remove an out-of-date external link.