Skip to content

Commit

Permalink
refactor(dependencies): share package using Rc instead of cloning ♻️
Browse files Browse the repository at this point in the history
Although Rc is used, it is still cloned in
dependency::DependencyProvider as it needs to sort the releases
  • Loading branch information
andho committed Dec 28, 2024
1 parent c4c8340 commit 22b1135
Show file tree
Hide file tree
Showing 2 changed files with 28 additions and 18 deletions.
15 changes: 9 additions & 6 deletions compiler-cli/src/dependencies.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use std::{
cell::RefCell,
collections::{HashMap, HashSet},
rc::Rc,
time::Instant,
};

Expand Down Expand Up @@ -1045,7 +1046,7 @@ async fn lookup_package(
}

struct PackageFetcher {
runtime_cache: RefCell<HashMap<String, hexpm::Package>>,
runtime_cache: RefCell<HashMap<String, Rc<hexpm::Package>>>,
runtime: tokio::runtime::Handle,
http: HttpClient,
}
Expand All @@ -1062,7 +1063,7 @@ impl PackageFetcher {
/// Caches the result of `get_dependencies` so that we don't need to make a network request.
/// Currently dependencies are fetched during initial version resolution, and then during check
/// for major version availability.
fn cache_package(&self, package: &str, result: hexpm::Package) {
fn cache_package(&self, package: &str, result: Rc<hexpm::Package>) {
let mut runtime_cache = self.runtime_cache.borrow_mut();
let _ = runtime_cache.insert(package.to_string(), result);
}
Expand Down Expand Up @@ -1098,7 +1099,7 @@ impl dependency::PackageFetcher for PackageFetcher {
fn get_dependencies(
&self,
package: &str,
) -> Result<hexpm::Package, Box<dyn std::error::Error>> {
) -> Result<Rc<hexpm::Package>, Box<dyn std::error::Error>> {
{
let runtime_cache = self.runtime_cache.borrow();
let result = runtime_cache.get(package);
Expand All @@ -1117,9 +1118,11 @@ impl dependency::PackageFetcher for PackageFetcher {
.map_err(Box::new)?;

match hexpm::get_package_response(response, HEXPM_PUBLIC_KEY) {
Ok(a) => {
self.cache_package(package, a.clone());
Ok(a)
Ok(pkg) => {
let pkg = Rc::new(pkg);
let pkg_ref = Rc::clone(&pkg);
self.cache_package(package, pkg);
Ok(pkg_ref)
}
Err(e) => match e {
hexpm::ApiError::NotFound => {
Expand Down
31 changes: 19 additions & 12 deletions compiler-core/src/dependency.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use std::{borrow::Borrow, cell::RefCell, collections::HashMap, error::Error as StdError};
use std::{borrow::Borrow, cell::RefCell, collections::HashMap, error::Error as StdError, rc::Rc};

use crate::{manifest, Error, Result};

Expand Down Expand Up @@ -197,7 +197,7 @@ but it is locked to {locked_version}, which is incompatible.",
}

pub trait PackageFetcher {
fn get_dependencies(&self, package: &str) -> Result<hexpm::Package, Box<dyn StdError>>;
fn get_dependencies(&self, package: &str) -> Result<Rc<hexpm::Package>, Box<dyn StdError>>;
}

struct DependencyProvider<'a, T: PackageFetcher> {
Expand Down Expand Up @@ -247,7 +247,9 @@ where
) -> Result<(), Box<dyn StdError>> {
let mut packages = self.packages.borrow_mut();
if packages.get(name).is_none() {
let mut package = self.remote.get_dependencies(name)?;
let package = self.remote.get_dependencies(name)?;
// mut (therefore clone) is required here in order to sort the releases
let mut package = (*package).clone();
// Sort the packages from newest to oldest, pres after all others
package.releases.sort_by(|a, b| a.version.cmp(&b.version));
package.releases.reverse();
Expand Down Expand Up @@ -362,14 +364,14 @@ mod tests {
use super::*;

struct Remote {
deps: HashMap<String, hexpm::Package>,
deps: HashMap<String, Rc<hexpm::Package>>,
}

impl PackageFetcher for Remote {
fn get_dependencies(&self, package: &str) -> Result<hexpm::Package, Box<dyn StdError>> {
fn get_dependencies(&self, package: &str) -> Result<Rc<hexpm::Package>, Box<dyn StdError>> {
self.deps
.get(package)
.cloned()
.map(Rc::clone)
.ok_or(Box::new(hexpm::ApiError::NotFound))
}
}
Expand Down Expand Up @@ -411,7 +413,8 @@ mod tests {
meta: (),
},
],
},
}
.into(),
);
let _ = deps.insert(
"gleam_otp".into(),
Expand Down Expand Up @@ -484,7 +487,8 @@ mod tests {
meta: (),
},
],
},
}
.into(),
);
let _ = deps.insert(
"package_with_retired".into(),
Expand All @@ -510,7 +514,8 @@ mod tests {
meta: (),
},
],
},
}
.into(),
);

let _ = deps.insert(
Expand All @@ -534,7 +539,8 @@ mod tests {
outer_checksum: vec![1, 2, 3],
meta: (),
}],
},
}
.into(),
);

let simple_deps_for_major_version_check = vec![
Expand Down Expand Up @@ -577,7 +583,7 @@ mod tests {

fn insert_simplified_deps(
simple_deps: Vec<(&str, Vec<(&str, Vec<(&str, &str)>)>)>,
deps: &mut HashMap<String, hexpm::Package>,
deps: &mut HashMap<String, Rc<hexpm::Package>>,
) {
for (name, releases) in simple_deps {
let _ = deps.insert(
Expand Down Expand Up @@ -608,7 +614,8 @@ mod tests {
meta: (),
})
.collect(),
},
}
.into(),
);
}
}
Expand Down

0 comments on commit 22b1135

Please sign in to comment.