once-map: avoid hard-coding Arc (#3242)

The only thing a `OnceMap` really needs to be able to do with the value
is to clone it. All extant uses benefited from having this done for them
by automatically wrapping values in an `Arc`. But this isn't necessarily
true for all things. For example, a value might have an `Arc` internally
to making cloning cheap in other contexts, and it doesn't make sense to
re-wrap it in an `Arc` just to use it with a `OnceMap`. Or
alternatively, cloning might just be cheap enough on its own that an
`Arc` isn't worth it.
This commit is contained in:
Andrew Gallant 2024-04-24 11:11:46 -04:00 committed by GitHub
parent 67d8805ca8
commit 0b84eb0140
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
6 changed files with 33 additions and 25 deletions

View file

@ -11,11 +11,14 @@ use tokio::sync::Notify;
/// requests for metadata. When multiple tasks start the same query in parallel, e.g. through source /// requests for metadata. When multiple tasks start the same query in parallel, e.g. through source
/// dist builds, we want to wait until the other task is done and get a reference to the same /// dist builds, we want to wait until the other task is done and get a reference to the same
/// result. /// result.
pub struct OnceMap<K: Eq + Hash, V> { ///
/// Note that this always clones the value out of the underlying map. Because
/// of this, it's common to wrap the `V` in an `Arc<V>` to make cloning cheap.
pub struct OnceMap<K, V> {
items: DashMap<K, Value<V>>, items: DashMap<K, Value<V>>,
} }
impl<K: Eq + Hash, V> OnceMap<K, V> { impl<K: Eq + Hash, V: Clone> OnceMap<K, V> {
/// Register that you want to start a job. /// Register that you want to start a job.
/// ///
/// If this method returns `true`, you need to start a job and call [`OnceMap::done`] eventually /// If this method returns `true`, you need to start a job and call [`OnceMap::done`] eventually
@ -34,8 +37,7 @@ impl<K: Eq + Hash, V> OnceMap<K, V> {
/// Submit the result of a job you registered. /// Submit the result of a job you registered.
pub fn done(&self, key: K, value: V) { pub fn done(&self, key: K, value: V) {
if let Some(Value::Waiting(notify)) = self.items.insert(key, Value::Filled(Arc::new(value))) if let Some(Value::Waiting(notify)) = self.items.insert(key, Value::Filled(value)) {
{
notify.notify_waiters(); notify.notify_waiters();
} }
} }
@ -43,7 +45,7 @@ impl<K: Eq + Hash, V> OnceMap<K, V> {
/// Wait for the result of a job that is running. /// Wait for the result of a job that is running.
/// ///
/// Will hang if [`OnceMap::done`] isn't called for this key. /// Will hang if [`OnceMap::done`] isn't called for this key.
pub async fn wait(&self, key: &K) -> Option<Arc<V>> { pub async fn wait(&self, key: &K) -> Option<V> {
let entry = self.items.get(key)?; let entry = self.items.get(key)?;
match entry.value() { match entry.value() {
Value::Filled(value) => Some(value.clone()), Value::Filled(value) => Some(value.clone()),
@ -62,7 +64,7 @@ impl<K: Eq + Hash, V> OnceMap<K, V> {
} }
/// Return the result of a previous job, if any. /// Return the result of a previous job, if any.
pub fn get<Q: ?Sized + Hash + Eq>(&self, key: &Q) -> Option<Arc<V>> pub fn get<Q: ?Sized + Hash + Eq>(&self, key: &Q) -> Option<V>
where where
K: Borrow<Q>, K: Borrow<Q>,
{ {
@ -84,5 +86,5 @@ impl<K: Eq + Hash + Clone, V> Default for OnceMap<K, V> {
enum Value<V> { enum Value<V> {
Waiting(Arc<Notify>), Waiting(Arc<Notify>),
Filled(Arc<V>), Filled(V),
} }

View file

@ -312,13 +312,12 @@ impl AuthMiddleware {
); );
if !self.cache().fetches.register(key.clone()) { if !self.cache().fetches.register(key.clone()) {
let credentials = Arc::<_>::unwrap_or_clone( let credentials = self
self.cache() .cache()
.fetches .fetches
.wait(&key) .wait(&key)
.await .await
.expect("The key must exist after register is called"), .expect("The key must exist after register is called");
);
if credentials.is_some() { if credentials.is_some() {
trace!("Using credentials from previous fetch for {url}"); trace!("Using credentials from previous fetch for {url}");

View file

@ -232,7 +232,7 @@ impl NoSolutionError {
mut self, mut self,
python_requirement: &PythonRequirement, python_requirement: &PythonRequirement,
visited: &DashSet<PackageName>, visited: &DashSet<PackageName>,
package_versions: &OnceMap<PackageName, VersionsResponse>, package_versions: &OnceMap<PackageName, Arc<VersionsResponse>>,
) -> Self { ) -> Self {
let mut available_versions = IndexMap::default(); let mut available_versions = IndexMap::default();
for package in self.derivation_tree.packages() { for package in self.derivation_tree.packages() {

View file

@ -1,5 +1,6 @@
use std::borrow::Cow; use std::borrow::Cow;
use std::hash::BuildHasherDefault; use std::hash::BuildHasherDefault;
use std::sync::Arc;
use anyhow::Result; use anyhow::Result;
use itertools::Itertools; use itertools::Itertools;
@ -71,8 +72,8 @@ impl ResolutionGraph {
pub(crate) fn from_state( pub(crate) fn from_state(
selection: &SelectedDependencies<UvDependencyProvider>, selection: &SelectedDependencies<UvDependencyProvider>,
pins: &FilePins, pins: &FilePins,
packages: &OnceMap<PackageName, VersionsResponse>, packages: &OnceMap<PackageName, Arc<VersionsResponse>>,
distributions: &OnceMap<VersionId, MetadataResponse>, distributions: &OnceMap<VersionId, Arc<MetadataResponse>>,
state: &State<UvDependencyProvider>, state: &State<UvDependencyProvider>,
preferences: &Preferences, preferences: &Preferences,
editables: Editables, editables: Editables,

View file

@ -11,21 +11,21 @@ use crate::resolver::provider::{MetadataResponse, VersionsResponse};
pub struct InMemoryIndex { pub struct InMemoryIndex {
/// A map from package name to the metadata for that package and the index where the metadata /// A map from package name to the metadata for that package and the index where the metadata
/// came from. /// came from.
pub(crate) packages: OnceMap<PackageName, VersionsResponse>, pub(crate) packages: OnceMap<PackageName, Arc<VersionsResponse>>,
/// A map from package ID to metadata for that distribution. /// A map from package ID to metadata for that distribution.
pub(crate) distributions: OnceMap<VersionId, MetadataResponse>, pub(crate) distributions: OnceMap<VersionId, Arc<MetadataResponse>>,
} }
impl InMemoryIndex { impl InMemoryIndex {
/// Insert a [`VersionsResponse`] into the index. /// Insert a [`VersionsResponse`] into the index.
pub fn insert_package(&self, package_name: PackageName, response: VersionsResponse) { pub fn insert_package(&self, package_name: PackageName, response: VersionsResponse) {
self.packages.done(package_name, response); self.packages.done(package_name, Arc::new(response));
} }
/// Insert a [`Metadata23`] into the index. /// Insert a [`Metadata23`] into the index.
pub fn insert_metadata(&self, version_id: VersionId, response: MetadataResponse) { pub fn insert_metadata(&self, version_id: VersionId, response: MetadataResponse) {
self.distributions.done(version_id, response); self.distributions.done(version_id, Arc::new(response));
} }
/// Get the [`VersionsResponse`] for a given package name, without waiting. /// Get the [`VersionsResponse`] for a given package name, without waiting.

View file

@ -1046,13 +1046,15 @@ impl<
match response? { match response? {
Some(Response::Package(package_name, version_map)) => { Some(Response::Package(package_name, version_map)) => {
trace!("Received package metadata for: {package_name}"); trace!("Received package metadata for: {package_name}");
self.index.packages.done(package_name, version_map); self.index
.packages
.done(package_name, Arc::new(version_map));
} }
Some(Response::Installed { dist, metadata }) => { Some(Response::Installed { dist, metadata }) => {
trace!("Received installed distribution metadata for: {dist}"); trace!("Received installed distribution metadata for: {dist}");
self.index.distributions.done( self.index.distributions.done(
dist.version_id(), dist.version_id(),
MetadataResponse::Found(ArchiveMetadata::from(metadata)), Arc::new(MetadataResponse::Found(ArchiveMetadata::from(metadata))),
); );
} }
Some(Response::Dist { Some(Response::Dist {
@ -1069,7 +1071,9 @@ impl<
} }
_ => {} _ => {}
} }
self.index.distributions.done(dist.version_id(), metadata); self.index
.distributions
.done(dist.version_id(), Arc::new(metadata));
} }
Some(Response::Dist { Some(Response::Dist {
dist: Dist::Source(dist), dist: Dist::Source(dist),
@ -1085,7 +1089,9 @@ impl<
} }
_ => {} _ => {}
} }
self.index.distributions.done(dist.version_id(), metadata); self.index
.distributions
.done(dist.version_id(), Arc::new(metadata));
} }
None => {} None => {}
} }