Skip to content

Commit

Permalink
Minor refactor and optimization (rust-lang#543)
Browse files Browse the repository at this point in the history
* Avoid potential panicking in `args::parse` by using `Vec::get` instead of indexing
* Refactor: Simplify `opts::{resolve, install}` API
   Many parameters can be shared and put into `opts::Options` intead and
   that would also avoid a few `Arc<Path>`.
* Optimize `get_install_path`: Avoid cloning `install_path`
* Optimize `LazyJobserverClient`: Un`Arc` & remove `Clone` impl
   to avoid additional boxing
* Optimize `find_version`: Avoid cloning `semver::Version`
* Optimize `GhCrateMeta::launch_baseline_find_tasks`
   return `impl Iterator<Item = impl Future<Output = ...>>`
   instead of `impl Iterator<Item = AutoAbortJoinHandle<...>>`
   to avoid unnecessary spawning.
   
   Each task spawned has to be boxed and then polled by tokio runtime.
   They might also be moved.
   
   While they increase parallelism, spawning these futures does not justify
   the costs because:
    - Each `Future` only calls `remote_exists`
    - Each `remote_exists` call send requests to the same domain, which is
      likely to share the same http2 connection.
      Since the conn is shared anyway, spawning does not speedup anything
      but merely add communication overhead.
    - Plus the tokio runtime spawning cost
* Optimize `install_crates`: Destruct `Args` before any `.await` point
   to reduce size of the future
* Refactor `logging`: Replace param `arg` with `log_level` & `json_output`
   to avoid dep on `Args`
* Add dep strum & strum_macros to crates/bin
* Derive `strum_macros::EnumCount` for `Strategy`
* Optimize strategies parsing in `install_crates`
* Fix panic in `install_crates` when `Compile` is not the last strategy specified
* Optimize: Take `Vec<Self>` instead of slice in `CrateName::dedup`
* Refactor: Extract new fn `compute_resolvers`
* Refactor: Extract new fn `compute_paths_and_load_manifests`
* Refactor: Extract new fn `filter_out_installed_crates`
* Reorder `install_crates`: Only run target detection if args are valid
   and there are some crates to be installed.
* Optimize `filter_out_installed_crates`: Avoid allocation
   by returning an `Iterator`
* Fix user_agent of `remote::Client`: Let user specify it
* Refactor: Replace `UIThread` with `ui::confirm`
   which is much simpler.

Signed-off-by: Jiahao XU <[email protected]>
  • Loading branch information
NobodyXu authored Nov 19, 2022
1 parent 325cb5c commit 50b6e62
Show file tree
Hide file tree
Showing 16 changed files with 315 additions and 338 deletions.
2 changes: 2 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions crates/bin/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ miette = "5.4.1"
mimalloc = { version = "0.1.32", default-features = false, optional = true }
once_cell = "1.16.0"
semver = "1.0.14"
strum = "0.24.1"
strum_macros = "0.24.3"
supports-color = "1.3.1"
tempfile = "3.3.0"
tokio = { version = "1.21.2", features = ["rt-multi-thread"], default-features = false }
Expand Down
5 changes: 3 additions & 2 deletions crates/bin/src/args.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ use binstalk::{
use clap::{Parser, ValueEnum};
use log::LevelFilter;
use semver::VersionReq;
use strum_macros::EnumCount;

#[derive(Debug, Parser)]
#[clap(
Expand Down Expand Up @@ -296,7 +297,7 @@ impl Default for RateLimit {
}

/// Strategy for installing the package
#[derive(Debug, Copy, Clone, Eq, PartialEq, Ord, PartialOrd, ValueEnum)]
#[derive(Debug, Copy, Clone, Eq, PartialEq, Ord, PartialOrd, ValueEnum, EnumCount)]
pub enum Strategy {
/// Attempt to download official pre-built artifacts using
/// information provided in `Cargo.toml`.
Expand All @@ -312,7 +313,7 @@ pub fn parse() -> Result<Args, BinstallError> {
// `cargo run -- --help` gives ["target/debug/cargo-binstall", "--help"]
// `cargo binstall --help` gives ["/home/ryan/.cargo/bin/cargo-binstall", "binstall", "--help"]
let mut args: Vec<OsString> = std::env::args_os().collect();
let args = if args.len() > 1 && args[1] == "binstall" {
let args = if args.get(1).map(|arg| arg == "binstall").unwrap_or_default() {
// Equivalent to
//
// args.remove(1);
Expand Down
Loading

0 comments on commit 50b6e62

Please sign in to comment.