perf(pm): thread warm project cache into resolver config#3042
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a warm_project_cache field to the BuildDepsConfig struct and its builder, allowing the resolver to be seeded with host-provided manifest data. Feedback focuses on performance and architecture: it is recommended to wrap the cache in an Arc to avoid expensive clones and to move the ProjectCacheData definition to a lower-level module to prevent circular dependencies between the service and resolver layers.
| use crate::resolver::preload::{PreloadConfig, preload_manifests}; | ||
| use crate::resolver::registry::{ResolveError, resolve_registry_dep}; | ||
| use crate::service::ManifestProvider; | ||
| use crate::service::{ManifestProvider, ProjectCacheData}; |
There was a problem hiding this comment.
Importing ProjectCacheData from crate::service into the resolver module further entrenches a circular dependency, as the service layer already depends on the resolver. While technically allowed within a single crate in Rust, this violates clean architectural layering where core logic (resolver) should ideally not depend on higher-level service abstractions. Consider moving ProjectCacheData to a lower-level module like model or traits to maintain a unidirectional dependency flow.
| /// Key `""` = default catalog, other keys = named catalogs. | ||
| pub catalogs: Catalogs, | ||
| /// Host-provided project cache used to seed resolver manifest state. | ||
| pub warm_project_cache: Option<ProjectCacheData>, |
There was a problem hiding this comment.
The BuildDepsConfig struct derives Clone, but warm_project_cache is an owned Option<ProjectCacheData>. Since this cache can contain a large number of manifests, cloning the configuration will result in expensive deep copies. Consider wrapping this field in an Arc to ensure that cloning the config remains a cheap operation, consistent with how git_clone_cache and http_fetch_cache are implemented.
| pub warm_project_cache: Option<ProjectCacheData>, | |
| pub warm_project_cache: Option<Arc<ProjectCacheData>>, |
| } | ||
|
|
||
| /// Set the host-provided warm project cache. | ||
| pub fn with_warm_project_cache(mut self, warm_project_cache: Option<ProjectCacheData>) -> Self { |
There was a problem hiding this comment.
Update the builder method signature to accept an Arc to match the suggested change in the struct field.
| pub fn with_warm_project_cache(mut self, warm_project_cache: Option<ProjectCacheData>) -> Self { | |
| pub fn with_warm_project_cache(mut self, warm_project_cache: Option<Arc<ProjectCacheData>>) -> Self { |
| .with_skip_preload(skip_preload) | ||
| .with_catalogs(catalogs); | ||
| .with_catalogs(catalogs) | ||
| .with_warm_project_cache(warm_project_cache); |
53b2356 to
c1d84fd
Compare
845d350 to
82721ee
Compare
c1d84fd to
17611a9
Compare
17611a9 to
32ef04c
Compare
82721ee to
0520cea
Compare
📊 pm-bench-phases ·
|
| PM | wall | ±σ | user | sys | RSS | pgMinor |
|---|---|---|---|---|---|---|
| bun | 8.99s | 0.16s | 10.16s | 9.66s | 763M | 326.7K |
| utoo-next | 7.89s | 0.23s | 10.00s | 11.62s | 949M | 118.5K |
| utoo-npm | 7.93s | 0.14s | 10.40s | 11.79s | 983M | 125.8K |
| utoo | 8.19s | 0.22s | 10.50s | 11.72s | 993M | 117.4K |
| PM | vCtx | iCtx | netRX | netTX | cache | node_mod | lock |
|---|---|---|---|---|---|---|---|
| bun | 15.2K | 17.9K | 1.19G | 6M | 1.86G | 1.75G | 1M |
| utoo-next | 111.5K | 78.0K | 1.16G | 5M | 1.71G | 1.70G | 2M |
| utoo-npm | 124.9K | 88.6K | 1.16G | 5M | 1.71G | 1.70G | 2M |
| utoo | 123.5K | 67.6K | 1.17G | 6M | 1.71G | 1.70G | 2M |
p1_resolve
| PM | wall | ±σ | user | sys | RSS | pgMinor |
|---|---|---|---|---|---|---|
| bun | 1.96s | 0.10s | 3.92s | 1.08s | 520M | 177.5K |
| utoo-next | 2.91s | 0.30s | 4.92s | 1.73s | 616M | 79.5K |
| utoo-npm | 3.18s | 0.23s | 5.07s | 2.08s | 609M | 86.0K |
| utoo | 3.12s | 0.09s | 5.42s | 1.89s | 624M | 89.6K |
| PM | vCtx | iCtx | netRX | netTX | cache | node_mod | lock |
|---|---|---|---|---|---|---|---|
| bun | 8.3K | 4.7K | 202M | 3M | 107M | - | 1M |
| utoo-next | 49.1K | 67.4K | 200M | 2M | 7M | 3M | 2M |
| utoo-npm | 72.3K | 87.2K | 200M | 2M | 7M | 3M | 2M |
| utoo | 54.6K | 73.0K | 202M | 3M | 7M | 3M | 2M |
p3_cold_install
| PM | wall | ±σ | user | sys | RSS | pgMinor |
|---|---|---|---|---|---|---|
| bun | 6.66s | 0.26s | 6.35s | 9.40s | 623M | 209.8K |
| utoo-next | 5.77s | 0.04s | 4.94s | 10.17s | 523M | 64.3K |
| utoo-npm | 7.10s | 2.52s | 5.10s | 10.35s | 474M | 58.5K |
| utoo | 6.53s | 2.17s | 4.96s | 10.25s | 483M | 62.4K |
| PM | vCtx | iCtx | netRX | netTX | cache | node_mod | lock |
|---|---|---|---|---|---|---|---|
| bun | 5.0K | 7.0K | 1019M | 4M | 1.76G | 1.76G | 1M |
| utoo-next | 102.6K | 46.1K | 989M | 3M | 1.70G | 1.70G | 2M |
| utoo-npm | 116.6K | 49.4K | 990M | 3M | 1.70G | 1.70G | 2M |
| utoo | 104.7K | 53.8K | 989M | 3M | 1.70G | 1.70G | 2M |
p4_warm_link
| PM | wall | ±σ | user | sys | RSS | pgMinor |
|---|---|---|---|---|---|---|
| bun | 3.34s | 0.02s | 0.19s | 2.38s | 135M | 32.3K |
| utoo-next | 2.41s | 0.09s | 0.46s | 3.72s | 80M | 18.3K |
| utoo-npm | 2.18s | 0.16s | 0.46s | 3.70s | 79M | 17.9K |
| utoo | 2.13s | 0.04s | 0.47s | 3.68s | 80M | 18.6K |
| PM | vCtx | iCtx | netRX | netTX | cache | node_mod | lock |
|---|---|---|---|---|---|---|---|
| bun | 259 | 22 | 5M | 26K | 1.91G | 1.73G | 1M |
| utoo-next | 40.2K | 18.2K | 24K | 9K | 1.70G | 1.70G | 2M |
| utoo-npm | 40.2K | 19.1K | 24K | 21K | 1.70G | 1.70G | 2M |
| utoo | 42.3K | 20.2K | 25K | 24K | 1.71G | 1.70G | 2M |
npmmirror.com: no output captured.
Summary
warm_project_cachetoBuildDepsConfigValidation
cargo fmtcargo check -p utoo-ruboristcargo test -p utoo-ruborist resolver::builder::tests::test_build -- --nocapturecargo test -p utoo-ruborist service::api::tests::test_build_deps_options_creation -- --nocapturecargo clippy --all-targets -- -D warnings --no-depsSplit Plan
Part of the resolver stack split from source PR #3028. This prepares the warm-cache data flow before the mainloop starts owning resolver-local manifest state.