Skip to content

Commit

Permalink
Box PrioritizedDistribution (#948)
Browse files Browse the repository at this point in the history
On top of #947, we can also box
`PrioritizedDistribution`.

In a simple benchmark, this seems to slightly improve performance when
comparing only this commit to main, even though the benchmark is too
noisy to establish significance:

```
$ hyperfine --warmup 30 --runs 300 "target/profiling/main-dev resolve meine_stadt_transparent" "target/profiling/puffin-dev resolve meine_stadt_transparent"
  Benchmark 1: target/profiling/main-dev resolve meine_stadt_transparent
    Time (mean ± σ):      83.6 ms ±   2.0 ms    [User: 77.7 ms, System: 20.0 ms]
    Range (min … max):    81.4 ms …  98.2 ms    300 runs

    Warning: Statistical outliers were detected. Consider re-running this benchmark on a quiet system without any interferences from other programs. It might help to use the '--warmup' or '--prepare' options.

  Benchmark 2: target/profiling/puffin-dev resolve meine_stadt_transparent
    Time (mean ± σ):      80.8 ms ±   2.2 ms    [User: 75.4 ms, System: 19.5 ms]
    Range (min … max):    78.6 ms …  98.6 ms    300 runs

    Warning: Statistical outliers were detected. Consider re-running this benchmark on a quiet system without any interferences from other programs. It might help to use the '--warmup' or '--prepare' options.

  Summary
    target/profiling/puffin-dev resolve meine_stadt_transparent ran
      1.03 ± 0.04 times faster than target/profiling/main-dev resolve meine_stadt_transparent
```

The effect on type sizes however is considerable ([downstack
PR](https://gist.github.com/konstin/38e6c774db541db46d61f1d4ea6b498f)
vs. [this
PR](https://gist.github.com/konstin/003a77fe7d7d246b0d535e3fc843cb36)):

```patch
--- branch.txt  2024-01-17 14:26:01.826085176 +0100
+++ boxed-prioritized-dist.txt  2024-01-17 14:25:57.101900963 +0100
@@ -1,19 +1,3 @@
-9264 alloc::collections::btree::node::InternalNode<pep440_rs::version::Version, distribution_types::PrioritizedDistribution> align=8
-   9168 data
-     96 edges
-
-9264 alloc::collections::btree::node::InternalNode<pep440_rs::Version, distribution_types::PrioritizedDistribution> align=8
-   9168 data
-     96 edges
-
-9168 alloc::collections::btree::node::LeafNode<pep440_rs::version::Version, distribution_types::PrioritizedDistribution> align=8
-   9064 vals
-     88 keys
-
-9168 alloc::collections::btree::node::LeafNode<pep440_rs::Version, distribution_types::PrioritizedDistribution> align=8
-   9064 vals
-     88 keys
-
 8992 tokio::sync::mpsc::block::Block<hyper::client::dispatch::Envelope<http::request::Request<reqwest::async_impl::body::ImplStream>, http::response::Response<hyper::body::body::Body>>> align=8
    8960 values
      32 header
@@ -74,10 +58,23 @@
          40 __tracing_attr_span
      64 variant Unresumed, Returned, Panicked

+5648 {async fn body@crates/puffin-client/src/registry_client.rs:224:5: 224:30} align=8
+   5647 variant Suspend0
+       5576 __awaitee align=8
+         40 __tracing_attr_span
```
  • Loading branch information
konstin authored Jan 19, 2024
1 parent 47fc90d commit cd2fb6f
Showing 1 changed file with 26 additions and 22 deletions.
48 changes: 26 additions & 22 deletions crates/distribution-types/src/prioritized_distribution.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,12 @@ pub struct DistRequiresPython {
pub requires_python: Option<VersionSpecifiers>,
}

// Boxed because `Dist` is large.
#[derive(Debug, Clone)]
pub struct PrioritizedDistribution {
pub struct PrioritizedDistribution(Box<PrioritizedDistributionInner>);

#[derive(Debug, Clone)]
struct PrioritizedDistributionInner {
/// An arbitrary source distribution for the package version.
source: Option<DistRequiresPython>,
/// The highest-priority, platform-compatible wheel for the package version.
Expand All @@ -33,7 +37,7 @@ impl PrioritizedDistribution {
priority: Option<TagPriority>,
) -> Self {
if let Some(priority) = priority {
Self {
Self(Box::new(PrioritizedDistributionInner {
source: None,
compatible_wheel: Some((
DistRequiresPython {
Expand All @@ -45,17 +49,17 @@ impl PrioritizedDistribution {
)),
incompatible_wheel: None,
hashes: hash.map(|hash| vec![hash]).unwrap_or_default(),
}
}))
} else {
Self {
Self(Box::new(PrioritizedDistributionInner {
source: None,
compatible_wheel: None,
incompatible_wheel: Some(DistRequiresPython {
dist,
requires_python,
}),
hashes: hash.map(|hash| vec![hash]).unwrap_or_default(),
}
}))
}
}

Expand All @@ -65,15 +69,15 @@ impl PrioritizedDistribution {
requires_python: Option<VersionSpecifiers>,
hash: Option<Hashes>,
) -> Self {
Self {
Self(Box::new(PrioritizedDistributionInner {
source: Some(DistRequiresPython {
dist,
requires_python,
}),
compatible_wheel: None,
incompatible_wheel: None,
hashes: hash.map(|hash| vec![hash]).unwrap_or_default(),
}
}))
}

/// Insert the given built distribution into the [`PrioritizedDistribution`].
Expand All @@ -86,9 +90,9 @@ impl PrioritizedDistribution {
) {
// Prefer the highest-priority, platform-compatible wheel.
if let Some(priority) = priority {
if let Some((.., existing_priority)) = &self.compatible_wheel {
if let Some((.., existing_priority)) = &self.0.compatible_wheel {
if priority > *existing_priority {
self.compatible_wheel = Some((
self.0.compatible_wheel = Some((
DistRequiresPython {
dist,
requires_python,
Expand All @@ -97,23 +101,23 @@ impl PrioritizedDistribution {
));
}
} else {
self.compatible_wheel = Some((
self.0.compatible_wheel = Some((
DistRequiresPython {
dist,
requires_python,
},
priority,
));
}
} else if self.incompatible_wheel.is_none() {
self.incompatible_wheel = Some(DistRequiresPython {
} else if self.0.incompatible_wheel.is_none() {
self.0.incompatible_wheel = Some(DistRequiresPython {
dist,
requires_python,
});
}

if let Some(hash) = hash {
self.hashes.push(hash);
self.0.hashes.push(hash);
}
}

Expand All @@ -124,24 +128,24 @@ impl PrioritizedDistribution {
requires_python: Option<VersionSpecifiers>,
hash: Option<Hashes>,
) {
if self.source.is_none() {
self.source = Some(DistRequiresPython {
if self.0.source.is_none() {
self.0.source = Some(DistRequiresPython {
dist,
requires_python,
});
}

if let Some(hash) = hash {
self.hashes.push(hash);
self.0.hashes.push(hash);
}
}

/// Return the highest-priority distribution for the package version, if any.
pub fn get(&self) -> Option<ResolvableDist> {
match (
&self.compatible_wheel,
&self.source,
&self.incompatible_wheel,
&self.0.compatible_wheel,
&self.0.source,
&self.0.incompatible_wheel,
) {
// Prefer the highest-priority, platform-compatible wheel.
(Some((wheel, tag_priority)), _, _) => {
Expand All @@ -162,17 +166,17 @@ impl PrioritizedDistribution {

/// Return the source distribution, if any.
pub fn source(&self) -> Option<&DistRequiresPython> {
self.source.as_ref()
self.0.source.as_ref()
}

/// Return the compatible built distribution, if any.
pub fn compatible_wheel(&self) -> Option<&(DistRequiresPython, TagPriority)> {
self.compatible_wheel.as_ref()
self.0.compatible_wheel.as_ref()
}

/// Return the hashes for each distribution.
pub fn hashes(&self) -> &[Hashes] {
&self.hashes
&self.0.hashes
}
}

Expand Down

0 comments on commit cd2fb6f

Please sign in to comment.