Optimization: parse manifest only once #2898
Conversation
|
You have reached a similar conclusion to that which led me in #2626 to think about writing #2627. My gut feeling is that it'd be best to rewrite the toml into something which can be parsed more quickly, rather than complicating the internal pathways in rustup regarding parsed toolchain manifests. I will have a read through your change, but I'm sceptical that as-is it'll be the right answer. I am grateful for the drive-by cleanup work though, that idea is excellent. |
There was a problem hiding this comment.
I take back my comment, I think the extra complication of that struct is simple enough that I do not object to it. This looks very worthwhile to include, thank you!
Could you please deal with the one comment I made on the code; and then when you're done, rebase the changes together, since I prefer to not merge "run rustfmt" type patches into the code if I can avoid it.
Thank you again.
src/toolchain.rs
Outdated
| pub(crate) struct ToolchainDescWithManifest { | ||
| toolchain: ToolchainDesc, | ||
| manifestation: Manifestation, | ||
| pub(crate) manifest: Manifest, |
There was a problem hiding this comment.
With the struct being pub(crate) there's no need for the (crate) modifier on this field.
This adds an intermediate struct which includes a parsed manifest, and eliminates a bunch of duplicated code. Only parsing the manifest once speeds up `find_or_install_override_toolchain_or_default` by about 50% which is the critical path for a rebuild with no changes. There's also a drive-by fix for running test on macOS aarch64.
mkeeter
force-pushed
the
parse-manifest-only-once
branch
from
6fba61f
to
9e945ed
Compare
|
Cool – I made that one change, then squashed into a single commit and force-pushed it, so we should be good to go! |
I was building a project which invoked
cargomultiple times, and noticed that a no-op build (i.e. a build with no local changes) was surprisingly slow.Generating a flamegraph of one
cargocall, I noticed the following:About half the work is done in
Manifestation::load_manifest, which is called twice to parse the same 700K TOML file (here and here).This PR adds a new
structintoolchain.rswhich encapsulates this parsed manifest plus other relevant data. Thisstructis then used inconfig.rsto eliminate the double-parsing.I also use this
structto clean up duplicate code intoolchain.rs, e.g. this comment:I see a 15-25% speedup in a no-op build, depending on the project!
clippylints (there are a few pre-existing lints in areas I haven't touched)rustfmtrun