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

[meta] a few changes to prevent duplicate dep builds #4535

Conversation

sunshowers
Copy link
Contributor

@sunshowers sunshowers commented Nov 21, 2023

This PR has a few changes that make builds and test runs significantly faster:

  1. Remove xtask from the list of default-members. This makes it so that cargo nextest run and cargo nextest run -p <package> use more dependency feature sets in common.
  2. Move opt-level settings from profile.test to profile.dev. Again, this results in more cache hits.
  3. Set profile.dev.panic to unwind. This is to unify build units across dev and test builds: tests are always built with panic = "unwind" so that proper backtraces can be printed out. Release builds stay as abort.
  4. For a belt-and-suspenders approach, make the crdb-seed script use the test profile. If there are any divergences between dev and test in the future, then crdb-seed should share its build cache with the tests it was presumably invoked for.
  5. Set profile.dev.build-override.debug to line-tables-only. This, along with 3, means that target (normal/dev) and build (host) dependencies are now unified.

All of this comes together for a pretty sweet improvement.

See #4392 for more details and how I investigated this issue.

Impact

With a fresh build on Linux with mold, I ran three commands in sequence:

  1. cargo nextest run --no-run
  2. cargo nextest run -p nexus-db-queries
  3. cargo build -p omicron-nexus

The results were:

command phase before before, cumul. after after, cumul.
cargo nextest run build 173s 173s 158s 158s
cargo nextest run -p nexus-db-queries build 61s 234s 51s 209s
cargo nextest run -p nexus-db-queries crdb-seed build 21s 255s 1s 210s
cargo build -p omicron-nexus build 99s 354s 69s 279s

So the cumulative time spent on these three commands went from 354s to 279s. That's a 1.26x speedup. And this should also make other commands better as well (omicron-nexus is a bit of a weird case because it takes a very long time to compile by itself, and that 69s in the "after" column is entirely building omicron-nexus).

Copy link
Collaborator

@smklein smklein left a comment

Choose a reason for hiding this comment

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

Thanks for doing this investigation and adjustment -- this is a significant boost, happy to see us re-use portions of the build where we can / should!

Cargo.toml Show resolved Hide resolved
Cargo.toml Show resolved Hide resolved
@rcgoodfellow
Copy link
Contributor

Unfortunately, this does not appear to move the needle on illumos.

command before full-compile after full-compile before incremental-recompile after incremental recompile
cargo nextest run --no-run 986s 985s 646s 664s

Incremental compile time measured by adding a comment in the file neuxs/src/app/sagas/switch_port_settings_apply.rs and re-running cargo nextest run --no-run

More details

System

  • AMD Ryzen Threadripper 3990x
  • 256GB Memory
  • Samsung 970 Evo NVMe 2Tb
  • helios-2.0.22298

Before full-compile

real    16:25.599274380
user  1:59:36.518864609
sys     19:15.753319431

Before incremental recompile

real    10:45.830306626
user    29:00.931336328
sys      1:31.905661961

After full-compile

real    16:25.070892103
user  1:48:28.561607858
sys     13:22.269607043

After incremental recompile

real    11:03.987974145
user    40:35.817986189
sys      1:21.120225119

@sunshowers
Copy link
Contributor Author

sunshowers commented Nov 21, 2023

Discussed this on #oxide-control-plane in Matrix -- unfortunately the listed case in Ry's comment would not benefit (nor be affected negatively) by this change. Where an improvement would occur is with e.g. cargo nextest run followed by cargo nextest run -p omicron-sled-agent.

@sunshowers sunshowers merged commit b07a8f5 into main Nov 21, 2023
22 checks passed
@sunshowers sunshowers deleted the sunshowers/spr/meta-a-few-changes-to-prevent-duplicate-dep-builds branch November 21, 2023 22:59
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