Skip to content

Commit 4f5c737

Browse files
committed
Add more profiling traces
Someone suggested I look at [zed](https://github.com/zed-industries/zed). It has some interesting performance characteristics compared to projects I looked at before: - A lot of workspace members - git dependencies - patches This adds traces that help provide more context for those scenarios when looking at them. Some of these areas might be of interest for optimizing: - On every invocation, we do a git2 `copy_to` call for each git checkout - On every invocation, we do a `du` on each git checkout - It'd be great to avoid parsing every manifest in a git checkout but we need to do a full parse to make sure we find all packages (see https://rust-lang.zulipchat.com/#narrow/stream/246057-t-cargo/topic/Redundant.20code.20in.20.60GitSouce.60.3F) - Its suspicious how much time we spend in the "poison" step of resolving when its a no-op
1 parent 0d67af0 commit 4f5c737

File tree

6 files changed

+106
-83
lines changed

6 files changed

+106
-83
lines changed

src/cargo/core/global_cache_tracker.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1799,6 +1799,7 @@ pub fn is_silent_error(e: &anyhow::Error) -> bool {
17991799
}
18001800

18011801
/// Returns the disk usage for a git checkout directory.
1802+
#[tracing::instrument]
18021803
pub fn du_git_checkout(path: &Path) -> CargoResult<u64> {
18031804
// !.git is used because clones typically use hardlinks for the git
18041805
// contents. TODO: Verify behavior on Windows.

src/cargo/core/registry.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -336,6 +336,7 @@ impl<'gctx> PackageRegistry<'gctx> {
336336
/// The return value is a `Vec` of patches that should *not* be locked.
337337
/// This happens when the patch is locked, but the patch has been updated
338338
/// so the locked value is no longer correct.
339+
#[tracing::instrument(skip(self, patch_deps))]
339340
pub fn patch(
340341
&mut self,
341342
url: &Url,
@@ -784,6 +785,7 @@ impl<'gctx> Registry for PackageRegistry<'gctx> {
784785
}
785786
}
786787

788+
#[tracing::instrument(skip_all)]
787789
fn block_until_ready(&mut self) -> CargoResult<()> {
788790
if cfg!(debug_assertions) {
789791
// Force borrow to catch invalid borrows, regardless of which source is used and how it

src/cargo/core/workspace.rs

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -723,6 +723,7 @@ impl<'gctx> Workspace<'gctx> {
723723
/// verifies that those are all valid packages to point to. Otherwise, this
724724
/// will transitively follow all `path` dependencies looking for members of
725725
/// the workspace.
726+
#[tracing::instrument(skip_all)]
726727
fn find_members(&mut self) -> CargoResult<()> {
727728
let Some(workspace_config) = self.load_workspace_config()? else {
728729
debug!("find_members - only me as a member");
@@ -886,6 +887,7 @@ impl<'gctx> Workspace<'gctx> {
886887
/// 1. A workspace only has one root.
887888
/// 2. All workspace members agree on this one root as the root.
888889
/// 3. The current crate is a member of this workspace.
890+
#[tracing::instrument(skip_all)]
889891
fn validate(&mut self) -> CargoResult<()> {
890892
// The rest of the checks require a VirtualManifest or multiple members.
891893
if self.root_manifest.is_none() {
@@ -954,6 +956,7 @@ impl<'gctx> Workspace<'gctx> {
954956
}
955957
}
956958

959+
#[tracing::instrument(skip_all)]
957960
fn validate_members(&mut self) -> CargoResult<()> {
958961
for member in self.members.clone() {
959962
let root = self.find_root(&member)?;
@@ -1812,6 +1815,7 @@ impl WorkspaceRootConfig {
18121815
self.members.is_some()
18131816
}
18141817

1818+
#[tracing::instrument(skip_all)]
18151819
fn members_paths(&self, globs: &[String]) -> CargoResult<Vec<PathBuf>> {
18161820
let mut expanded_list = Vec::new();
18171821

src/cargo/ops/cargo_read_manifest.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@ pub fn read_package(
3434
Ok(Package::new(manifest, path))
3535
}
3636

37+
#[tracing::instrument(skip(source_id, gctx))]
3738
pub fn read_packages(
3839
path: &Path,
3940
source_id: SourceId,

src/cargo/ops/resolve.rs

Lines changed: 97 additions & 83 deletions
Original file line numberDiff line numberDiff line change
@@ -386,6 +386,8 @@ pub fn resolve_with_previous<'gctx>(
386386
register_previous_locks(ws, registry, r, &keep, dev_deps);
387387

388388
// Prefer to use anything in the previous lock file, aka we want to have conservative updates.
389+
let span = tracing::span!(tracing::Level::TRACE, "prefer_package_id");
390+
let _enter = span.enter();
389391
for id in r.iter().filter(keep) {
390392
debug!("attempting to prefer {}", id);
391393
version_prefs.prefer_package_id(id);
@@ -396,20 +398,23 @@ pub fn resolve_with_previous<'gctx>(
396398
registry.lock_patches();
397399
}
398400

399-
let summaries: Vec<(Summary, ResolveOpts)> = ws
400-
.members_with_features(specs, cli_features)?
401-
.into_iter()
402-
.map(|(member, features)| {
403-
let summary = registry.lock(member.summary().clone());
404-
(
405-
summary,
406-
ResolveOpts {
407-
dev_deps,
408-
features: RequestedFeatures::CliFeatures(features),
409-
},
410-
)
411-
})
412-
.collect();
401+
let summaries: Vec<(Summary, ResolveOpts)> = {
402+
let span = tracing::span!(tracing::Level::TRACE, "registry.lock");
403+
let _enter = span.enter();
404+
ws.members_with_features(specs, cli_features)?
405+
.into_iter()
406+
.map(|(member, features)| {
407+
let summary = registry.lock(member.summary().clone());
408+
(
409+
summary,
410+
ResolveOpts {
411+
dev_deps,
412+
features: RequestedFeatures::CliFeatures(features),
413+
},
414+
)
415+
})
416+
.collect()
417+
};
413418

414419
let replace = lock_replacements(ws, previous, &keep);
415420

@@ -497,6 +502,7 @@ pub fn get_resolved_packages<'gctx>(
497502
///
498503
/// Note that this function, at the time of this writing, is basically the
499504
/// entire fix for issue #4127.
505+
#[tracing::instrument(skip_all)]
500506
fn register_previous_locks(
501507
ws: &Workspace<'_>,
502508
registry: &mut PackageRegistry<'_>,
@@ -577,60 +583,64 @@ fn register_previous_locks(
577583
// crates from crates.io* are not locked (aka added to `avoid_locking`).
578584
// For dependencies like `log` their previous version in the lock file will
579585
// come up first before newer version, if newer version are available.
580-
let mut path_deps = ws.members().cloned().collect::<Vec<_>>();
581-
let mut visited = HashSet::new();
582-
while let Some(member) = path_deps.pop() {
583-
if !visited.insert(member.package_id()) {
584-
continue;
585-
}
586-
let is_ws_member = ws.is_member(&member);
587-
for dep in member.dependencies() {
588-
// If this dependency didn't match anything special then we may want
589-
// to poison the source as it may have been added. If this path
590-
// dependencies is **not** a workspace member, however, and it's an
591-
// optional/non-transitive dependency then it won't be necessarily
592-
// be in our lock file. If this shows up then we avoid poisoning
593-
// this source as otherwise we'd repeatedly update the registry.
594-
//
595-
// TODO: this breaks adding an optional dependency in a
596-
// non-workspace member and then simultaneously editing the
597-
// dependency on that crate to enable the feature. For now,
598-
// this bug is better than the always-updating registry though.
599-
if !is_ws_member && (dep.is_optional() || !dep.is_transitive()) {
586+
{
587+
let span = tracing::span!(tracing::Level::TRACE, "poison");
588+
let _enter = span.enter();
589+
let mut path_deps = ws.members().cloned().collect::<Vec<_>>();
590+
let mut visited = HashSet::new();
591+
while let Some(member) = path_deps.pop() {
592+
if !visited.insert(member.package_id()) {
600593
continue;
601594
}
595+
let is_ws_member = ws.is_member(&member);
596+
for dep in member.dependencies() {
597+
// If this dependency didn't match anything special then we may want
598+
// to poison the source as it may have been added. If this path
599+
// dependencies is **not** a workspace member, however, and it's an
600+
// optional/non-transitive dependency then it won't be necessarily
601+
// be in our lock file. If this shows up then we avoid poisoning
602+
// this source as otherwise we'd repeatedly update the registry.
603+
//
604+
// TODO: this breaks adding an optional dependency in a
605+
// non-workspace member and then simultaneously editing the
606+
// dependency on that crate to enable the feature. For now,
607+
// this bug is better than the always-updating registry though.
608+
if !is_ws_member && (dep.is_optional() || !dep.is_transitive()) {
609+
continue;
610+
}
602611

603-
// If dev-dependencies aren't being resolved, skip them.
604-
if !dep.is_transitive() && !dev_deps {
605-
continue;
606-
}
612+
// If dev-dependencies aren't being resolved, skip them.
613+
if !dep.is_transitive() && !dev_deps {
614+
continue;
615+
}
607616

608-
// If this is a path dependency, then try to push it onto our
609-
// worklist.
610-
if let Some(pkg) = path_pkg(dep.source_id()) {
611-
path_deps.push(pkg);
612-
continue;
613-
}
617+
// If this is a path dependency, then try to push it onto our
618+
// worklist.
619+
if let Some(pkg) = path_pkg(dep.source_id()) {
620+
path_deps.push(pkg);
621+
continue;
622+
}
614623

615-
// If we match *anything* in the dependency graph then we consider
616-
// ourselves all ok, and assume that we'll resolve to that.
617-
if resolve.iter().any(|id| dep.matches_ignoring_source(id)) {
618-
continue;
619-
}
624+
// If we match *anything* in the dependency graph then we consider
625+
// ourselves all ok, and assume that we'll resolve to that.
626+
if resolve.iter().any(|id| dep.matches_ignoring_source(id)) {
627+
continue;
628+
}
620629

621-
// Ok if nothing matches, then we poison the source of these
622-
// dependencies and the previous lock file.
623-
debug!(
624-
"poisoning {} because {} looks like it changed {}",
625-
dep.source_id(),
626-
member.package_id(),
627-
dep.package_name()
628-
);
629-
for id in resolve
630-
.iter()
631-
.filter(|id| id.source_id() == dep.source_id())
632-
{
633-
add_deps(resolve, id, &mut avoid_locking);
630+
// Ok if nothing matches, then we poison the source of these
631+
// dependencies and the previous lock file.
632+
debug!(
633+
"poisoning {} because {} looks like it changed {}",
634+
dep.source_id(),
635+
member.package_id(),
636+
dep.package_name()
637+
);
638+
for id in resolve
639+
.iter()
640+
.filter(|id| id.source_id() == dep.source_id())
641+
{
642+
add_deps(resolve, id, &mut avoid_locking);
643+
}
634644
}
635645
}
636646
}
@@ -660,28 +670,32 @@ fn register_previous_locks(
660670
let keep = |id: &PackageId| keep(id) && !avoid_locking.contains(id);
661671

662672
registry.clear_lock();
663-
for node in resolve.iter().filter(keep) {
664-
let deps = resolve
665-
.deps_not_replaced(node)
666-
.map(|p| p.0)
667-
.filter(keep)
668-
.collect::<Vec<_>>();
669-
670-
// In the v2 lockfile format and prior the `branch=master` dependency
671-
// directive was serialized the same way as the no-branch-listed
672-
// directive. Nowadays in Cargo, however, these two directives are
673-
// considered distinct and are no longer represented the same way. To
674-
// maintain compatibility with older lock files we register locked nodes
675-
// for *both* the master branch and the default branch.
676-
//
677-
// Note that this is only applicable for loading older resolves now at
678-
// this point. All new lock files are encoded as v3-or-later, so this is
679-
// just compat for loading an old lock file successfully.
680-
if let Some(node) = master_branch_git_source(node, resolve) {
681-
registry.register_lock(node, deps.clone());
682-
}
673+
{
674+
let span = tracing::span!(tracing::Level::TRACE, "register_lock");
675+
let _enter = span.enter();
676+
for node in resolve.iter().filter(keep) {
677+
let deps = resolve
678+
.deps_not_replaced(node)
679+
.map(|p| p.0)
680+
.filter(keep)
681+
.collect::<Vec<_>>();
682+
683+
// In the v2 lockfile format and prior the `branch=master` dependency
684+
// directive was serialized the same way as the no-branch-listed
685+
// directive. Nowadays in Cargo, however, these two directives are
686+
// considered distinct and are no longer represented the same way. To
687+
// maintain compatibility with older lock files we register locked nodes
688+
// for *both* the master branch and the default branch.
689+
//
690+
// Note that this is only applicable for loading older resolves now at
691+
// this point. All new lock files are encoded as v3-or-later, so this is
692+
// just compat for loading an old lock file successfully.
693+
if let Some(node) = master_branch_git_source(node, resolve) {
694+
registry.register_lock(node, deps.clone());
695+
}
683696

684-
registry.register_lock(node, deps);
697+
registry.register_lock(node, deps);
698+
}
685699
}
686700

687701
/// Recursively add `node` and all its transitive dependencies to `set`.

src/cargo/sources/git/utils.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -164,6 +164,7 @@ impl GitRemote {
164164

165165
impl GitDatabase {
166166
/// Checkouts to a revision at `dest`ination from this database.
167+
#[tracing::instrument(skip(self, gctx))]
167168
pub fn copy_to(
168169
&self,
169170
rev: git2::Oid,

0 commit comments

Comments
 (0)