nexus shattering 3 of 3: Extract nexus-db-model crate from nexus::db::model#1478
Merged
Conversation
davepacheco
reviewed
Jul 25, 2022
davepacheco
left a comment
Collaborator
There was a problem hiding this comment.
What a huge improvement! I've got one minor comment below and also asking if there's a TODO here you meant to address before landing this. Anyway, this is looking great and seems like it'll be a big quality of life improvement.
davepacheco
approved these changes
Jul 25, 2022
4f7e8a6 to
b434e18
Compare
This was referenced Jul 25, 2022
29acee6 to
66f1e5f
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is "nexus shattering" PR 3 of 3 (for now), which splits the database model into its own crate (
nexus-db-model). In addition to the files that were previously undernexus/db/model/, I moveddb/schema.rs,db/ipv6.rs, anddb/saga_types.rsintonexus-db-model. I moved them primarily because other modules underdb/model.rsdepend on them, but I think all three of those arguably fit with the model logically anyway; I'd appreciate it if someone could confirm/deny that.TL;DR: This PR (together with the 2 previous ones) speeds up several cases of both incremental and clean builds by 17-25%, with middling improvements (3-7%) to the remaining cases. At least as of this PR (and arguably before), incremental build times are dominated by linking, and using a (much) faster linker can decrease them by another 50+%. More numbers and analysis below the fold.
d023a6d9)454959e0)molddatastore/mod.rsmodifiedmodel/image.rsmodifieddatastore/mod.rsmodifiedmodel/image.rsmodifiedAs with previous timings, in this table "lib+bin" is
cargo build -p omicron-nexusand "tests" iscargo build -p omicron-nexus --tests(i.e., the build of the tests without running them). "Modified" means the a space was inserted at the top of the file (which can cause more work than justtouch, thanks to issues like rust-lang/rust#47389). Modifyingmodel/image.rson this branch triggers a rebuild of bothnexus-db-modelandnexus, as opposed to modifyingdatastore/mod.rs(which only triggers a rebuild ofnexus, notnexus-db-model).New as of my timings on this branch: I did all the above timings using a 32GiB ramdisk as my
targetdirectory and repeated them 3x to check for variance. Before switching to a ramdisk, I was seeing a lot of wild variance and even outright failure (#461) which appears to have been due to poor filesystem performance. A very full filesystem or an ext4 filesystem on an SSD that has had a lot of churn since the last timefstrimhas been run can have an outsized impact on build times (I've observed as much as 2x the numbers in the table above for successful builds, plus the builds that failed outright).I was very surprised to see the full build including tests speed up by nearly a full minute. Repeating that build on both
mainand this branch (using the stock Linux linker) with--timings, here are screenshots of the end of the build process with the min unit time set to 1.1s and the scale set to 5.main:this branch:
It looks like there are two factors:
nexus-db-modelandomicron-nexuson this branch is roughly the same as the time to buildomicron-nexuson main, but compilingnexus-db-modelis able to start sooner and concurrently with other dependencies ofomicron-nexus. (It's hard to tell from the screenshots, but I also thinkomicron-nexusis able to start slightly beforenexus-db-modelcompletes; perhaps rustc doesn't have to wait for the full codegen of a dependency before it starts building the dependent?)omicron-nexus lib (test)(the final and longest crate built) decreased by over 30 seconds.The second point made me realize this is perhaps not a fair comparison, although it might still be the most useful one. On this branch running
cargo build -p omicron-nexus --testsno longer builds any tests that moved into the shattered crates. If one is working in nexus proper and not touching the shattered crates, that's probably a good thing. But for a more fair comparison, perhapscargo build -p omicron-nexus -p nexus-db-model -p nexus-types -p nexus-defaults --testswould be better? This is only a couple seconds slower (3m57s), again due to the shatterednexus-*test crates (which take a combined ~36s, ignoring parallelization) being able to be built concurrently withomicron-nexusitself: