Minor optimization (#544)

* Optimization: Rm `debug!` in `find_version`
   printing all version iterated obviously doesn't help much in debugging
   in the problem but rather just confusing.
   
   Also this makes it hard for the compiler to optimize the iterators.
* Use let-else in `ManifestVisitor`
* Optimize `BinFile::preview_{bin, link}` for zero-copy
   Return `impl Display` that lazily format instead of allocating a `String`
* Optimize `infer_bin_dir_template`: Generate dir lazily
* Optimize `find_version`: Lazily clone `version_req` only on err
* Refactor `find_version`: Use `bool::then_some`
* Add dep either v1.8.0 to binstalk
* Optimize `GhCrateMeta::find`: Avoid cloning and `Vec` creation
   by using `Either`
* Optimize `ops::install::install_from_package`: Make it a regular fn
   instead of async fn since it does not `.await` on any async fn.
* Optimize `QuickInstall`: Rm field `target`
   since `Arc<Data>` already contains that field.
* Optimize `Fetcher`s: Extract new struct `TargetData`
   so that `Data` can be shared by all fetchers, regardless of the target.
* Optimize `QuickInstall`: Rm unused field `data`
* Optimize `Resolution::print`: Replace branching with conditional move

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
This commit is contained in:
Jiahao XU 2022-11-21 08:32:46 +11:00 committed by GitHub
parent 696d8c2a82
commit bdb4b2070d
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
11 changed files with 145 additions and 128 deletions

View file

@ -17,6 +17,7 @@ cargo_toml = "0.13.0"
compact_str = { version = "0.6.0", features = ["serde"] }
crates_io_api = { version = "0.8.1", default-features = false }
detect-targets = { version = "0.1.2", path = "../detect-targets" }
either = "1.8.0"
futures-util = { version = "0.3.25", default-features = false, features = ["std"] }
home = "0.5.4"
itertools = "0.10.5"

View file

@ -1,7 +1,7 @@
use std::{
borrow::Cow,
fs,
path::{Component, Path, PathBuf},
fmt, fs,
path::{self, Component, Path, PathBuf},
};
use compact_str::CompactString;
@ -31,29 +31,30 @@ fn is_valid_path(path: &Path) -> bool {
/// Must be called after the archive is downloaded and extracted.
/// This function might uses blocking I/O.
pub fn infer_bin_dir_template(data: &Data) -> Cow<'static, str> {
let name = &data.name;
let target = &data.target;
let version = &data.version;
let name = data.name;
let target = data.target;
let version = data.version;
// Make sure to update
// fetchers::gh_crate_meta::hosting::{FULL_FILENAMES,
// NOVERSION_FILENAMES} if you update this array.
let possible_dirs = [
format!("{name}-{target}-v{version}"),
format!("{name}-{target}-{version}"),
format!("{name}-{version}-{target}"),
format!("{name}-v{version}-{target}"),
format!("{name}-{target}"),
let gen_possible_dirs: [for<'r> fn(&'r str, &'r str, &'r str) -> String; 8] = [
|name, target, version| format!("{name}-{target}-v{version}"),
|name, target, version| format!("{name}-{target}-{version}"),
|name, target, version| format!("{name}-{version}-{target}"),
|name, target, version| format!("{name}-v{version}-{target}"),
|name, target, _version| format!("{name}-{target}"),
// Ignore the following when updating hosting::{FULL_FILENAMES, NOVERSION_FILENAMES}
format!("{name}-{version}"),
format!("{name}-v{version}"),
name.to_string(),
|name, _target, version| format!("{name}-{version}"),
|name, _target, version| format!("{name}-v{version}"),
|name, _target, _version| name.to_string(),
];
let default_bin_dir_template = Cow::Borrowed("{ bin }{ binary-ext }");
possible_dirs
gen_possible_dirs
.into_iter()
.map(|gen_possible_dir| gen_possible_dir(name, target, version))
.find(|dirname| data.bin_path.join(dirname).is_dir())
.map(|mut dir| {
dir.reserve_exact(1 + default_bin_dir_template.len());
@ -138,26 +139,20 @@ impl BinFile {
})
}
pub fn preview_bin(&self) -> String {
format!(
"{} ({} -> {})",
self.base_name,
self.source.file_name().unwrap().to_string_lossy(),
self.dest.display()
)
pub fn preview_bin(&self) -> impl fmt::Display + '_ {
LazyFormat {
base_name: &self.base_name,
source: self.source.file_name().unwrap().to_string_lossy(),
dest: self.dest.display(),
}
}
pub fn preview_link(&self) -> String {
if let Some(link) = &self.link {
format!(
"{} ({} -> {})",
self.base_name,
link.display(),
self.link_dest().display()
)
} else {
String::new()
}
pub fn preview_link(&self) -> impl fmt::Display + '_ {
OptionalLazyFormat(self.link.as_ref().map(|link| LazyFormat {
base_name: &self.base_name,
source: link.display(),
dest: self.link_dest().display(),
}))
}
/// Return `Ok` if the source exists, otherwise `Err`.
@ -253,3 +248,27 @@ impl<'c> Context<'c> {
Ok(tt.render("path", self)?)
}
}
struct LazyFormat<'a, S: fmt::Display> {
base_name: &'a str,
source: S,
dest: path::Display<'a>,
}
impl<S: fmt::Display> fmt::Display for LazyFormat<'_, S> {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
write!(f, "{} ({} -> {})", self.base_name, self.source, self.dest)
}
}
struct OptionalLazyFormat<'a, S: fmt::Display>(Option<LazyFormat<'a, S>>);
impl<S: fmt::Display> fmt::Display for OptionalLazyFormat<'_, S> {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
if let Some(lazy_format) = self.0.as_ref() {
fmt::Display::fmt(lazy_format, f)
} else {
Ok(())
}
}
}

