Skip to content

/cargo

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

Backtrack more aggressively when resolving #1804

Merged
merged 4 commits into from Jul 17, 2015

Conversation

Reviewers
No reviews
Assignees

@brson brson

Labels
None yet
Projects
None yet
Milestone
No milestone
5 participants
@alexcrichton @rust-highfive @bors @brson @wycats
@alexcrichton
Copy link
Member

alexcrichton commented Jul 13, 2015

Resolving a dependency graph may involve quite a bit of backtracking to select
different versions of crates, but the previous implementation of resolution
would not backtrack enough.

Once a dependency is completely resolved it's possible that any number of
candidates for its transitive dependencies were left out of the resolution
process (e.g. we didn't hit them yet). These candidates were not previously used
for backtracking (because resolution overall of the dependency finished), but
this commit alters the implementation to instead consider these as candidates
for backtracking.

Architecturally this changes the code to CPS to pass around a finished
continuation to allow tweaking the behavior whenever a dependency successfully
resolves. The key change is then that whenever a candidate for a dependency is
activated, we ensure the recursive step for the rest of the graph happens as a
sub-call. This means that if anything in the recursive call fails (including
some previous up-the-stack resolution) we'll retry the next candidate.

Closes #1800

@rust-highfive rust-highfive assigned wycats Jul 13, 2015

@rust-highfive

This comment has been minimized.

Sign in to view
Copy link

rust-highfive commented Jul 13, 2015

r? @wycats

(rust_highfive has picked a reviewer for you, use r? to override)
@alexcrichton

This comment has been minimized.

Sign in to view
Copy link
Member Author

alexcrichton commented Jul 13, 2015

Hm, maybe don't r+ just yet, want to test out on Servo to make sure this doesn't blow any stacks. It'd be unfortunate to have to switch to an explicit stack...

@alexcrichton alexcrichton force-pushed the alexcrichton:aggressively-backtrack branch from 69d2513 to 596fa60 Jul 14, 2015

@alexcrichton

This comment has been minimized.

Sign in to view
Copy link
Member Author

alexcrichton commented Jul 14, 2015

Ok, I have overhauled this to include some more changes. This has evolved into a bit of a larger scale refactor of the resolution code, although most of the changes are just code movement. I can split out the literal code movement changes if it makes it easier to review. The other changes made are:

  • Many functions are now on impl Context directly.
  • Many functions are tagged #[inline(never)] to limit the stack size of the recursive activate and activate_deps functions.
  • Documentation has been added for the module as a whole as well as the performance tricks currently in play
  • The Rust snapshot version has been updated as its LLVM apparently has a few optimizations Cargo needs to get those smaller stacks.

r? @brson

@rust-highfive rust-highfive assigned brson and unassigned wycats Jul 14, 2015

@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Jul 17, 2015

☔️ The latest upstream changes (presumably #1812) made this pull request unmergeable. Please resolve the merge conflicts.

@alexcrichton alexcrichton force-pushed the alexcrichton:aggressively-backtrack branch from 596fa60 to 8e9ce3a Jul 17, 2015

@brson

This comment has been minimized.

Sign in to view
Copy link
Contributor

brson commented Jul 17, 2015

r=me

alexcrichton added some commits Jul 13, 2015

@alexcrichton
Fix running `cargo test` from the project root 
Previously the `cit` folder was placed in the root directory but this adds logic
to ensure it stays within the `target` subdirectory.
4b69b3b
@alexcrichton
Update Rust nightly 
Looks like the new LLVM version has optimizations which help out a good deal
with the recursion faced in the resolver, so let's use that version instead!
f3d6e35
@alexcrichton
Backtrack more aggressively when resolving 
Resolving a dependency graph may involve quite a bit of backtracking to select
different versions of crates, but the previous implementation of resolution
would not backtrack enough.

Once a dependency is completely resolved it's possible that any number of
candidates for its transitive dependencies were left out of the resolution
process (e.g. we didn't hit them yet). These candidates were not previously used
for backtracking (because resolution overall of the dependency finished), but
this commit alters the implementation to instead consider these as candidates
for backtracking.

Architecturally this changes the code to CPS to pass around a `finished`
continuation to allow tweaking the behavior whenever a dependency successfully
resolves. The key change is then that whenever a candidate for a dependency is
activated, we ensure the recursive step for the rest of the graph happens as a
sub-call. This means that if *anything* in the recursive call fails (including
some previous up-the-stack resolution) we'll retry the next candidate.

Closes #1800
44eb16d

@alexcrichton alexcrichton force-pushed the alexcrichton:aggressively-backtrack branch from 8e9ce3a to 13971b4 Jul 17, 2015

@alexcrichton alexcrichton force-pushed the alexcrichton:aggressively-backtrack branch from 13971b4 to bcdb747 Jul 17, 2015

@alexcrichton

This comment has been minimized.

Sign in to view
Copy link
Member Author

alexcrichton commented Jul 17, 2015

@bors: r=brson bcdb747
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Jul 17, 2015

⌛️ Testing commit bcdb747 with merge a10897c...

bors added a commit that referenced this pull request Jul 17, 2015

@bors
Auto merge of #1804 - alexcrichton:aggressively-backtrack, r=brson 
Resolving a dependency graph may involve quite a bit of backtracking to select
different versions of crates, but the previous implementation of resolution
would not backtrack enough.

Once a dependency is completely resolved it's possible that any number of
candidates for its transitive dependencies were left out of the resolution
process (e.g. we didn't hit them yet). These candidates were not previously used
for backtracking (because resolution overall of the dependency finished), but
this commit alters the implementation to instead consider these as candidates
for backtracking.

Architecturally this changes the code to CPS to pass around a `finished`
continuation to allow tweaking the behavior whenever a dependency successfully
resolves. The key change is then that whenever a candidate for a dependency is
activated, we ensure the recursive step for the rest of the graph happens as a
sub-call. This means that if *anything* in the recursive call fails (including
some previous up-the-stack resolution) we'll retry the next candidate.

Closes #1800
Loading status checks…
a10897c
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Jul 17, 2015

☀️ Test successful - cargo-linux-32, cargo-linux-64, cargo-mac-32, cargo-mac-64, cargo-win-gnu-32, cargo-win-gnu-64, cargo-win-msvc-64

@bors bors merged commit bcdb747 into rust-lang:master Jul 17, 2015
2 of 3 checks passed

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/appveyor AppVeyor build succeeded
Details
homu Test successful
Details

@alexcrichton alexcrichton deleted the alexcrichton:aggressively-backtrack branch Jul 17, 2015

@retep998 retep998 referenced this pull request Jul 18, 2015

Closed

Failed to select a version for `winapi` #1815

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.