Optimizations plus bug fix (#541)

* Optimize `Download::and_extract`: Avoid dup monomorphization
* Increase buffer size for binstall_crates_v1 to `4096 * 5`
* Optimize `opts::resolve`: Avoid unnecessary `clone`s
* Fix reserve in `opts::resolve`: Do not over-reserve
* Rename field `opts::Options::resolver` => `resolvers`
* Refactor: Extract new type `resolve::PackageInfo`
    - which makes `opts::resolve_inner` easier to understand
    - reduce number of parameters required for `download_extract_and_verify` and
      `collect_bin_files`
    - reducing size of future returned by `opts::resolve_inner` by dropping
      `cargo_toml::{Manifest, Package}` as early as possible since
      `Manifest` is 3000 Bytes large while `Package` is 600 Bytes large.
* Optimize `fetchers::Data`: Use `CompactString` for field name & version
   since they are usually small enough to fit in inlined version of
   `CompactString`.
* Optimize `gh_crate_meta`: Avoid unnecessary allocation
   in `RepositoryHost::get_default_pkg_url_template`.
* Refacator: Use `Itertools::cartesian_product` in `apply_filenames_to_paths`
* Optimize `ops::resolve`: Avoid unnecessary `clone` & reduce future size
   by calling `fetcher.target_meta()` to obtain final metadata after
   downloaded and extracted the binaries.
* Optimize `ops::resolve`: Avoid unnecessary allocation
   in `download_extract_and_verify`: Replace `Itertools::join` with
   `Itertools::format` to avoid allocating the string.
* Fix disabling cargo-install fallback
* Simplify `BinFile::from_product`: Takes `&str` instead of `&Product`
   since we only need `product.name`
* Rename `BinFile::from_product` => `BinFile::new`
* Refactor: Create newtype `ops::resolve::Bin`
   so that we don't need to `unwrap()` on `Product::name`
   and reduce memory usage.

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
This commit is contained in:
Jiahao XU 2022-11-18 10:59:35 +11:00 committed by GitHub
parent bb1f51a739
commit 325cb5cc19
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
9 changed files with 220 additions and 160 deletions

View file

@ -1,12 +1,12 @@
use std::{
borrow::Cow,
collections::BTreeSet,
collections::{BTreeMap, BTreeSet},
iter, mem,
path::{Path, PathBuf},
path::Path,
sync::Arc,
};
use cargo_toml::{Manifest, Package, Product};
use cargo_toml::Manifest;
use compact_str::{CompactString, ToCompactString};
use itertools::Itertools;
use semver::{Version, VersionReq};
@ -20,7 +20,7 @@ use crate::{
errors::BinstallError,
fetchers::{Data, Fetcher},
helpers::{remote::Client, tasks::AutoAbortJoinHandle},
manifests::cargo_toml_binstall::{Meta, PkgMeta},
manifests::cargo_toml_binstall::{Meta, PkgMeta, PkgOverride},
};
mod crate_name;
@ -128,70 +128,30 @@ async fn resolve_inner(
) -> Result<Resolution, BinstallError> {
info!("Resolving package: '{}'", crate_name);
let version_req: VersionReq = match (&crate_name.version_req, &opts.version_req) {
(Some(version), None) => version.clone(),
let version_req: VersionReq = match (crate_name.version_req, &opts.version_req) {
(Some(version), None) => version,
(None, Some(version)) => version.clone(),
(Some(_), Some(_)) => Err(BinstallError::SuperfluousVersionOption)?,
(None, None) => VersionReq::STAR,
};
// Fetch crate via crates.io, git, or use a local manifest path
// TODO: work out which of these to do based on `opts.name`
// TODO: support git-based fetches (whole repo name rather than just crate name)
let manifest = match opts.manifest_path.clone() {
Some(manifest_path) => load_manifest_path(manifest_path)?,
None => {
fetch_crate_cratesio(
client.clone(),
&crates_io_api_client,
&crate_name.name,
&version_req,
)
.await?
}
let version_req_str = version_req.to_compact_string();
let Some(package_info) = PackageInfo::resolve(opts,
crate_name.name,
curr_version,
version_req,
client.clone(),
crates_io_api_client).await?
else {
return Ok(Resolution::AlreadyUpToDate)
};
let package = manifest
.package
.ok_or_else(|| BinstallError::CargoTomlMissingPackage(crate_name.name.clone()))?;
let new_version =
Version::parse(package.version()).map_err(|err| BinstallError::VersionParse {
v: package.version().to_compact_string(),
err,
})?;
if let Some(curr_version) = curr_version {
if new_version == curr_version {
info!(
"{} v{curr_version} is already installed, use --force to override",
crate_name.name
);
return Ok(Resolution::AlreadyUpToDate);
}
}
let (mut meta, mut binaries) = (
package
.metadata
.as_ref()
.and_then(|m| m.binstall.clone())
.unwrap_or_default(),
manifest.bin,
);
binaries.retain(|product| product.name.is_some());
// Check binaries
if binaries.is_empty() {
return Err(BinstallError::UnspecifiedBinaries);
}
let desired_targets = opts.desired_targets.get().await;
let resolvers = &opts.resolvers;
let mut handles: Vec<(Arc<dyn Fetcher>, _)> = Vec::with_capacity(desired_targets.len() * 2);
let overrides = mem::take(&mut meta.overrides);
let mut handles: Vec<(Arc<dyn Fetcher>, _)> =
Vec::with_capacity(desired_targets.len() * resolvers.len());
handles.extend(
desired_targets
@ -199,20 +159,21 @@ async fn resolve_inner(
.map(|target| {
debug!("Building metadata for target: {target}");
let target_meta = meta
.merge_overrides(iter::once(&opts.cli_overrides).chain(overrides.get(target)));
let target_meta = package_info.meta.merge_overrides(
iter::once(&opts.cli_overrides).chain(package_info.overrides.get(target)),
);
debug!("Found metadata: {target_meta:?}");
Arc::new(Data {
name: package.name.clone(),
name: package_info.name.clone(),
target: target.clone(),
version: package.version().to_string(),
repo: package.repository().map(ToString::to_string),
version: package_info.version_str.clone(),
repo: package_info.repo.clone(),
meta: target_meta,
})
})
.cartesian_product(&opts.resolver)
.cartesian_product(resolvers)
.map(|(fetcher_data, f)| {
let fetcher = f(&client, &fetcher_data);
(
@ -228,7 +189,7 @@ async fn resolve_inner(
// Generate temporary binary path
let bin_path = temp_dir.join(format!(
"bin-{}-{}-{}",
crate_name.name,
package_info.name,
fetcher.target(),
fetcher.fetcher_name()
));
@ -236,9 +197,8 @@ async fn resolve_inner(
match download_extract_and_verify(
fetcher.as_ref(),
&bin_path,
&package,
&package_info,
&install_path,
&binaries,
opts.no_symlinks,
)
.await
@ -247,9 +207,9 @@ async fn resolve_inner(
if !bin_files.is_empty() {
return Ok(Resolution::Fetch {
fetcher,
new_version,
name: crate_name.name,
version_req: version_req.to_compact_string(),
new_version: package_info.version,
name: package_info.name,
version_req: version_req_str,
bin_files,
});
} else {
@ -283,29 +243,31 @@ async fn resolve_inner(
}
}
Ok(Resolution::InstallFromSource {
name: crate_name.name,
version: package.version().to_compact_string(),
})
if opts.cargo_install_fallback {
Ok(Resolution::InstallFromSource {
name: package_info.name,
version: package_info.version_str,
})
} else {
Err(BinstallError::NoFallbackToCargoInstall)
}
}
/// * `fetcher` - `fetcher.find()` must return `Ok(true)`.
/// * `binaries` - must not be empty
async fn download_extract_and_verify(
fetcher: &dyn Fetcher,
bin_path: &Path,
package: &Package<Meta>,
package_info: &PackageInfo,
install_path: &Path,
binaries: &[Product],
no_symlinks: bool,
) -> Result<Vec<bins::BinFile>, BinstallError> {
// Build final metadata
let meta = fetcher.target_meta();
// Download and extract it.
// If that fails, then ignore this fetcher.
fetcher.fetch_and_extract(bin_path).await?;
// Build final metadata
let meta = fetcher.target_meta();
#[cfg(incomplete)]
{
// Fetch and check package signature if available
@ -334,17 +296,17 @@ async fn download_extract_and_verify(
block_in_place(|| {
let bin_files = collect_bin_files(
fetcher,
package,
package_info,
meta,
binaries,
bin_path.to_path_buf(),
install_path.to_path_buf(),
bin_path,
install_path,
no_symlinks,
)?;
let name = &package.name;
let name = &package_info.name;
binaries
package_info
.binaries
.iter()
.zip(bin_files)
.filter_map(|(bin, bin_file)| {
@ -360,8 +322,8 @@ async fn download_extract_and_verify(
Some(Err(err))
} else {
// Optional, print a warning and continue.
let bin_name = bin.name.as_deref().unwrap();
let features = required_features.join(",");
let bin_name = bin.name.as_str();
let features = required_features.iter().format(",");
warn!(
"When resolving {name} bin {bin_name} is not found. \
But since it requies features {features}, this bin is ignored."
@ -375,23 +337,21 @@ async fn download_extract_and_verify(
})
}
/// * `binaries` - must not be empty
fn collect_bin_files(
fetcher: &dyn Fetcher,
package: &Package<Meta>,
package_info: &PackageInfo,
meta: PkgMeta,
binaries: &[Product],
bin_path: PathBuf,
install_path: PathBuf,
bin_path: &Path,
install_path: &Path,
no_symlinks: bool,
) -> Result<Vec<bins::BinFile>, BinstallError> {
// List files to be installed
// based on those found via Cargo.toml
let bin_data = bins::Data {
name: &package.name,
name: &package_info.name,
target: fetcher.target(),
version: package.version(),
repo: package.repository(),
version: &package_info.version_str,
repo: package_info.repo.as_deref(),
meta,
bin_path,
install_path,
@ -405,9 +365,10 @@ fn collect_bin_files(
.unwrap_or_else(|| bins::infer_bin_dir_template(&bin_data));
// Create bin_files
let bin_files = binaries
let bin_files = package_info
.binaries
.iter()
.map(|p| bins::BinFile::from_product(&bin_data, p, &bin_dir, no_symlinks))
.map(|bin| bins::BinFile::new(&bin_data, bin.name.as_str(), &bin_dir, no_symlinks))
.collect::<Result<Vec<_>, BinstallError>>()?;
let mut source_set = BTreeSet::new();
@ -423,6 +384,99 @@ fn collect_bin_files(
Ok(bin_files)
}
struct PackageInfo {
meta: PkgMeta,
binaries: Vec<Bin>,
name: CompactString,
version_str: CompactString,
version: Version,
repo: Option<String>,
overrides: BTreeMap<String, PkgOverride>,
}
struct Bin {
name: String,
required_features: Vec<String>,
}
impl PackageInfo {
/// Return `None` if already up-to-date.
async fn resolve(
opts: &Options,
name: CompactString,
curr_version: Option<Version>,
version_req: VersionReq,
client: Client,
crates_io_api_client: crates_io_api::AsyncClient,
) -> Result<Option<Self>, BinstallError> {
// Fetch crate via crates.io, git, or use a local manifest path
let manifest = match opts.manifest_path.as_ref() {
Some(manifest_path) => load_manifest_path(manifest_path)?,
None => {
fetch_crate_cratesio(client, &crates_io_api_client, &name, &version_req).await?
}
};
let Some(mut package) = manifest.package else {
return Err(BinstallError::CargoTomlMissingPackage(name));
};
let new_version_str = package.version().to_compact_string();
let new_version = match Version::parse(&new_version_str) {
Ok(new_version) => new_version,
Err(err) => {
return Err(BinstallError::VersionParse {
v: new_version_str,
err,
})
}
};
if let Some(curr_version) = curr_version {
if new_version == curr_version {
info!(
"{} v{curr_version} is already installed, use --force to override",
name
);
return Ok(None);
}
}
let (mut meta, binaries): (_, Vec<Bin>) = (
package
.metadata
.take()
.and_then(|mut m| m.binstall.take())
.unwrap_or_default(),
manifest
.bin
.into_iter()
.filter_map(|p| {
p.name.map(|name| Bin {
name,
required_features: p.required_features,
})
})
.collect(),
);
// Check binaries
if binaries.is_empty() {
Err(BinstallError::UnspecifiedBinaries)
} else {
Ok(Some(Self {
overrides: mem::take(&mut meta.overrides),
meta,
binaries,
name,
version_str: new_version_str,
version: new_version,
repo: package.repository().map(ToString::to_string),
}))
}
}
}
/// Load binstall metadata from the crate `Cargo.toml` at the provided path
pub fn load_manifest_path<P: AsRef<Path>>(
manifest_path: P,