From ab3e47c42b4750c33ebb1df9ce30ea8a0643a20c Mon Sep 17 00:00:00 2001 From: Jiahao XU Date: Wed, 30 Nov 2022 14:15:22 +1100 Subject: [PATCH] Optimize `Fetcher::find` and fix quickinstall reporting (#579) * Optimize `Fetcher::find`: Make it non-async to avoid boxing and return `AutoAbortJoinHandle<...>` instead. Since spawning a new task in tokio box the future anyway, boxing the returned future again in `Fetcher::find` merely adds unnecessary overheads. * Optimize `QuickInstall::report`: Make it async fn instead of ret `JoinHandle` This provides several benefits: - On debug build, no task will be spawned - The calls to `self.stats_url()` can be evaluated in parallel. - The task now returns `()` so that the task spawned is smaller. * Fix `QuickInstall::find`: `warn!` if quickinstall report fails Signed-off-by: Jiahao XU --- crates/binstalk/src/fetchers.rs | 4 +- crates/binstalk/src/fetchers/gh_crate_meta.rs | 111 ++++++++++-------- crates/binstalk/src/fetchers/quickinstall.rs | 55 +++++---- crates/binstalk/src/ops/resolve.rs | 7 +- 4 files changed, 93 insertions(+), 84 deletions(-) diff --git a/crates/binstalk/src/fetchers.rs b/crates/binstalk/src/fetchers.rs index 5da5d33a..1b9d324c 100644 --- a/crates/binstalk/src/fetchers.rs +++ b/crates/binstalk/src/fetchers.rs @@ -6,7 +6,7 @@ pub use quickinstall::*; use crate::{ errors::BinstallError, - helpers::remote::Client, + helpers::{remote::Client, tasks::AutoAbortJoinHandle}, manifests::cargo_toml_binstall::{PkgFmt, PkgMeta}, }; @@ -32,7 +32,7 @@ pub trait Fetcher: Send + Sync { /// /// Must return `true` if a package is available, `false` if none is, and reserve errors to /// fatal conditions only. - async fn find(&self) -> Result; + fn find(self: Arc) -> AutoAbortJoinHandle>; /// Return the package format fn pkg_fmt(&self) -> PkgFmt; diff --git a/crates/binstalk/src/fetchers/gh_crate_meta.rs b/crates/binstalk/src/fetchers/gh_crate_meta.rs index 20c99e3a..15defd2e 100644 --- a/crates/binstalk/src/fetchers/gh_crate_meta.rs +++ b/crates/binstalk/src/fetchers/gh_crate_meta.rs @@ -16,6 +16,7 @@ use crate::{ download::Download, remote::{Client, Method}, signal::wait_on_cancellation_signal, + tasks::AutoAbortJoinHandle, }, manifests::cargo_toml_binstall::{PkgFmt, PkgMeta}, }; @@ -83,26 +84,27 @@ impl super::Fetcher for GhCrateMeta { }) } - async fn find(&self) -> Result { - let repo = if let Some(repo) = self.data.repo.as_deref() { - Some( - self.client - .get_redirected_final_url(Url::parse(repo)?) - .await?, - ) - } else { - None - }; - - 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() - { - Either::Right(pkg_urls) + fn find(self: Arc) -> AutoAbortJoinHandle> { + AutoAbortJoinHandle::spawn(async move { + let repo = if let Some(repo) = self.data.repo.as_deref() { + Some( + self.client + .get_redirected_final_url(Url::parse(repo)?) + .await?, + ) } else { - warn!( + None + }; + + 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() + { + 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 {}." @@ -110,10 +112,10 @@ impl super::Fetcher for GhCrateMeta { repo, self.target_data.target ); - return Ok(false); - } - } else { - warn!( + return Ok(false); + } + } else { + warn!( concat!( "Package does not specify repository, cargo-binstall cannot provide default pkg_url for it.\n", "Please ask the upstream to provide it for target {}." @@ -121,39 +123,44 @@ impl super::Fetcher for GhCrateMeta { self.target_data.target ); - return Ok(false); - }; + return Ok(false); + }; - // Convert Option to Option to reduce size of future. - let repo = repo.map(String::from); - let repo = repo.as_deref().map(|u| u.trim_end_matches('/')); + // Convert Option to Option 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)), - Either::Right(pkg_urls) => Either::Right(pkg_urls.iter().map(Deref::deref)), + // Use reference to self to fix error of closure + // launch_baseline_find_tasks which moves `this` + let this = &self; + + let launch_baseline_find_tasks = |pkg_fmt| { + 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| this.launch_baseline_find_tasks(pkg_fmt, pkg_url, repo)) + }; + + 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() + .flat_map(launch_baseline_find_tasks) + .collect() + }; + + while let Some(res) = handles.next().await { + if let Some((url, pkg_fmt)) = res? { + debug!("Winning URL is {url}, with pkg_fmt {pkg_fmt}"); + self.resolution.set((url, pkg_fmt)).unwrap(); // find() is called first + return Ok(true); + } } - .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.target_data.meta.pkg_fmt - { - launch_baseline_find_tasks(pkg_fmt).collect() - } else { - PkgFmt::iter() - .flat_map(launch_baseline_find_tasks) - .collect() - }; - - while let Some(res) = handles.next().await { - if let Some((url, pkg_fmt)) = res? { - debug!("Winning URL is {url}, with pkg_fmt {pkg_fmt}"); - self.resolution.set((url, pkg_fmt)).unwrap(); // find() is called first - return Ok(true); - } - } - - Ok(false) + Ok(false) + }) } async fn fetch_and_extract(&self, dst: &Path) -> Result<(), BinstallError> { diff --git a/crates/binstalk/src/fetchers/quickinstall.rs b/crates/binstalk/src/fetchers/quickinstall.rs index f03eec41..d49847e3 100644 --- a/crates/binstalk/src/fetchers/quickinstall.rs +++ b/crates/binstalk/src/fetchers/quickinstall.rs @@ -1,8 +1,7 @@ use std::{path::Path, sync::Arc}; use compact_str::CompactString; -use tokio::task::JoinHandle; -use tracing::debug; +use tracing::{debug, warn}; use url::Url; use crate::{ @@ -11,6 +10,7 @@ use crate::{ download::Download, remote::{Client, Method}, signal::wait_on_cancellation_signal, + tasks::AutoAbortJoinHandle, }, manifests::cargo_toml_binstall::{PkgFmt, PkgMeta}, }; @@ -43,14 +43,29 @@ impl super::Fetcher for QuickInstall { }) } - async fn find(&self) -> Result { - let url = self.package_url(); - self.report(); - debug!("Checking for package at: '{url}'"); - Ok(self - .client - .remote_exists(Url::parse(&url)?, Method::HEAD) - .await?) + fn find(self: Arc) -> AutoAbortJoinHandle> { + AutoAbortJoinHandle::spawn(async move { + if cfg!(debug_assertions) { + debug!("Not sending quickinstall report in debug mode"); + } else { + let this = self.clone(); + tokio::spawn(async move { + if let Err(err) = this.report().await { + warn!( + "Failed to send quickinstall report for package {}: {err}", + this.package + ) + } + }); + } + + let url = self.package_url(); + debug!("Checking for package at: '{url}'"); + Ok(self + .client + .remote_exists(Url::parse(&url)?, Method::HEAD) + .await?) + }) } async fn fetch_and_extract(&self, dst: &Path) -> Result<(), BinstallError> { @@ -110,22 +125,12 @@ impl QuickInstall { ) } - pub fn report(&self) -> JoinHandle> { - let stats_url = self.stats_url(); - let client = self.client.clone(); + pub async fn report(&self) -> Result<(), BinstallError> { + let url = Url::parse(&self.stats_url())?; + debug!("Sending installation report to quickinstall ({url})"); - tokio::spawn(async move { - if cfg!(debug_assertions) { - debug!("Not sending quickinstall report in debug mode"); - return Ok(()); - } + self.client.remote_exists(url, Method::HEAD).await?; - let url = Url::parse(&stats_url)?; - debug!("Sending installation report to quickinstall ({url})"); - - client.remote_exists(url, Method::HEAD).await?; - - Ok(()) - }) + Ok(()) } } diff --git a/crates/binstalk/src/ops/resolve.rs b/crates/binstalk/src/ops/resolve.rs index 09784b7b..44dcb11e 100644 --- a/crates/binstalk/src/ops/resolve.rs +++ b/crates/binstalk/src/ops/resolve.rs @@ -20,7 +20,7 @@ use crate::{ drivers::fetch_crate_cratesio, errors::BinstallError, fetchers::{Data, Fetcher, TargetData}, - helpers::{remote::Client, tasks::AutoAbortJoinHandle}, + helpers::remote::Client, manifests::cargo_toml_binstall::{Meta, PkgMeta, PkgOverride}, }; @@ -162,10 +162,7 @@ async fn resolve_inner( .cartesian_product(resolvers) .map(|(target_data, f)| { let fetcher = f(opts.client.clone(), data.clone(), target_data); - ( - fetcher.clone(), - AutoAbortJoinHandle::spawn(async move { fetcher.find().await }), - ) + (fetcher.clone(), fetcher.find()) }), );