From 270bf08a2409920e9e68ec11905e3e6f94b9697d Mon Sep 17 00:00:00 2001 From: Jiahao XU Date: Thu, 26 Jan 2023 15:33:49 +1100 Subject: [PATCH] Optimize use of `tinytemplate` (#647) * Optimize `GhCrateMeta::find`: Cache compiled template `pkg_url` * Optimize `collect_bin_files`: Cache compiled template `bin_dir` * Refactor: Improve readability of `gh_crate_meta` * Optimize `GhCrateMeta::launch_baseline_find_tasks`: Dedup urls before checking * Optimize `BinFile::new`: Avoid one heap alloc when creating `dest` * Improve `warn!` msg in `GhCrateMeta::launch_baseline_find_tasks` Signed-off-by: Jiahao XU --- crates/binstalk/src/bins.rs | 24 ++++++--- crates/binstalk/src/fetchers/gh_crate_meta.rs | 50 +++++++++++++------ crates/binstalk/src/ops/resolve.rs | 7 ++- 3 files changed, 58 insertions(+), 23 deletions(-) diff --git a/crates/binstalk/src/bins.rs b/crates/binstalk/src/bins.rs index 38c42fef..5fefc6c6 100644 --- a/crates/binstalk/src/bins.rs +++ b/crates/binstalk/src/bins.rs @@ -74,10 +74,11 @@ pub struct BinFile { } impl BinFile { + /// * `tt` - must have a template with name "bin_dir" pub fn new( data: &Data<'_>, base_name: &str, - bin_dir: &str, + tt: &TinyTemplate, no_symlinks: bool, ) -> Result { let binary_ext = if data.target.contains("windows") { @@ -101,7 +102,7 @@ impl BinFile { } else { // Generate install paths // Source path is the download dir + the generated binary path - let path = ctx.render(bin_dir)?; + let path = ctx.render_with_compiled_tt(tt)?; let path_normalized = Path::new(&path).normalize(); @@ -119,13 +120,21 @@ impl BinFile { }; // Destination at install dir + base-name{.extension} - let dest = data.install_path.join(ctx.render("{ bin }{ binary-ext }")?); + let mut dest = data.install_path.join(ctx.bin); + if !binary_ext.is_empty() { + let binary_ext = binary_ext.strip_prefix('.').unwrap(); + + // PathBuf::set_extension returns false if Path::file_name + // is None, but we know that the file name must be Some, + // thus we assert! the return value here. + assert!(dest.set_extension(binary_ext)); + } let (dest, link) = if no_symlinks { (dest, None) } else { // Destination path is the install dir + base-name-version{.extension} - let dest_file_path_with_ver = ctx.render("{ bin }-v{ version }{ binary-ext }")?; + let dest_file_path_with_ver = format!("{}-v{}{}", ctx.bin, ctx.version, ctx.binary_ext); let dest_with_ver = data.install_path.join(dest_file_path_with_ver); (dest_with_ver, Some(dest)) @@ -235,10 +244,9 @@ struct Context<'c> { } impl<'c> Context<'c> { - fn render(&self, template: &str) -> Result { - let mut tt = TinyTemplate::new(); - tt.add_template("path", template)?; - Ok(tt.render("path", self)?) + /// * `tt` - must have a template with name "bin_dir" + fn render_with_compiled_tt(&self, tt: &TinyTemplate) -> Result { + Ok(tt.render("bin_dir", self)?) } } diff --git a/crates/binstalk/src/fetchers/gh_crate_meta.rs b/crates/binstalk/src/fetchers/gh_crate_meta.rs index 72c467ea..bde0af83 100644 --- a/crates/binstalk/src/fetchers/gh_crate_meta.rs +++ b/crates/binstalk/src/fetchers/gh_crate_meta.rs @@ -3,6 +3,7 @@ use std::{borrow::Cow, future::Future, iter, path::Path, sync::Arc}; use compact_str::{CompactString, ToCompactString}; use either::Either; use futures_util::stream::{FuturesUnordered, StreamExt}; +use itertools::Itertools; use once_cell::sync::OnceCell; use serde::Serialize; use strum::IntoEnumIterator; @@ -35,23 +36,30 @@ pub struct GhCrateMeta { type FindTaskRes = Result, BinstallError>; impl GhCrateMeta { + /// * `tt` - must have added a template named "pkg_url". fn launch_baseline_find_tasks<'a>( &'a self, pkg_fmt: PkgFmt, + tt: &'a TinyTemplate, pkg_url: &'a str, repo: Option<&'a str>, ) -> impl Iterator + 'static> + '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, &self.target_data.target, ext, repo); - match ctx.render_url(pkg_url) { - Ok(url) => Some(url), - Err(err) => { - warn!("Failed to render url for {ctx:#?}: {err:#?}"); - None + let urls = pkg_fmt + .extensions() + .iter() + .filter_map(move |ext| { + let ctx = + Context::from_data_with_repo(&self.data, &self.target_data.target, ext, repo); + match ctx.render_url_with_compiled_tt(tt, pkg_url) { + Ok(url) => Some(url), + Err(err) => { + warn!("Failed to render url for {ctx:#?}: {err}"); + None + } } - } - }); + }) + .dedup(); // go check all potential URLs at once urls.map(move |url| { @@ -139,12 +147,16 @@ impl super::Fetcher for GhCrateMeta { // Iterate over pkg_urls first to avoid String::clone. for pkg_url in pkg_urls { + let mut tt = TinyTemplate::new(); + + tt.add_template("pkg_url", &pkg_url)?; + // Clone iter pkg_fmts to ensure all pkg_fmts is // iterated over for each pkg_url, which is // basically cartesian product. // | for pkg_fmt in pkg_fmts.clone() { - handles.extend(this.launch_baseline_find_tasks(pkg_fmt, &pkg_url, repo)); + handles.extend(this.launch_baseline_find_tasks(pkg_fmt, &tt, &pkg_url, repo)); } } @@ -266,12 +278,22 @@ impl<'c> Context<'c> { Self::from_data_with_repo(data, target, archive_format, data.repo.as_deref()) } - pub(self) fn render_url(&self, template: &str) -> Result { - debug!("Render {template:?} using context: {:?}", self); + /// * `tt` - must have added a template named "pkg_url". + pub(self) fn render_url_with_compiled_tt( + &self, + tt: &TinyTemplate, + template: &str, + ) -> Result { + debug!("Render {template} using context: {self:?}"); + Ok(Url::parse(&tt.render("pkg_url", self)?)?) + } + + #[cfg(test)] + pub(self) fn render_url(&self, template: &str) -> Result { let mut tt = TinyTemplate::new(); - tt.add_template("path", template)?; - Ok(Url::parse(&tt.render("path", self)?)?) + tt.add_template("pkg_url", template)?; + self.render_url_with_compiled_tt(&tt, template) } } diff --git a/crates/binstalk/src/ops/resolve.rs b/crates/binstalk/src/ops/resolve.rs index 1e5c5f03..4140b062 100644 --- a/crates/binstalk/src/ops/resolve.rs +++ b/crates/binstalk/src/ops/resolve.rs @@ -12,6 +12,7 @@ use crates_io_api::AsyncClient as CratesIoApiClient; use itertools::Itertools; use maybe_owned::MaybeOwned; use semver::{Version, VersionReq}; +use tinytemplate::TinyTemplate; use tokio::task::block_in_place; use tracing::{debug, info, instrument, warn}; @@ -297,11 +298,15 @@ fn collect_bin_files( .map(Cow::Borrowed) .unwrap_or_else(|| bins::infer_bin_dir_template(&bin_data)); + let mut tt = TinyTemplate::new(); + + tt.add_template("bin_dir", &bin_dir)?; + // Create bin_files let bin_files = package_info .binaries .iter() - .map(|bin| bins::BinFile::new(&bin_data, bin.name.as_str(), &bin_dir, no_symlinks)) + .map(|bin| bins::BinFile::new(&bin_data, bin.name.as_str(), &tt, no_symlinks)) .collect::, BinstallError>>()?; let mut source_set = BTreeSet::new();