Skip to content

Conversation

jonatack
Copy link
Member

@jonatack jonatack commented Jul 27, 2023

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.

@DrahtBot
Copy link
Contributor

DrahtBot commented Jul 27, 2023

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK maflcko, brunoerg, ns-xvrn, achow101

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #28134 (refactor: deduplicate AmountFromValue() functions by jonatack)

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.

Copy link
Member

@maflcko maflcko left a 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.

@fanquake
Copy link
Member

fanquake commented Aug 1, 2023

Also unsure. Can you better-explain the changes here/respond to the review comment?

@jonatack jonatack force-pushed the 2023-07-avoid-string-copies-in-ParseHex-functions branch from a42464e to 811b4de Compare August 2, 2023 16:25
@jonatack jonatack changed the title rpc, util: avoid string copies in ParseHash/ParseHex functions rpc, util: use string_view for passing string literals to Parse{Hash,Hex} Aug 2, 2023
@DrahtBot DrahtBot changed the title rpc, util: use string_view for passing string literals to Parse{Hash,Hex} rpc, util: use string_view for passing string literals to Parse{Hash,Hex} Aug 2, 2023
@jonatack jonatack force-pushed the 2023-07-avoid-string-copies-in-ParseHex-functions branch from 811b4de to be3a44a Compare August 2, 2023 16:48
@jonatack
Copy link
Member Author

jonatack commented Aug 2, 2023

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!

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;
}

Copy link
Member

@maflcko maflcko left a 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.

@DrahtBot DrahtBot changed the title rpc, util: use string_view for passing string literals to Parse{Hash,Hex} refactor: use string_view for passing string literals to Parse{Hash,Hex} Aug 3, 2023
@jonatack jonatack force-pushed the 2023-07-avoid-string-copies-in-ParseHex-functions branch from be3a44a to b94581a Compare August 3, 2023 18:41
@jonatack
Copy link
Member Author

jonatack commented Aug 3, 2023

Updated the pull description and repushed to take @MarcoFalke's review feedback (thanks!)

@bitcoin bitcoin deleted a comment from Malk8628 Aug 4, 2023
@maflcko
Copy link
Member

maflcko commented Aug 4, 2023

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
@jonatack jonatack force-pushed the 2023-07-avoid-string-copies-in-ParseHex-functions branch from b94581a to bb91131 Compare August 25, 2023 22:43
@jonatack
Copy link
Member Author

Rebased per git range-diff c9273f6 b94581a bb91131 for an include header from the merge of #28230. Should be trivial to re-ACK.

@jonatack jonatack requested a review from maflcko August 28, 2023 13:51
@maflcko
Copy link
Member

maflcko commented Aug 28, 2023

lgtm ACK bb91131

@DrahtBot DrahtBot removed the request for review from maflcko August 28, 2023 14:00
Copy link
Contributor

@brunoerg brunoerg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

crACK bb91131

@nsvrn
Copy link
Contributor

nsvrn commented Oct 13, 2023

Tested locally and reviewed the code changes, looks good.

ACK bb91131

@achow101
Copy link
Member

achow101 commented Nov 2, 2023

ACK bb91131

@achow101 achow101 merged commit 9b68c9b into bitcoin:master Nov 2, 2023
@jonatack jonatack deleted the 2023-07-avoid-string-copies-in-ParseHex-functions branch November 2, 2023 21:16
@bitcoin bitcoin locked and limited conversation to collaborators Nov 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants