Skip to content

Commit

Permalink
More mionr optimizations (rust-lang#553)
Browse files Browse the repository at this point in the history
* Optimize `BinFile::preview_bin`: Use `Path::display` instead of `to_string_lossy` to avoid allocation.
* Refactor: Rm useless `AsRef::as_ref` call
* Optimize `GhCrateMeta::find`: Reduce fut size by converting `Url` to `String`
   `Url` is much larger than `String`.
* Refactor `PackageInfo::resolve`: Destruct `Meta::binstall`
* Optimize `resolve::load_manifest_path`: Avoid allocation

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
  • Loading branch information
NobodyXu committed Nov 22, 2022
1 parent 707b173 commit 66abf1b
Show file tree
Hide file tree
Showing 4 changed files with 28 additions and 20 deletions.
12 changes: 6 additions & 6 deletions crates/binstalk/src/bins.rs
Expand Up @@ -142,7 +142,7 @@ impl BinFile {
pub fn preview_bin(&self) -> impl fmt::Display + '_ {
LazyFormat {
base_name: &self.base_name,
source: self.source.file_name().unwrap().to_string_lossy(),
source: Path::new(self.source.file_name().unwrap()).display(),
dest: self.dest.display(),
}
}
Expand Down Expand Up @@ -249,21 +249,21 @@ impl<'c> Context<'c> {
}
}

struct LazyFormat<'a, S: fmt::Display> {
struct LazyFormat<'a> {
base_name: &'a str,
source: S,
source: path::Display<'a>,
dest: path::Display<'a>,
}

impl<S: fmt::Display> fmt::Display for LazyFormat<'_, S> {
impl fmt::Display for LazyFormat<'_> {
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>>);
struct OptionalLazyFormat<'a>(Option<LazyFormat<'a>>);

impl<S: fmt::Display> fmt::Display for OptionalLazyFormat<'_, S> {
impl fmt::Display for OptionalLazyFormat<'_> {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
if let Some(lazy_format) = self.0.as_ref() {
fmt::Display::fmt(lazy_format, f)
Expand Down
15 changes: 8 additions & 7 deletions crates/binstalk/src/drivers/crates_io.rs
Expand Up @@ -33,13 +33,14 @@ pub async fn fetch_crate_cratesio(
debug!("Looking up crate information");

// Fetch online crate information
let base_info = crates_io_api_client
.get_crate(name.as_ref())
.await
.map_err(|err| BinstallError::CratesIoApi {
crate_name: name.into(),
err: Box::new(err),
})?;
let base_info =
crates_io_api_client
.get_crate(name)
.await
.map_err(|err| BinstallError::CratesIoApi {
crate_name: name.into(),
err: Box::new(err),
})?;

// Locate matching version
let version_iter = base_info.versions.iter().filter(|v| !v.yanked);
Expand Down
5 changes: 4 additions & 1 deletion crates/binstalk/src/fetchers/gh_crate_meta.rs
Expand Up @@ -124,7 +124,10 @@ impl super::Fetcher for GhCrateMeta {
return Ok(false);
};

let repo = repo.as_ref().map(|u| u.as_str().trim_end_matches('/'));
// Convert Option<Url> to Option<String> to reduce size of future.
let repo = repo.map(String::from);
let repo = repo.as_deref().map(|u| u.trim_end_matches('/'));

let launch_baseline_find_tasks = |pkg_fmt| {
match &pkg_urls {
Either::Left(pkg_url) => Either::Left(iter::once(*pkg_url)),
Expand Down
16 changes: 10 additions & 6 deletions crates/binstalk/src/ops/resolve.rs
Expand Up @@ -239,7 +239,10 @@ async fn resolve_inner(
}
}

/// * `fetcher` - `fetcher.find()` must return `Ok(true)`.
/// * `fetcher` - `fetcher.find()` must have returned `Ok(true)`.
///
/// Can return empty Vec if all `BinFile` is optional and does not exist
/// in the archive downloaded.
async fn download_extract_and_verify(
fetcher: &dyn Fetcher,
bin_path: &Path,
Expand Down Expand Up @@ -278,7 +281,7 @@ async fn download_extract_and_verify(
}
}

// Verify that all the bin_files exist
// Verify that all non-optional bin_files exist
block_in_place(|| {
let bin_files = collect_bin_files(
fetcher,
Expand Down Expand Up @@ -430,7 +433,7 @@ impl PackageInfo {
package
.metadata
.take()
.and_then(|mut m| m.binstall.take())
.and_then(|m| m.binstall)
.unwrap_or_default(),
manifest
.bin
Expand Down Expand Up @@ -465,12 +468,13 @@ impl PackageInfo {
pub fn load_manifest_path<P: AsRef<Path>>(
manifest_path: P,
) -> Result<Manifest<Meta>, BinstallError> {
let manifest_path = manifest_path.as_ref();

block_in_place(|| {
let manifest_path = manifest_path.as_ref();
let manifest_path = if manifest_path.is_dir() {
manifest_path.join("Cargo.toml")
Cow::Owned(manifest_path.join("Cargo.toml"))
} else if manifest_path.is_file() {
manifest_path.into()
Cow::Borrowed(manifest_path)
} else {
return Err(BinstallError::CargoManifestPath);
};
Expand Down

0 comments on commit 66abf1b

Please sign in to comment.