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

Introduce an example binary useful for profiling #646

Merged
merged 6 commits into from
Mar 4, 2024

Conversation

RCasatta
Copy link
Contributor

@RCasatta RCasatta commented Feb 27, 2024

Tools like cargo bloat don't work on libraries but on final executable.

As seen in #585 and #584 the parse example is used as a base to profile binary bloat.

However, since the parse example is not using all the API surface, we may have good optimization that are not recognized as such because the optimized function are not in the tree of the functions used.

For benchmarking size optimization a specific binary that ideally touch all the API surface is needed and this MR introduce it.

More coverage will be added if this seem a good idea for reviewers.

@tcharding
Copy link
Member

Cool idea, should we be doing this for our other crates (specifically as we crate smash them out of rust-bitcoin e.g., hex, units, primitives)?

@apoelstra
Copy link
Member

We may want a maintainer-tools repo in the org, similar to bitcoin-core/bitcoin-maintainer-tools, where we could put stuff like this. I'd also like to try unifying our test.sh scripts, API checking scripts, etc etc, there.

@RCasatta
Copy link
Contributor Author

Certainly it is useful in other crates too, but at the moment in particular this crate and its companion elements-miniscript have high percentage of binary size consumption in downstream libraries due to the high use of generics so it makes the most sense here.

@RCasatta
Copy link
Contributor Author

RCasatta commented Feb 28, 2024

We may want a maintainer-tools repo in the org

Why do you think this is not meant to be in this repo? Thanks to the required-features in the Cargo.toml we can add a specific "big" feature so it would be compiled only with --examples and all the features including "big" or --all-features flag, minimizing the impact. (I don't even see the usage of the flag --all-features in the test script)

@apoelstra
Copy link
Member

Sorry, I didn't actually look at the diff (because I am waiting for #645 to get in so that CI will work). I didn't realize it was actually an example in the examples/ directory. I assumed it was a whole separate crate, similar to our embedded tests in some other repos.

Yes, this should certainly stay in this repo.

My comments about wanting a maintainer-tools repo are separate -- we should have this anyway so we can deduplicate our test scripts and write "end to end" tests of the whole ecosystem.

@RCasatta
Copy link
Contributor Author

RCasatta commented Mar 3, 2024

Rebased and added some calls:

in debug big executable is 72Mb and the second biggest example is 51Mb (41% bigger)
in release big executable is 9Mb and the second biggest example is 7.3Mb (23% bigger)

examples/big.rs Outdated
@@ -0,0 +1,40 @@
// SPDX-License-Identifier: CC0-1.0
//! This is not an example and will surely panic if executed, the purpose of this is using the
Copy link
Member

Choose a reason for hiding this comment

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

In ec751fb:

trailing space after the

Copy link
Member

Choose a reason for hiding this comment

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

Ah, the formatter got this in the last commit.

Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK c8d3b9a thanks!

@apoelstra
Copy link
Member

Let's get this in. It's a super useful benchmark. I am planning to give "redo the error types" a shot in the coming week and hopefully I can get this size down.

@apoelstra apoelstra merged commit 551932e into rust-bitcoin:master Mar 4, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants