mirror of
https://github.com/astral-sh/uv.git
synced 2025-08-04 10:58:28 +00:00
Arc-wrap PubGrubPackage for cheap cloning in pubgrub (#3688)
Pubgrub stores incompatibilities as (package name, version range) tuples, meaning it needs to clone the package name for each incompatibility, and each non-borrowed operation on incompatibilities. https://github.com/astral-sh/uv/pull/3673 made me realize that `PubGrubPackage` has gotten large (expensive to copy), so like `Version` and other structs, i've added an `Arc` wrapper around it. It's a pity clippy forbids `.deref()`, it's less opaque than `&**` and has IDE support (clicking on `.deref()` jumps to the right impl). ## Benchmarks It looks like this matters most for complex resolutions which, i assume because they carry larger `PubGrubPackageInner::Package` and `PubGrubPackageInner::Extra` types. ```bash hyperfine --warmup 5 "./uv-main pip compile -q ./scripts/requirements/jupyter.in" "./uv-branch pip compile -q ./scripts/requirements/jupyter.in" hyperfine --warmup 5 "./uv-main pip compile -q ./scripts/requirements/airflow.in" "./uv-branch pip compile -q ./scripts/requirements/airflow.in" hyperfine --warmup 5 "./uv-main pip compile -q ./scripts/requirements/boto3.in" "./uv-branch pip compile -q ./scripts/requirements/boto3.in" ``` ``` Benchmark 1: ./uv-main pip compile -q ./scripts/requirements/jupyter.in Time (mean ± σ): 18.2 ms ± 1.6 ms [User: 14.4 ms, System: 26.0 ms] Range (min … max): 15.8 ms … 22.5 ms 181 runs Benchmark 2: ./uv-branch pip compile -q ./scripts/requirements/jupyter.in Time (mean ± σ): 17.8 ms ± 1.4 ms [User: 14.4 ms, System: 25.3 ms] Range (min … max): 15.4 ms … 23.1 ms 159 runs Summary ./uv-branch pip compile -q ./scripts/requirements/jupyter.in ran 1.02 ± 0.12 times faster than ./uv-main pip compile -q ./scripts/requirements/jupyter.in ``` ``` Benchmark 1: ./uv-main pip compile -q ./scripts/requirements/airflow.in Time (mean ± σ): 153.7 ms ± 3.5 ms [User: 165.2 ms, System: 157.6 ms] Range (min … max): 150.4 ms … 163.0 ms 19 runs Benchmark 2: ./uv-branch pip compile -q ./scripts/requirements/airflow.in Time (mean ± σ): 123.9 ms ± 4.6 ms [User: 152.4 ms, System: 133.8 ms] Range (min … max): 118.4 ms … 138.1 ms 24 runs Summary ./uv-branch pip compile -q ./scripts/requirements/airflow.in ran 1.24 ± 0.05 times faster than ./uv-main pip compile -q ./scripts/requirements/airflow.in ``` ``` Benchmark 1: ./uv-main pip compile -q ./scripts/requirements/boto3.in Time (mean ± σ): 327.0 ms ± 3.8 ms [User: 344.5 ms, System: 71.6 ms] Range (min … max): 322.7 ms … 334.6 ms 10 runs Benchmark 2: ./uv-branch pip compile -q ./scripts/requirements/boto3.in Time (mean ± σ): 311.2 ms ± 3.1 ms [User: 339.3 ms, System: 63.1 ms] Range (min … max): 307.8 ms … 317.0 ms 10 runs Summary ./uv-branch pip compile -q ./scripts/requirements/boto3.in ran 1.05 ± 0.02 times faster than ./uv-main pip compile -q ./scripts/requirements/boto3.in ``` <!-- Thank you for contributing to uv! To help us out with reviewing, please consider the following: - Does this pull request include a summary of the change? (See below.) - Does this pull request include a descriptive title? - Does this pull request include references to any relevant issues? -->
This commit is contained in:
konsti
GitHub
parent
fbae55019e
commit
76418f5bdf
10 changed files with 159 additions and 122 deletions
|
|
@ -43,8 +43,8 @@ use crate::manifest::Manifest;
|
|||
use crate::pins::FilePins;
|
||||
use crate::preferences::Preferences;
|
||||
use crate::pubgrub::{
|
||||
PubGrubDependencies, PubGrubDistribution, PubGrubPackage, PubGrubPriorities, PubGrubPython,
|
||||
PubGrubRequirement, PubGrubSpecifier,
|
||||
PubGrubDependencies, PubGrubDistribution, PubGrubPackage, PubGrubPackageInner,
|
||||
PubGrubPriorities, PubGrubPython, PubGrubRequirement, PubGrubSpecifier,
|
||||
};
|
||||
use crate::python_requirement::PythonRequirement;
|
||||
use crate::resolution::ResolutionGraph;
|
||||
|
|
@ -401,7 +401,7 @@ impl<InstalledPackages: InstalledPackagesProvider> ResolverState<InstalledPackag
|
|||
visited: &mut FxHashSet<PackageName>,
|
||||
request_sink: Sender<Request>,
|
||||
) -> Result<ResolutionGraph, ResolveError> {
|
||||
let root = PubGrubPackage::Root(self.project.clone());
|
||||
let root = PubGrubPackage::from(PubGrubPackageInner::Root(self.project.clone()));
|
||||
let mut prefetcher = BatchPrefetcher::default();
|
||||
let mut state = SolveState {
|
||||
pubgrub: State::init(root.clone(), MIN_VERSION.clone()),
|
||||
|
|
@ -480,7 +480,7 @@ impl<InstalledPackages: InstalledPackagesProvider> ResolverState<InstalledPackag
|
|||
.expect("a package was chosen but we don't have a term.");
|
||||
|
||||
// Check if the decision was due to the package being unavailable
|
||||
if let PubGrubPackage::Package { ref name, .. } = state.next {
|
||||
if let PubGrubPackageInner::Package { ref name, .. } = &*state.next {
|
||||
if let Some(entry) = self.unavailable_packages.get(name) {
|
||||
state
|
||||
.pubgrub
|
||||
|
|
@ -530,7 +530,10 @@ impl<InstalledPackages: InstalledPackagesProvider> ResolverState<InstalledPackag
|
|||
.add_incompatibility(Incompatibility::from_dependency(
|
||||
package.clone(),
|
||||
Range::singleton(version.clone()),
|
||||
(PubGrubPackage::Python(kind), python_version.clone()),
|
||||
(
|
||||
PubGrubPackage::from(PubGrubPackageInner::Python(kind)),
|
||||
python_version.clone(),
|
||||
),
|
||||
));
|
||||
}
|
||||
state
|
||||
|
|
@ -633,11 +636,11 @@ impl<InstalledPackages: InstalledPackagesProvider> ResolverState<InstalledPackag
|
|||
package: &PubGrubPackage,
|
||||
request_sink: &Sender<Request>,
|
||||
) -> Result<(), ResolveError> {
|
||||
match package {
|
||||
PubGrubPackage::Root(_) => {}
|
||||
PubGrubPackage::Python(_) => {}
|
||||
PubGrubPackage::Extra { .. } => {}
|
||||
PubGrubPackage::Package {
|
||||
match &**package {
|
||||
PubGrubPackageInner::Root(_) => {}
|
||||
PubGrubPackageInner::Python(_) => {}
|
||||
PubGrubPackageInner::Extra { .. } => {}
|
||||
PubGrubPackageInner::Package {
|
||||
name, url: None, ..
|
||||
} => {
|
||||
// Verify that the package is allowed under the hash-checking policy.
|
||||
|
|
@ -650,7 +653,7 @@ impl<InstalledPackages: InstalledPackagesProvider> ResolverState<InstalledPackag
|
|||
request_sink.blocking_send(Request::Package(name.clone()))?;
|
||||
}
|
||||
}
|
||||
PubGrubPackage::Package {
|
||||
PubGrubPackageInner::Package {
|
||||
name,
|
||||
url: Some(url),
|
||||
..
|
||||
|
|
@ -684,12 +687,12 @@ impl<InstalledPackages: InstalledPackagesProvider> ResolverState<InstalledPackag
|
|||
// Iterate over the potential packages, and fetch file metadata for any of them. These
|
||||
// represent our current best guesses for the versions that we _might_ select.
|
||||
for (package, range) in packages {
|
||||
let PubGrubPackage::Package {
|
||||
let PubGrubPackageInner::Package {
|
||||
name,
|
||||
extra: None,
|
||||
marker: None,
|
||||
url: None,
|
||||
} = package
|
||||
} = &**package
|
||||
else {
|
||||
continue;
|
||||
};
|
||||
|
|
@ -711,10 +714,12 @@ impl<InstalledPackages: InstalledPackagesProvider> ResolverState<InstalledPackag
|
|||
visited: &mut FxHashSet<PackageName>,
|
||||
request_sink: &Sender<Request>,
|
||||
) -> Result<Option<ResolverVersion>, ResolveError> {
|
||||
match package {
|
||||
PubGrubPackage::Root(_) => Ok(Some(ResolverVersion::Available(MIN_VERSION.clone()))),
|
||||
match &**package {
|
||||
PubGrubPackageInner::Root(_) => {
|
||||
Ok(Some(ResolverVersion::Available(MIN_VERSION.clone())))
|
||||
}
|
||||
|
||||
PubGrubPackage::Python(PubGrubPython::Installed) => {
|
||||
PubGrubPackageInner::Python(PubGrubPython::Installed) => {
|
||||
let version = self.python_requirement.installed();
|
||||
if range.contains(version) {
|
||||
Ok(Some(ResolverVersion::Available(version.deref().clone())))
|
||||
|
|
@ -723,7 +728,7 @@ impl<InstalledPackages: InstalledPackagesProvider> ResolverState<InstalledPackag
|
|||
}
|
||||
}
|
||||
|
||||
PubGrubPackage::Python(PubGrubPython::Target) => {
|
||||
PubGrubPackageInner::Python(PubGrubPython::Target) => {
|
||||
let version = self.python_requirement.target();
|
||||
if range.contains(version) {
|
||||
Ok(Some(ResolverVersion::Available(version.deref().clone())))
|
||||
|
|
@ -732,12 +737,12 @@ impl<InstalledPackages: InstalledPackagesProvider> ResolverState<InstalledPackag
|
|||
}
|
||||
}
|
||||
|
||||
PubGrubPackage::Extra {
|
||||
PubGrubPackageInner::Extra {
|
||||
name,
|
||||
url: Some(url),
|
||||
..
|
||||
}
|
||||
| PubGrubPackage::Package {
|
||||
| PubGrubPackageInner::Package {
|
||||
name,
|
||||
url: Some(url),
|
||||
..
|
||||
|
|
@ -833,10 +838,10 @@ impl<InstalledPackages: InstalledPackagesProvider> ResolverState<InstalledPackag
|
|||
Ok(Some(ResolverVersion::Available(version.clone())))
|
||||
}
|
||||
|
||||
PubGrubPackage::Extra {
|
||||
PubGrubPackageInner::Extra {
|
||||
name, url: None, ..
|
||||
}
|
||||
| PubGrubPackage::Package {
|
||||
| PubGrubPackageInner::Package {
|
||||
name, url: None, ..
|
||||
} => {
|
||||
// Wait for the metadata to be available.
|
||||
|
|
@ -916,7 +921,7 @@ impl<InstalledPackages: InstalledPackagesProvider> ResolverState<InstalledPackag
|
|||
let version = candidate.version().clone();
|
||||
|
||||
// Emit a request to fetch the metadata for this version.
|
||||
if matches!(package, PubGrubPackage::Package { .. }) {
|
||||
if matches!(&**package, PubGrubPackageInner::Package { .. }) {
|
||||
if self.index.distributions().register(candidate.version_id()) {
|
||||
let request = Request::from(dist.for_resolution());
|
||||
request_sink.blocking_send(request)?;
|
||||
|
|
@ -937,8 +942,8 @@ impl<InstalledPackages: InstalledPackagesProvider> ResolverState<InstalledPackag
|
|||
priorities: &mut PubGrubPriorities,
|
||||
request_sink: &Sender<Request>,
|
||||
) -> Result<Dependencies, ResolveError> {
|
||||
match package {
|
||||
PubGrubPackage::Root(_) => {
|
||||
match &**package {
|
||||
PubGrubPackageInner::Root(_) => {
|
||||
// Add the root requirements.
|
||||
let dependencies = PubGrubDependencies::from_requirements(
|
||||
&self.requirements,
|
||||
|
|
@ -1021,9 +1026,9 @@ impl<InstalledPackages: InstalledPackagesProvider> ResolverState<InstalledPackag
|
|||
Ok(Dependencies::Available(dependencies.into()))
|
||||
}
|
||||
|
||||
PubGrubPackage::Python(_) => Ok(Dependencies::Available(Vec::default())),
|
||||
PubGrubPackageInner::Python(_) => Ok(Dependencies::Available(Vec::default())),
|
||||
|
||||
PubGrubPackage::Package {
|
||||
PubGrubPackageInner::Package {
|
||||
name,
|
||||
extra,
|
||||
marker: _marker,
|
||||
|
|
@ -1192,28 +1197,28 @@ impl<InstalledPackages: InstalledPackagesProvider> ResolverState<InstalledPackag
|
|||
}
|
||||
|
||||
// Add a dependency on both the extra and base package.
|
||||
PubGrubPackage::Extra {
|
||||
PubGrubPackageInner::Extra {
|
||||
name,
|
||||
extra,
|
||||
marker: _marker,
|
||||
url,
|
||||
} => Ok(Dependencies::Available(vec![
|
||||
(
|
||||
PubGrubPackage::Package {
|
||||
PubGrubPackage::from(PubGrubPackageInner::Package {
|
||||
name: name.clone(),
|
||||
extra: None,
|
||||
marker: None,
|
||||
url: url.clone(),
|
||||
},
|
||||
}),
|
||||
Range::singleton(version.clone()),
|
||||
),
|
||||
(
|
||||
PubGrubPackage::Package {
|
||||
PubGrubPackage::from(PubGrubPackageInner::Package {
|
||||
name: name.clone(),
|
||||
extra: Some(extra.clone()),
|
||||
marker: None,
|
||||
url: url.clone(),
|
||||
},
|
||||
}),
|
||||
Range::singleton(version.clone()),
|
||||
),
|
||||
])),
|
||||
|
|
@ -1438,18 +1443,18 @@ impl<InstalledPackages: InstalledPackagesProvider> ResolverState<InstalledPackag
|
|||
|
||||
fn on_progress(&self, package: &PubGrubPackage, version: &Version) {
|
||||
if let Some(reporter) = self.reporter.as_ref() {
|
||||
match package {
|
||||
PubGrubPackage::Root(_) => {}
|
||||
PubGrubPackage::Python(_) => {}
|
||||
PubGrubPackage::Extra { .. } => {}
|
||||
PubGrubPackage::Package {
|
||||
match &**package {
|
||||
PubGrubPackageInner::Root(_) => {}
|
||||
PubGrubPackageInner::Python(_) => {}
|
||||
PubGrubPackageInner::Extra { .. } => {}
|
||||
PubGrubPackageInner::Package {
|
||||
name,
|
||||
url: Some(url),
|
||||
..
|
||||
} => {
|
||||
reporter.on_progress(name, &VersionOrUrlRef::Url(&url.verbatim));
|
||||
}
|
||||
PubGrubPackage::Package {
|
||||
PubGrubPackageInner::Package {
|
||||
name, url: None, ..
|
||||
} => {
|
||||
reporter.on_progress(name, &VersionOrUrlRef::Version(version));
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue