Set fork solution as preference when resolving #4662
Conversation
charliermarsh
added
performance
charliermarsh
force-pushed
the
charlie/pref
branch
from
9807a54
to
f6d8613
Compare
charliermarsh
force-pushed
the
charlie/pref
branch
from
f6d8613
to
22e0a61
Compare
| @@ -99,7 +99,7 @@ impl Preference { | |||
|
|
|||
| /// A set of pinned packages that should be preserved during resolution, if possible. | |||
| #[derive(Debug, Clone, Default)] | |||
| pub struct Preferences(Arc<FxHashMap<PackageName, Pin>>); | |||
| pub struct Preferences(FxHashMap<PackageName, Pin>); | |||
There was a problem hiding this comment.
Does anyone know why this is in an Arc?
There was a problem hiding this comment.
This was added when moving the solver into its own thread @ibraheemdev
charliermarsh
force-pushed
the
charlie/pref
branch
from
22e0a61
to
aa5b5a0
Compare
|
|
||
| // Walk over the selected versions, and mark them as preferences. | ||
| for state in &mut forked_states { | ||
| for (package, versions) in &resolution.packages { |
There was a problem hiding this comment.
This is quadratic but presumedly there are a small number of forks...?
There was a problem hiding this comment.
What about updating the ResolverState preferences instead? This should update all forks implicitly.
| @@ -1733,8 +1707,7 @@ fn fork_marker_selection() -> Result<()> { | |||
| version = "0.1.0" | |||
| source = { editable = "." } | |||
| dependencies = [ | |||
| { name = "package-a", version = "0.1.0", source = { registry = "https://astral-sh.github.io/packse/0.3.29/simple-html/" } }, | |||
| { name = "package-a", version = "0.2.0", source = { registry = "https://astral-sh.github.io/packse/0.3.29/simple-html/" } }, | |||
| { name = "package-a" }, | |||
There was a problem hiding this comment.
The comment on this test says:
/// So this acts as a regression test to ensure that only one version of `a` is selected.But I guess it wasn't actually doing that!
There was a problem hiding this comment.
Yeah I think we need a way to assert the result in the packse scenario itself. Otherwise it's pretty easy to miss if the result of the test isn't what you expected it to be.
| @@ -99,7 +99,7 @@ impl Preference { | |||
|
|
|||
| /// A set of pinned packages that should be preserved during resolution, if possible. | |||
| #[derive(Debug, Clone, Default)] | |||
| pub struct Preferences(Arc<FxHashMap<PackageName, Pin>>); | |||
| pub struct Preferences(FxHashMap<PackageName, Pin>); | |||
There was a problem hiding this comment.
This was added when moving the solver into its own thread @ibraheemdev
|
|
||
| // Walk over the selected versions, and mark them as preferences. | ||
| for state in &mut forked_states { | ||
| for (package, versions) in &resolution.packages { |
There was a problem hiding this comment.
What about updating the ResolverState preferences instead? This should update all forks implicitly.
That would require that the resolver state is taken mutably, but it's not here. |
|
I could do it by wrapping |
|
We could try a mutable |
|
That's true, I can try that. |
## Summary Avoids a quadratic loop. See: #4662.
Summary
This should both make it faster to solve forks (since we have a guess for a valid resolution, and will bias towards packages we've already fetched) and improve consistency between forks.
Closes #4617.