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

cargo locate-project --workspace loads all the workspace #15107

Open
tkr-sh opened this issue Jan 26, 2025 · 6 comments
Open

cargo locate-project --workspace loads all the workspace #15107

tkr-sh opened this issue Jan 26, 2025 · 6 comments
Labels
C-bug Category: bug Command-locate-project Performance Gotta go fast! S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review

Comments

@tkr-sh
Copy link

tkr-sh commented Jan 26, 2025

Problem

cargo locate-project --workspace loads all the workspace instead of just returning the path of the workspace root.

Steps

cargo locate-project --workspace

Possible Solution(s)

Instead of doing

src/bin/cargo/commands/locate_project.rs

WhatToFind::Workspace => {
    workspace = args.workspace(gctx)?;
    workspace.root_manifest()
}

I think that

WhatToFind::Workspace => {
    let root = args.root_manifest(gctx)?;
    let mut ws = Workspace::new_default(root.to_path_buf(), gctx);
    ws.target_dir = gctx.target_dir()?;
    ws.find_root(manifest_path)?
}

or something like that would be better. Because when in a big workspace with a lot of members in a workspace, it will load some unused data.

@tkr-sh tkr-sh added C-bug Category: bug S-triage Status: This issue is waiting on initial triage. labels Jan 26, 2025
@weihanglo
Copy link
Member

Could you elaborate a bit on performance data you're looking for, and the scenario? It seems to me in a workspace of 400+ members (aws-sdk-rust) it takes ~150ms. Not ideal but it would be better to understand the use cases.

@weihanglo weihanglo added Command-locate-project S-needs-info Status: Needs more info, such as a reproduction or more background for a feature request. and removed S-triage Status: This issue is waiting on initial triage. labels Jan 26, 2025
@tkr-sh
Copy link
Author

tkr-sh commented Jan 26, 2025

@weihanglo I sometimes work on a workspace with ~1500 members (not open source sadly), and in this case, it takes most of the time more than a second.

@weihanglo
Copy link
Member

arg.workspace calls Workspace::new which additionally validates the workspace. I am unsure if the proposed solution is the correct approach, as it completely skip the validation. Granted, the validation needs loading all members, which is the core of the performance issue.

@tkr-sh
Copy link
Author

tkr-sh commented Jan 26, 2025

Hmmm interesting. Maybe adding a flag --no-verify would be a possible solution when performance is needed and that extra validation isn't required because the workspace is assumed to be correct ?

@epage
Copy link
Contributor

epage commented Jan 27, 2025

imo if this command is just about locating the workspace, I think its reasonable to skip validation. Other commands will need to be run and those can do that validation.

We could punt on proper plumbing commands which would require doing the minimal work. I don't think its a big loss to this command to do it now.

@epage epage added the Performance Gotta go fast! label Jan 27, 2025
@weihanglo
Copy link
Member

This sounds pretty reasonable. I went ahead and marked this as accepted.

@weihanglo weihanglo added S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review and removed S-needs-info Status: Needs more info, such as a reproduction or more background for a feature request. labels Jan 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: bug Command-locate-project Performance Gotta go fast! S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review
Projects
None yet
Development

No branches or pull requests

3 participants