Skip to content
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

bench, doc: benchmarking updates and fixups #22292

Merged
merged 3 commits into from
Jul 5, 2021

Conversation

jonatack
Copy link
Member

@jonatack jonatack commented Jun 20, 2021

Fixups and updates I noticed while writing benchmarks for #22284.

@kiminuo
Copy link
Contributor

kiminuo commented Jun 21, 2021

Concept ACK

Thanks for improving this.

Copy link
Member

@jarolrod jarolrod left a comment

Choose a reason for hiding this comment

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

Concept ACK, is obviously an improvement to the docs

src/bench/bench.h Show resolved Hide resolved
doc/benchmarking.md Outdated Show resolved Hide resolved
src/bench/bench.h Show resolved Hide resolved
- remove unneeded strprintf
- consistent punctuation (no EOL periods)
- sort helps by order they are printed (alphabetical order)
@jonatack
Copy link
Member Author

Thanks @fanquake, @jarolrod and @kiminuo for the reviews. Updated per feedback and fixed the last commit message (bencharking -> benchmarking).

@theStack
Copy link
Contributor

Concept ACK, nice benchmark doc improvements!
Planning to verify the changes more in detail within the next days.

Copy link
Contributor

@theStack theStack left a comment

Choose a reason for hiding this comment

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

ACK d8513fe 🚤

Reviewed the documentation changes, ran ./src/bench/bench_bitcoin -filter="AddrManAdd|AddrManGetAddr|Base58CheckEncode|Base58Decode" to verify that the example output format in the last commit matches.

@za-kk
Copy link
Contributor

za-kk commented Jul 4, 2021

ACK d8513fe

Reviewed documentation and it's definitely an improvement to what was already there, also ran ./src/bench/bench_bitcoin -filter="AddrManAdd|AddrManGetAddr|Base58CheckEncode|Base58Decode" and verified format matches with documentation

Note - running ./src/bench/bench_bitcoin -? causes an error in zsh as ? is considered a wildcard, instead ./src/bench/bench_bitcoin "-?" must be run. Now that we are specifying -? in the docs instead of --help could cause confusion when using zsh (which is default on macos). But I'm not sure whether the intricacies of different shells should impact the documentation 🤷

@fanquake fanquake merged commit c609e10 into bitcoin:master Jul 5, 2021
@jonatack jonatack deleted the benchmarking-updates branch July 5, 2021 04:52
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jul 5, 2021
hebasto added a commit to hebasto/bitcoin-maintainer-tools that referenced this pull request Aug 11, 2021
This change is required since bitcoin/bitcoin#22292 was merged.
fanquake added a commit to bitcoin-core/bitcoin-maintainer-tools that referenced this pull request Aug 12, 2021
75b84b4 Update patches/stripbuildinfo.patch (Hennadii Stepanov)

Pull request description:

  This change is required since bitcoin/bitcoin#22292 was merged.

ACKs for top commit:
  fanquake:
    Tested ACK 75b84b4

Tree-SHA512: d318a802c8c27574c04b7bec339ca41e7dd8648e34a5c9e57ec6a3234a92606dc86df9e09e280a0b3bb0e537650b14f3c69a80c36b7601b5e07766a19e930a9f
gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 1, 2022
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022
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.

None yet

7 participants