-
Notifications
You must be signed in to change notification settings - Fork 37.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. |
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;
} |
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 b94581a |
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 |
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_view
is optimized to be trivially copiable, whereas the current code creates astd::string
copy 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.