Skip to content

bench, doc: benchmarking updates and fixups #22292

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.

Sign up for GitHub

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

Jump to bottom
Merged
fanquake merged 3 commits into from
Jul 5, 2021
Merged

bench, doc: benchmarking updates and fixups #22292

fanquake merged 3 commits into from
Jul 5, 2021

Conversation

@jonatack
Copy link
Member

@jonatack jonatack commented Jun 20, 2021 •
edited
Loading

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

jonatack
jonatack commented Jun 20, 2021
View reviewed changes
@kiminuo
Copy link
Contributor

kiminuo commented Jun 21, 2021

Concept ACK

Thanks for improving this.

jarolrod
jarolrod reviewed Jun 21, 2021
View reviewed changes
Copy link
Contributor

@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

@jonatack jonatack force-pushed the benchmarking-updates branch from 3c48b78 to 3f408dd Compare June 22, 2021 11:42
fanquake
fanquake reviewed Jun 24, 2021
View reviewed changes
jonatack added 3 commits June 24, 2021 11:13
@jonatack
bench: bench.h fixes and improvements
10f4ce2
@jonatack
bench: bench_bitcoin.cpp help fixups
84e2d5b 
- remove unneeded strprintf
- consistent punctuation (no EOL periods)
- sort helps by order they are printed (alphabetical order)
@jonatack
doc: update doc/benchmarking.md
d8513fe
@jonatack jonatack force-pushed the benchmarking-updates branch from 3f408dd to d8513fe Compare June 24, 2021 09:16
@jonatack
Copy link
Member Author

jonatack commented Jun 24, 2021

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

@theStack
Copy link
Contributor

theStack commented Jun 24, 2021

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

theStack
theStack approved these changes Jun 27, 2021
View reviewed changes
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
@fanquake
Merge bitcoin#22292: bench, doc: benchmarking updates and fixups
9e48031
hebasto added a commit to hebasto/bitcoin-maintainer-tools that referenced this pull request Aug 11, 2021
@hebasto
Update patches/stripbuildinfo.patch
75b84b4 
This change is required since bitcoin/bitcoin#22292 was merged.
@hebasto hebasto mentioned this pull request Aug 11, 2021
Merged
fanquake added a commit to bitcoin-core/bitcoin-maintainer-tools that referenced this pull request Aug 12, 2021
@fanquake
Merge #108: Update patches/stripbuildinfo.patch
436cd98 
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
@apoelstra
Merge c609e10 into merged_master (Bitcoin PR bitcoin/bitcoin#22292)
1dd57b5
@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.

Reviewers

@fanquake fanquake fanquake left review comments

+2 more reviewers

@jarolrod jarolrod jarolrod left review comments

@theStack theStack theStack approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Docs Tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@jonatack @kiminuo @theStack @za-kk @fanquake @jarolrod @DrahtBot