View file

@ -43,9 +43,8 @@ impl TarEntriesVisitor for ManifestVisitor {
let path = entry.path()?;
let path = path.normalize();
let path = if let Ok(path) = path.strip_prefix(&self.manifest_dir_path) {
path
} else {
let Ok(path) = path.strip_prefix(&self.manifest_dir_path)
else {
// The path is outside of the curr dir (manifest dir),
// ignore it.
continue;

View file

@ -1,5 +1,4 @@
use semver::VersionReq;
use tracing::debug;
use crate::errors::BinstallError;
@ -37,16 +36,11 @@ pub(super) fn find_version<Item: Version, VersionIter: Iterator<Item = Item>>(
let ver = item.get_version()?;
// Filter by version match
if version_req.matches(&ver) {
debug!("Version: {:?}", ver);
Some((item, ver))
} else {
None
}
version_req.matches(&ver).then_some((item, ver))
})
// Return highest version
.max_by(|(_item_x, ver_x), (_item_y, ver_y)| ver_x.cmp(ver_y))
.ok_or(BinstallError::VersionMismatch {
.ok_or_else(|| BinstallError::VersionMismatch {
req: version_req.clone(),
})
}

View file

@ -17,7 +17,7 @@ pub(crate) mod quickinstall;
pub trait Fetcher: Send + Sync {
/// Create a new fetcher from some data
#[allow(clippy::new_ret_no_self)]
fn new(client: &Client, data: &Arc<Data>) -> Arc<dyn Fetcher>
fn new(client: Client, data: Arc<Data>, target_data: Arc<TargetData>) -> Arc<dyn Fetcher>
where
Self: Sized;
@ -61,8 +61,13 @@ pub trait Fetcher: Send + Sync {
#[derive(Clone, Debug)]
pub struct Data {
pub name: CompactString,
pub target: String,
pub version: CompactString,
pub repo: Option<String>,
}
/// Target specific data required to fetch a package
#[derive(Clone, Debug)]
pub struct TargetData {
pub target: String,
pub meta: PkgMeta,
}

View file

@ -1,6 +1,7 @@
use std::{future::Future, path::Path, sync::Arc};
use std::{future::Future, iter, ops::Deref, path::Path, sync::Arc};
use compact_str::{CompactString, ToCompactString};
use either::Either;
use futures_util::stream::{FuturesUnordered, StreamExt};
use once_cell::sync::OnceCell;
use serde::Serialize;
@ -19,7 +20,7 @@ use crate::{
manifests::cargo_toml_binstall::{PkgFmt, PkgMeta},
};
use super::Data;
use super::{Data, TargetData};
pub(crate) mod hosting;
use hosting::RepositoryHost;
@ -27,6 +28,7 @@ use hosting::RepositoryHost;
pub struct GhCrateMeta {
client: Client,
data: Arc<Data>,
target_data: Arc<TargetData>,
resolution: OnceCell<(Url, PkgFmt)>,
}
@ -41,7 +43,7 @@ impl GhCrateMeta {
) -> impl Iterator<Item = impl Future<Output = FindTaskRes> + 'a> + 'a {
// build up list of potential URLs
let urls = pkg_fmt.extensions().iter().filter_map(move |ext| {
let ctx = Context::from_data_with_repo(&self.data, ext, repo);
let ctx = Context::from_data_with_repo(&self.data, &self.target_data.target, ext, repo);
match ctx.render_url(pkg_url) {
Ok(url) => Some(url),
Err(err) => {
@ -68,10 +70,15 @@ impl GhCrateMeta {
#[async_trait::async_trait]
impl super::Fetcher for GhCrateMeta {
fn new(client: &Client, data: &Arc<Data>) -> Arc<dyn super::Fetcher> {
fn new(
client: Client,
data: Arc<Data>,
target_data: Arc<TargetData>,
) -> Arc<dyn super::Fetcher> {
Arc::new(Self {
client: client.clone(),
data: data.clone(),
client,
data,
target_data,
resolution: OnceCell::new(),
})
}
@ -87,20 +94,20 @@ impl super::Fetcher for GhCrateMeta {
None
};
let pkg_urls = if let Some(pkg_url) = self.data.meta.pkg_url.clone() {
vec![pkg_url]
let pkg_urls = if let Some(pkg_url) = self.target_data.meta.pkg_url.as_deref() {
Either::Left(pkg_url)
} else if let Some(repo) = repo.as_ref() {
if let Some(pkg_urls) =
RepositoryHost::guess_git_hosting_services(repo)?.get_default_pkg_url_template()
{
pkg_urls
Either::Right(pkg_urls)
} else {
warn!(
concat!(
"Unknown repository {}, cargo-binstall cannot provide default pkg_url for it.\n",
"Please ask the upstream to provide it for target {}."
),
repo, self.data.target
repo, self.target_data.target
);
return Ok(false);
@ -111,7 +118,7 @@ impl super::Fetcher for GhCrateMeta {
"Package does not specify repository, cargo-binstall cannot provide default pkg_url for it.\n",
"Please ask the upstream to provide it for target {}."
),
self.data.target
self.target_data.target
);
return Ok(false);
@ -119,12 +126,15 @@ impl super::Fetcher for GhCrateMeta {
let repo = repo.as_ref().map(|u| u.as_str().trim_end_matches('/'));
let launch_baseline_find_tasks = |pkg_fmt| {
pkg_urls
.iter()
.flat_map(move |pkg_url| self.launch_baseline_find_tasks(pkg_fmt, pkg_url, repo))
match &pkg_urls {
Either::Left(pkg_url) => Either::Left(iter::once(*pkg_url)),
Either::Right(pkg_urls) => Either::Right(pkg_urls.iter().map(Deref::deref)),
}
.flat_map(move |pkg_url| self.launch_baseline_find_tasks(pkg_fmt, pkg_url, repo))
};
let mut handles: FuturesUnordered<_> = if let Some(pkg_fmt) = self.data.meta.pkg_fmt {
let mut handles: FuturesUnordered<_> = if let Some(pkg_fmt) = self.target_data.meta.pkg_fmt
{
launch_baseline_find_tasks(pkg_fmt).collect()
} else {
PkgFmt::iter()
@ -156,7 +166,7 @@ impl super::Fetcher for GhCrateMeta {
}
fn target_meta(&self) -> PkgMeta {
let mut meta = self.data.meta.clone();
let mut meta = self.target_data.meta.clone();
meta.pkg_fmt = Some(self.pkg_fmt());
meta
}
@ -185,7 +195,7 @@ impl super::Fetcher for GhCrateMeta {
}
fn target(&self) -> &str {
&self.data.target
&self.target_data.target
}
}
@ -215,6 +225,7 @@ struct Context<'c> {
impl<'c> Context<'c> {
pub(self) fn from_data_with_repo(
data: &'c Data,
target: &'c str,
archive_suffix: &'c str,
repo: Option<&'c str>,
) -> Self {
@ -230,12 +241,12 @@ impl<'c> Context<'c> {
Self {
name: &data.name,
repo,
target: &data.target,
target,
version: &data.version,
format: archive_format,
archive_format,
archive_suffix,
binary_ext: if data.target.contains("windows") {
binary_ext: if target.contains("windows") {
".exe"
} else {
""
@ -244,8 +255,8 @@ impl<'c> Context<'c> {
}
#[cfg(test)]
pub(self) fn from_data(data: &'c Data, archive_format: &'c str) -> Self {
Self::from_data_with_repo(data, archive_format, data.repo.as_deref())
pub(self) fn from_data(data: &'c Data, target: &'c str, archive_format: &'c str) -> Self {
Self::from_data_with_repo(data, target, archive_format, data.repo.as_deref())
}
pub(self) fn render_url(&self, template: &str) -> Result<Url, BinstallError> {
@ -273,16 +284,13 @@ mod test {
#[test]
fn defaults() {
let meta = PkgMeta::default();
let data = Data {
name: "cargo-binstall".to_compact_string(),
target: "x86_64-unknown-linux-gnu".to_string(),
version: "1.2.3".to_compact_string(),
repo: Some("https://github.com/ryankurte/cargo-binstall".to_string()),
meta,
};
let ctx = Context::from_data(&data, ".tgz");
let ctx = Context::from_data(&data, "x86_64-unknown-linux-gnu", ".tgz");
assert_eq!(
ctx.render_url(DEFAULT_PKG_URL).unwrap(),
url("https://github.com/ryankurte/cargo-binstall/releases/download/v1.2.3/cargo-binstall-x86_64-unknown-linux-gnu-v1.2.3.tgz")
@ -295,15 +303,12 @@ mod test {
let meta = PkgMeta::default();
let data = Data {
name: "cargo-binstall".to_compact_string(),
target: "x86_64-unknown-linux-gnu".to_string(),
version: "1.2.3".to_compact_string(),
repo: None,
meta,
};
let ctx = Context::from_data(&data, ".tgz");
ctx.render_url(data.meta.pkg_url.as_deref().unwrap())
.unwrap();
let ctx = Context::from_data(&data, "x86_64-unknown-linux-gnu", ".tgz");
ctx.render_url(meta.pkg_url.as_deref().unwrap()).unwrap();
}
#[test]
@ -315,15 +320,13 @@ mod test {
let data = Data {
name: "cargo-binstall".to_compact_string(),
target: "x86_64-unknown-linux-gnu".to_string(),
version: "1.2.3".to_compact_string(),
repo: None,
meta,
};
let ctx = Context::from_data(&data, ".tgz");
let ctx = Context::from_data(&data, "x86_64-unknown-linux-gnu", ".tgz");
assert_eq!(
ctx.render_url(data.meta.pkg_url.as_deref().unwrap()).unwrap(),
ctx.render_url(meta.pkg_url.as_deref().unwrap()).unwrap(),
url("https://example.com/releases/download/v1.2.3/cargo-binstall-x86_64-unknown-linux-gnu-v1.2.3.tgz")
);
}
@ -339,15 +342,13 @@ mod test {
let data = Data {
name: "radio-sx128x".to_compact_string(),
target: "x86_64-unknown-linux-gnu".to_string(),
version: "0.14.1-alpha.5".to_compact_string(),
repo: Some("https://github.com/rust-iot/rust-radio-sx128x".to_string()),
meta,
};
let ctx = Context::from_data(&data, ".tgz");
let ctx = Context::from_data(&data, "x86_64-unknown-linux-gnu", ".tgz");
assert_eq!(
ctx.render_url(data.meta.pkg_url.as_deref().unwrap()).unwrap(),
ctx.render_url(meta.pkg_url.as_deref().unwrap()).unwrap(),
url("https://github.com/rust-iot/rust-radio-sx128x/releases/download/v0.14.1-alpha.5/sx128x-util-x86_64-unknown-linux-gnu-v0.14.1-alpha.5.tgz")
);
}
@ -361,15 +362,13 @@ mod test {
let data = Data {
name: "radio-sx128x".to_compact_string(),
target: "x86_64-unknown-linux-gnu".to_string(),
version: "0.14.1-alpha.5".to_compact_string(),
repo: Some("https://github.com/rust-iot/rust-radio-sx128x".to_string()),
meta,
};
let ctx = Context::from_data(&data, ".tgz");
let ctx = Context::from_data(&data, "x86_64-unknown-linux-gnu", ".tgz");
assert_eq!(
ctx.render_url(data.meta.pkg_url.as_deref().unwrap()).unwrap(),
ctx.render_url(meta.pkg_url.as_deref().unwrap()).unwrap(),
url("https://github.com/rust-iot/rust-radio-sx128x/releases/download/v0.14.1-alpha.5/sx128x-util-x86_64-unknown-linux-gnu-v0.14.1-alpha.5.tgz")
);
}
@ -387,15 +386,13 @@ mod test {
let data = Data {
name: "cargo-watch".to_compact_string(),
target: "aarch64-apple-darwin".to_string(),
version: "9.0.0".to_compact_string(),
repo: Some("https://github.com/watchexec/cargo-watch".to_string()),
meta,
};
let ctx = Context::from_data(&data, ".txz");
let ctx = Context::from_data(&data, "aarch64-apple-darwin", ".txz");
assert_eq!(
ctx.render_url(data.meta.pkg_url.as_deref().unwrap()).unwrap(),
ctx.render_url(meta.pkg_url.as_deref().unwrap()).unwrap(),
url("https://github.com/watchexec/cargo-watch/releases/download/v9.0.0/cargo-watch-v9.0.0-aarch64-apple-darwin.tar.xz")
);
}
@ -410,15 +407,13 @@ mod test {
let data = Data {
name: "cargo-watch".to_compact_string(),
target: "aarch64-pc-windows-msvc".to_string(),
version: "9.0.0".to_compact_string(),
repo: Some("https://github.com/watchexec/cargo-watch".to_string()),
meta,
};
let ctx = Context::from_data(&data, ".bin");
let ctx = Context::from_data(&data, "aarch64-pc-windows-msvc", ".bin");
assert_eq!(
ctx.render_url(data.meta.pkg_url.as_deref().unwrap()).unwrap(),
ctx.render_url(meta.pkg_url.as_deref().unwrap()).unwrap(),
url("https://github.com/watchexec/cargo-watch/releases/download/v9.0.0/cargo-watch-v9.0.0-aarch64-pc-windows-msvc.exe")
);
}

View file

@ -15,7 +15,7 @@ use crate::{
manifests::cargo_toml_binstall::{PkgFmt, PkgMeta},
};
use super::Data;
use super::{Data, TargetData};
const BASE_URL: &str = "https://github.com/alsuren/cargo-quickinstall/releases/download";
const STATS_URL: &str = "https://warehouse-clerk-tmp.vercel.app/api/crate";
@ -23,21 +23,23 @@ const STATS_URL: &str = "https://warehouse-clerk-tmp.vercel.app/api/crate";
pub struct QuickInstall {
client: Client,
package: String,
target: String,
data: Arc<Data>,
target_data: Arc<TargetData>,
}
#[async_trait::async_trait]
impl super::Fetcher for QuickInstall {
fn new(client: &Client, data: &Arc<Data>) -> Arc<dyn super::Fetcher> {
fn new(
client: Client,
data: Arc<Data>,
target_data: Arc<TargetData>,
) -> Arc<dyn super::Fetcher> {
let crate_name = &data.name;
let version = &data.version;
let target = data.target.clone();
let target = &target_data.target;
Arc::new(Self {
client: client.clone(),
client,
package: format!("{crate_name}-{version}-{target}"),
target,
data: data.clone(),
target_data,
})
}
@ -68,7 +70,7 @@ impl super::Fetcher for QuickInstall {
}
fn target_meta(&self) -> PkgMeta {
let mut meta = self.data.meta.clone();
let mut meta = self.target_data.meta.clone();
meta.pkg_fmt = Some(self.pkg_fmt());
meta.bin_dir = Some("{ bin }{ binary-ext }".to_string());
meta
@ -87,7 +89,7 @@ impl super::Fetcher for QuickInstall {
}
fn target(&self) -> &str {
&self.target
&self.target_data.target
}
}

View file

@ -6,7 +6,7 @@ use crates_io_api::AsyncClient as CratesIoApiClient;
use semver::VersionReq;
use crate::{
fetchers::{Data, Fetcher},
fetchers::{Data, Fetcher, TargetData},
helpers::{jobserver_client::LazyJobserverClient, remote::Client},
manifests::cargo_toml_binstall::PkgOverride,
DesiredTargets,
@ -15,7 +15,7 @@ use crate::{
pub mod install;
pub mod resolve;
pub type Resolver = fn(&Client, &Arc<Data>) -> Arc<dyn Fetcher>;
pub type Resolver = fn(Client, Arc<Data>, Arc<TargetData>) -> Arc<dyn Fetcher>;
pub struct Options {
pub no_symlinks: bool,

View file

@ -28,7 +28,7 @@ pub async fn install(
} => {
let target = fetcher.target().into();
install_from_package(opts, bin_files).await.map(|option| {
install_from_package(opts, bin_files).map(|option| {
option.map(|bins| CrateInfo {
name,
version_req,
@ -66,7 +66,7 @@ pub async fn install(
}
}
async fn install_from_package(
fn install_from_package(
opts: Arc<Options>,
bin_files: Vec<bins::BinFile>,
) -> Result<Option<Vec<CompactString>>, BinstallError> {

View file

@ -19,7 +19,7 @@ use crate::{
bins,
drivers::fetch_crate_cratesio,
errors::BinstallError,
fetchers::{Data, Fetcher},
fetchers::{Data, Fetcher, TargetData},
helpers::{remote::Client, tasks::AutoAbortJoinHandle},
manifests::cargo_toml_binstall::{Meta, PkgMeta, PkgOverride},
};
@ -58,17 +58,15 @@ impl Resolution {
fetcher.source_name()
);
if fetcher.is_third_party() {
warn!(
"The package will be downloaded from third-party source {}",
fetcher.source_name()
);
} else {
info!(
"The package will be downloaded from {}",
fetcher.source_name()
);
}
warn!(
"The package will be downloaded from {}{}",
if fetcher.is_third_party() {
"third-party source "
} else {
""
},
fetcher.source_name()
);
info!("This will install the following binaries:");
for file in bin_files {
@ -138,6 +136,12 @@ async fn resolve_inner(
let mut handles: Vec<(Arc<dyn Fetcher>, _)> =
Vec::with_capacity(desired_targets.len() * resolvers.len());
let data = Arc::new(Data {
name: package_info.name.clone(),
version: package_info.version_str.clone(),
repo: package_info.repo.clone(),
});
handles.extend(
desired_targets
.iter()
@ -150,17 +154,14 @@ async fn resolve_inner(
debug!("Found metadata: {target_meta:?}");
Arc::new(Data {
name: package_info.name.clone(),
Arc::new(TargetData {
target: target.clone(),
version: package_info.version_str.clone(),
repo: package_info.repo.clone(),
meta: target_meta,
})
})
.cartesian_product(resolvers)
.map(|(fetcher_data, f)| {
let fetcher = f(&opts.client, &fetcher_data);
.map(|(target_data, f)| {
let fetcher = f(opts.client.clone(), data.clone(), target_data);
(
fetcher.clone(),
AutoAbortJoinHandle::spawn(async move { fetcher.find().await }),