Simplify checking for dependency cycles #14089
Merged
Conversation
This file contains 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
|
r? @weihanglo rustbot has assigned @weihanglo. Use |
rustbot
added
A-dependency-resolution
weihanglo
approved these changes
Jun 18, 2024
Comment on lines
+1012
to
+1013
| let mut path = Vec::with_capacity(4); | ||
| let mut visited = HashSet::with_capacity(4); |
There was a problem hiding this comment.
Why are these initialized with a capacity of 4?
There was a problem hiding this comment.
Random guess. Initializing them with a capacity of 0 (::new()) seems silly when we know were going to push an item to it. Initializing them to the largest possible length (::with_capacity(resolve.len());) prevents a reallocation ever being needed, but is excessive as dependency trees tend to be wide not deep. So this was a compromise value I came up with off the top of my head. In effect it's just moving the allocation from "the first time we push" up to here. Given that none of this PR seems to have affected perf, I'd be happy to change it to whatever seems clear. What would you prefer?
There was a problem hiding this comment.
I am fine with merge this as-is, as the rationale has been called out here.
|
@bors r+ Thanks! |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
bors
added a commit
that referenced
this pull request
Jun 20, 2024
Simplify checking feature syntax ### What does this PR try to resolve? Similar to #14089, in my PubGrub work I had to try to understand the `build_feature_map` code that determines if the name refers to a feature or an optional dependency. There were a couple of little things I wanted to clean up while I was staring at the code. Specifically: - there was a lot of vertical space taken up with arguments to `bail` that could be in line at the format string. - some match statements were used where a named helper method would be clearer. - `split_once` could replace a `find ... split_at` dance. - in `dep_map` we were constructing a full `Vec<Dependency>`, when we only cared about whether there was any dependency and whether any of the dependencies were optional. ### How should we test and review this PR? It's an internal re-factor, and the tests still pass. ### Additional information If you're having questions about this code while you're reviewing, this would be a perfect opportunity for better naming and comments. Please ask questions. `@epage:` After this PR I am likely to copy this code into my pubgrub tester. Are there other users who might be interested in looking at a cargo.toml or an index entry and determining what feature names are available and what dependencies they enable? If so maybe this function should be moved to one of the new stable-ish reusable libraries.
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jun 22, 2024
Update cargo 17 commits in 3ed207e416fb2f678a40cc79c02dcf4f936a21ce..bc89bffa5987d4af8f71011c7557119b39e44a65 2024-06-18 19:18:22 +0000 to 2024-06-22 00:36:36 +0000 - test: migrate weak_dep_features, workspaces and yank to snapbox (rust-lang/cargo#14111) - test: migrate features and features(2|_namespaced) to snapbox (rust-lang/cargo#14100) - test: Add auto-redaction for not found error (rust-lang/cargo#14124) - test: migrate build to snapbox (rust-lang/cargo#14068) - test: migrate unit_graph, update and vendor to snapbox (rust-lang/cargo#14119) - fix(test): Un-redact Packaged files (rust-lang/cargo#14123) - test: Auto-redact file number (rust-lang/cargo#14121) - test: migrate lints_table and lints/(mod|unknown_lints) to snapbox (rust-lang/cargo#14104) - Simplify checking feature syntax (rust-lang/cargo#14106) - test: migrate testsuites to snapbox (rust-lang/cargo#14091) - Make `-Cmetadata` consistent across platforms (rust-lang/cargo#14107) - fix(toml): Warn when edition is unuset, even when MSRV is unset (rust-lang/cargo#14110) - Add `CodeFix::apply_solution` and impl `Clone` (rust-lang/cargo#14092) - test: migrate `cargo_alias_config&cargo_config/mod` to snapbox (rust-lang/cargo#14093) - Simplify checking for dependency cycles (rust-lang/cargo#14089) - test: Migrate `pub_priv.rs` to snapshot (rust-lang/cargo#14103) - test: migrate rustdoc and rustdocflags to snapbox (rust-lang/cargo#14098) <!-- r? ghost -->
github-actions bot
pushed a commit
to rust-lang/miri
that referenced
this pull request
Jun 23, 2024
Update cargo 17 commits in 3ed207e416fb2f678a40cc79c02dcf4f936a21ce..bc89bffa5987d4af8f71011c7557119b39e44a65 2024-06-18 19:18:22 +0000 to 2024-06-22 00:36:36 +0000 - test: migrate weak_dep_features, workspaces and yank to snapbox (rust-lang/cargo#14111) - test: migrate features and features(2|_namespaced) to snapbox (rust-lang/cargo#14100) - test: Add auto-redaction for not found error (rust-lang/cargo#14124) - test: migrate build to snapbox (rust-lang/cargo#14068) - test: migrate unit_graph, update and vendor to snapbox (rust-lang/cargo#14119) - fix(test): Un-redact Packaged files (rust-lang/cargo#14123) - test: Auto-redact file number (rust-lang/cargo#14121) - test: migrate lints_table and lints/(mod|unknown_lints) to snapbox (rust-lang/cargo#14104) - Simplify checking feature syntax (rust-lang/cargo#14106) - test: migrate testsuites to snapbox (rust-lang/cargo#14091) - Make `-Cmetadata` consistent across platforms (rust-lang/cargo#14107) - fix(toml): Warn when edition is unuset, even when MSRV is unset (rust-lang/cargo#14110) - Add `CodeFix::apply_solution` and impl `Clone` (rust-lang/cargo#14092) - test: migrate `cargo_alias_config&cargo_config/mod` to snapbox (rust-lang/cargo#14093) - Simplify checking for dependency cycles (rust-lang/cargo#14089) - test: Migrate `pub_priv.rs` to snapshot (rust-lang/cargo#14103) - test: migrate rustdoc and rustdocflags to snapbox (rust-lang/cargo#14098) <!-- r? ghost -->
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.
What does this PR try to resolve?
In my PubGrub work I had to try to understand the
check_cyclespost resolution pass. I found the code tricky to understand and doing some unnecessary reallocations. I cleaned up the code to remove the unnecessary data structures so that I could better understand.How should we test and review this PR?
It's an internal re-factor, and the tests still pass. But this code is not extensively tested by our test suite. In addition I ran my "resolve every crate on crates.io" script against a local check out of this branch. With the old code as
check_cycles_oldand the PR code ascheck_cycles_newthe following assertion did not hit:That experiment also demonstrated that this was not a significant performance change in either direction.
Additional information
If you're having questions about this code while you're reviewing, this would be a perfect opportunity for better naming and comments. Please ask questions.