Public dependency refactor and re-allow backjumping #7361
Conversation
|
r? @nrc (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
@Eh2406 would you like me to take over reviewing this? If so, anything else you'd like me to know before starting? |
|
Happy to have your revue! It is probably under documented, so guidance on what is confusing is helpful. The older commits are more clear benefits then the newer ones. If the new representation of a conflict requires long discussion then we can split this PR. Land the refactorings, and discuss the representation separately. |
Eh2406
force-pushed
the
public_dependency-as-type_4
branch
from
001093e
to
52d5ea3
Compare
|
☔ The latest upstream changes (presumably #7452) made this pull request unmergeable. Please resolve the merge conflicts. |
Eh2406
force-pushed
the
public_dependency-as-type_4
branch
from
52d5ea3
to
7d4ad71
Compare
Note: this commit does not change code, just moves it
A pub dep conflict can be made by connecting two already activated pids
Eh2406
force-pushed
the
public_dependency-as-type_4
branch
from
7d4ad71
to
0750caf
Compare
|
Rebased |
|
Sorry for taking awhile to get back to this. My eyes unfortunately still sort of glaze over when reading the resolver, so I think I'm sort of quickly becoming not able to review too much of the technical content of these sorts of changes to the resolver. That being said I trust you and this is a pretty sequestered part of the resolver, so I'm fine adding this in-tree and we can continue to iterate over time. Again sorry for the delay, it takes me a long time to build up energy to dive into the resolver nowadays :( @bors: r+ |
|
📌 Commit 0750caf has been approved by |
bors
added
S-waiting-on-bors
|
@bors: r+ |
|
💡 This pull request was already approved, no need to approve it again. |
|
📌 Commit 0750caf has been approved by |
|
⌛ Testing commit 0750caf with merge 8df2b21e7a303a9693db2fba78a191b207802777... |
Public dependency refactor and re-allow backjumping There were **three** attempts at vanquishing exponential time spent in Public dependency resolution. All failures. All three started with some refactoring that seams worth saving. Specifically the data structure `public_dependency` that is used to test for Public dependency conflicts is large, tricky, and modified in line. So lets make it a type with a name and move the interactions into methods. Next each attempt needed to know how far back to jump to undo any given dependency edge. I am fairly confident that any full solution will need this functionality. I also think any solution will need a way to represent richer conflicts than the existing "is this pid active". So let's keep the `still_applies` structure from the last attempt. Last each attempt needs to pick a way to represent a Public dependency conflict. The last attempt used three facts about a situation. - `a1`: `PublicDependency(p)` witch can be read as the package `p` can see the package `a1` - `b`: `PublicDependency(p)` witch can be read as the package `p` can see the package `b` - `a2`: `PubliclyExports(b)` witch can be read as the package `b` has the package `a2` in its publick interface. This representation is good enough to allow for `backjumping`. I.E. `find_candidate` can go back several frames until the `age` when the Public dependency conflict was introduced. This optimization, added for normal dependencies in #4834, saves the most time in practice. So having it for Public dependency conflicts is important for allowing real world experimentation of the Public dependencies feature. We will have to alter/improve/replace this representation to unlock all of the important optimizations. But I don't know one that will work for all of them and this is a major step forward. Can be read one commit at a time.
|
☀️ Test successful - checks-azure |
Update Cargo To pull rust-lang/cargo#7482 List of merged PRs: - Fix wrong directories in PATH on Windows (rust-lang/cargo#7482) - Update SPDX list to 3.6 (rust-lang/cargo#7481) - Mark Emscripten's .wasm files auxiliary (rust-lang/cargo#7476) - Update `curl-sys` dependency requirement (rust-lang/cargo#7464) - add dependencies for `pkg-config` (rust-lang/cargo#7443) - Removing hash from output files when using MSVC (rust-lang/cargo#7400) - Disable preserving mtimes on archives (rust-lang/cargo#7465) - Removed redundant borrow (rust-lang/cargo#7462) - Public dependency refactor and re-allow backjumping (rust-lang/cargo#7361) - unify the quote in Cargo.toml (rust-lang/cargo#7461)
There were three attempts at vanquishing exponential time spent in Public dependency resolution. All failures. All three started with some refactoring that seams worth saving. Specifically the data structure
public_dependencythat is used to test for Public dependency conflicts is large, tricky, and modified in line. So lets make it a type with a name and move the interactions into methods.Next each attempt needed to know how far back to jump to undo any given dependency edge. I am fairly confident that any full solution will need this functionality. I also think any solution will need a way to represent richer conflicts than the existing "is this pid active". So let's keep the
still_appliesstructure from the last attempt.Last each attempt needs to pick a way to represent a Public dependency conflict. The last attempt used three facts about a situation.
a1:PublicDependency(p)witch can be read as the packagepcan see the packagea1b:PublicDependency(p)witch can be read as the packagepcan see the packageba2:PubliclyExports(b)witch can be read as the packagebhas the packagea2in its publick interface.This representation is good enough to allow for
backjumping. I.E.find_candidatecan go back several frames until theagewhen the Public dependency conflict was introduced. This optimization, added for normal dependencies in #4834, saves the most time in practice. So having it for Public dependency conflicts is important for allowing real world experimentation of the Public dependencies feature. We will have to alter/improve/replace this representation to unlock all of the important optimizations. But I don't know one that will work for all of them and this is a major step forward.Can be read one commit at a time.