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

Try optimizing only math-heavy dependencies in the test profile #1606

Closed
daira opened this issue Nov 2, 2024 · 2 comments
Closed

Try optimizing only math-heavy dependencies in the test profile #1606

daira opened this issue Nov 2, 2024 · 2 comments

Comments

@daira
Copy link
Contributor

daira commented Nov 2, 2024

Originally posted by @str4d in #1443 (comment):

We should experiment with Cargo profile overrides to just optimize the math-heavy dependencies [when building for tests]: https://docs.rust-embedded.org/book/unsorted/speed-vs-size.html#optimizing-dependencies

#1603 included the same change as #1443, changing the test profile to enable both optimization (at level 3, but without LTO) and debug info. This improved build times substantially relative to cargo test --release, without any significant regression in test times, and with an improvement in debuggability. However, we might be able to improve build times even further by following @str4d's suggestion.

@daira
Copy link
Contributor Author

daira commented Nov 2, 2024

I cannot find any combination of options using profile overrides that beats what is currently on main for build+test after a clean (which is effectively the situation in CI). The sweet spots seem to be "everything at opt-level = 3" and "everything at opt-level = 2".

If we try to compile most things at any opt-level lower than 2, the test time balloons enormously. If we try to compile almost everything at opt-level 3 and just the top-level crates in the librustzcash workspace at any lower level (even 2), there is no improvement in build time. The issue with the latter is that cargo/rustc seem to build more efficiently when all crates are using the same options, so you actually lose on build time even when compiling some crates at a lower opt-level. It could be that profile overrides are introducing some specific build performance suboptimality. There is also the issue (as documented in the Cargo book) that the effective optimization level for generic code is determined by the crate that instantiates it, and that limits the scope to use lower opt-levels for the crates in this workspace.

The best option overall seemed to be "everything at opt-level = 2" (456 seconds on my machine, vs 484 seconds for "everything at opt-level = 3"), so I'll file a PR to change it from 3 to 2 in the test profile. I would expect this also to be an improvement when running fewer tests. There could be a slight regression when running all or most of the tests after a small change, but only by a few seconds, and there might be a small compensating improvement in debuggability. (It didn't affect the wallet::transparent::tests::transparent_balance_spendability test case I was using, though.)

@daira
Copy link
Contributor Author

daira commented Nov 6, 2024

As I said in #1607:

There is an improvement to build+test latency on CI, but it's small:

  • "Test on Linux" took 5m 53s with this PR, vs 6m 2s before (a 2.5%, or 9 second improvement)
  • "Test Orchard on Linux" took 7m 23s with this PR, vs 7m 39s before (a 3.5%, or 16 second improvement)

The disadvantage is that testing would not see miscompilations that happen at opt-level 3 but not opt-level 2. On reflection, I think this is a significant downside. Testing at the same optimization level as the code we release (even without LTO, which admittedly could be a source of miscompilations) seems like a good thing to maintain. So I'm inclined to close this without merging.

@daira daira closed this as completed Nov 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant