Optimizations, mainly to env (#1226)

* Avoid unneeded allocations for Arch

* Avoid repetition for VersionFileStrategy

* Use write_str when possible

* Avoid unneeded allocation in rehash

* Make LogLevel Copy and add as_str

* VersionFileStrategy is Copy

* Clone if Some

* Avoid hashing and allocation if not JSON

* Arch is Copy

* Fix Windows target

* create changeset

---------

Co-authored-by: Gal Schlezinger <gal@spitfire.co.il>

.changeset/tall-hornets-rush.md

@@ -0,0 +1,5 @@
+---
+"fnm": patch
+---
+
+performance optimizations, especially for `fnm env`

src/arch.rs

@@ -1,6 +1,6 @@
 use crate::version::Version;
 
-#[derive(Clone, PartialEq, Eq, Debug)]
+#[derive(Clone, Copy, PartialEq, Eq, Debug)]
 pub enum Arch {
     X86,
     X64,
@@ -12,20 +12,35 @@ pub enum Arch {
     S390x,
 }
 
+impl Arch {
+    pub fn as_str(self) -> &'static str {
+        match self {
+            Arch::X86 => "x86",
+            Arch::X64 => "x64",
+            Arch::X64Musl => "x64-musl",
+            Arch::Arm64 => "arm64",
+            Arch::Armv7l => "armv7l",
+            Arch::Ppc64le => "ppc64le",
+            Arch::Ppc64 => "ppc64",
+            Arch::S390x => "s390x",
+        }
+    }
+}
+
 #[cfg(unix)]
 /// handle common case: Apple Silicon / Node < 16
-pub fn get_safe_arch<'a>(arch: &'a Arch, version: &Version) -> &'a Arch {
+pub fn get_safe_arch(arch: Arch, version: &Version) -> Arch {
     use crate::system_info::{platform_arch, platform_name};
 
     match (platform_name(), platform_arch(), version) {
-        ("darwin", "arm64", Version::Semver(v)) if v.major < 16 => &Arch::X64,
+        ("darwin", "arm64", Version::Semver(v)) if v.major < 16 => Arch::X64,
         _ => arch,
     }
 }
 
 #[cfg(windows)]
 /// handle common case: Apple Silicon / Node < 16
-pub fn get_safe_arch<'a>(arch: &'a Arch, _version: &Version) -> &'a Arch {
+pub fn get_safe_arch(arch: Arch, _version: &Version) -> Arch {
     arch
 }
 
@@ -50,25 +65,14 @@ impl std::str::FromStr for Arch {
             "ppc64le" => Ok(Arch::Ppc64le),
             "ppc64" => Ok(Arch::Ppc64),
             "s390x" => Ok(Arch::S390x),
-            unknown => Err(ArchError::new(&format!("Unknown Arch: {unknown}"))),
+            unknown => Err(ArchError::new(format!("Unknown Arch: {unknown}"))),
         }
     }
 }
 
 impl std::fmt::Display for Arch {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-        let arch_str = match self {
-            Arch::X86 => String::from("x86"),
-            Arch::X64 => String::from("x64"),
-            Arch::X64Musl => String::from("x64-musl"),
-            Arch::Arm64 => String::from("arm64"),
-            Arch::Armv7l => String::from("armv7l"),
-            Arch::Ppc64le => String::from("ppc64le"),
-            Arch::Ppc64 => String::from("ppc64"),
-            Arch::S390x => String::from("s390x"),
-        };
-
-        write!(f, "{arch_str}")
+        f.write_str(self.as_str())
     }
 }
 
@@ -78,16 +82,14 @@ pub struct ArchError {
 }
 
 impl ArchError {
-    fn new(msg: &str) -> ArchError {
-        ArchError {
-            details: msg.to_string(),
-        }
+    fn new(msg: String) -> ArchError {
+        ArchError { details: msg }
     }
 }
 
 impl std::fmt::Display for ArchError {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-        write!(f, "{}", self.details)
+        f.write_str(&self.details)
     }
 }
 

src/commands/env.rs

@@ -48,6 +48,15 @@ fn make_symlink(config: &FnmConfig) -> Result<std::path::PathBuf, Error> {
     }
 }
 
+#[inline]
+fn bool_as_str(value: bool) -> &'static str {
+    if value {
+        "true"
+    } else {
+        "false"
+    }
+}
+
 impl Command for Env {
     type Error = Error;
 
@@ -63,42 +72,30 @@ impl Command for Env {
         }
 
         let multishell_path = make_symlink(config)?;
-        let multishell_path_str = multishell_path.to_str().unwrap().to_owned();
+        let base_dir = config.base_dir_with_default();
 
-        let binary_path = if cfg!(windows) {
-            multishell_path
-        } else {
-            multishell_path.join("bin")
-        };
-
-        let env_vars = HashMap::from([
-            ("FNM_MULTISHELL_PATH", multishell_path_str),
+        let env_vars = [
+            ("FNM_MULTISHELL_PATH", multishell_path.to_str().unwrap()),
             (
                 "FNM_VERSION_FILE_STRATEGY",
-                config.version_file_strategy().as_str().to_owned(),
-            ),
-            (
-                "FNM_DIR",
-                config.base_dir_with_default().to_str().unwrap().to_owned(),
-            ),
-            (
-                "FNM_LOGLEVEL",
-                <&'static str>::from(config.log_level().clone()).to_owned(),
-            ),
-            (
-                "FNM_NODE_DIST_MIRROR",
-                config.node_dist_mirror.as_str().to_owned(),
+                config.version_file_strategy().as_str(),
             ),
+            ("FNM_DIR", base_dir.to_str().unwrap()),
+            ("FNM_LOGLEVEL", config.log_level().as_str()),
+            ("FNM_NODE_DIST_MIRROR", config.node_dist_mirror.as_str()),
             (
                 "FNM_COREPACK_ENABLED",
-                config.corepack_enabled().to_string(),
+                bool_as_str(config.corepack_enabled()),
             ),
-            ("FNM_RESOLVE_ENGINES", config.resolve_engines().to_string()),
-            ("FNM_ARCH", config.arch.to_string()),
-        ]);
+            ("FNM_RESOLVE_ENGINES", bool_as_str(config.resolve_engines())),
+            ("FNM_ARCH", config.arch.as_str()),
+        ];
 
         if self.json {
-            println!("{}", serde_json::to_string(&env_vars).unwrap());
+            println!(
+                "{}",
+                serde_json::to_string(&HashMap::from(env_vars)).unwrap()
+            );
             return Ok(());
         }
 
@@ -108,7 +105,13 @@ impl Command for Env {
             .or_else(infer_shell)
             .ok_or(Error::CantInferShell)?;
 
-        println!("{}", shell.path(&binary_path)?);
+        let binary_path = if cfg!(windows) {
+            shell.path(&multishell_path)
+        } else {
+            shell.path(&multishell_path.join("bin"))
+        };
+
+        println!("{}", binary_path?);
 
         for (name, value) in &env_vars {
             println!("{}", shell.set_env_var(name, value));

src/commands/install.rs

@@ -126,15 +126,15 @@ impl Command for Install {
         };
 
         // Automatically swap Apple Silicon to x64 arch for appropriate versions.
-        let safe_arch = get_safe_arch(&config.arch, &version);
+        let safe_arch = get_safe_arch(config.arch, &version);
 
         let version_str = format!("Node {}", &version);
         outln!(
             config,
             Info,
             "Installing {} ({})",
             version_str.cyan(),
-            safe_arch.to_string()
+            safe_arch.as_str()
         );
 
         match install_node_dist(

src/config.rs

@@ -109,8 +109,8 @@ impl Default for FnmConfig {
 }
 
 impl FnmConfig {
-    pub fn version_file_strategy(&self) -> &VersionFileStrategy {
-        &self.version_file_strategy
+    pub fn version_file_strategy(&self) -> VersionFileStrategy {
+        self.version_file_strategy
     }
 
     pub fn corepack_enabled(&self) -> bool {
@@ -128,14 +128,13 @@ impl FnmConfig {
         }
     }
 
-    pub fn log_level(&self) -> &LogLevel {
-        &self.log_level
+    pub fn log_level(&self) -> LogLevel {
+        self.log_level
     }
 
     pub fn base_dir_with_default(&self) -> std::path::PathBuf {
-        let user_pref = self.base_dir.clone();
-        if let Some(dir) = user_pref {
-            return dir;
+        if let Some(dir) = &self.base_dir {
+            return dir.clone();
         }
 
         self.directories.default_base_dir()

src/downloader.rs

@@ -38,7 +38,7 @@ pub enum Error {
 }
 
 #[cfg(unix)]
-fn filename_for_version(version: &Version, arch: &Arch) -> String {
+fn filename_for_version(version: &Version, arch: Arch) -> String {
     format!(
         "node-{node_ver}-{platform}-{arch}.tar.xz",
         node_ver = &version,
@@ -48,15 +48,11 @@ fn filename_for_version(version: &Version, arch: &Arch) -> String {
 }
 
 #[cfg(windows)]
-fn filename_for_version(version: &Version, arch: &Arch) -> String {
-    format!(
-        "node-{node_ver}-win-{arch}.zip",
-        node_ver = &version,
-        arch = arch,
-    )
+fn filename_for_version(version: &Version, arch: Arch) -> String {
+    format!("node-{node_ver}-win-{arch}.zip", node_ver = &version)
 }
 
-fn download_url(base_url: &Url, version: &Version, arch: &Arch) -> Url {
+fn download_url(base_url: &Url, version: &Version, arch: Arch) -> Url {
     Url::parse(&format!(
         "{}/{}/{}",
         base_url.as_str().trim_end_matches('/'),
@@ -80,7 +76,7 @@ pub fn install_node_dist<P: AsRef<Path>>(
     version: &Version,
     node_dist_mirror: &Url,
     installations_dir: P,
-    arch: &Arch,
+    arch: Arch,
     show_progress: bool,
 ) -> Result<(), Error> {
     let installation_dir = PathBuf::from(installations_dir.as_ref()).join(version.v_str());
@@ -105,7 +101,7 @@ pub fn install_node_dist<P: AsRef<Path>>(
     if response.status() == 404 {
         return Err(Error::VersionNotFound {
             version: version.clone(),
-            arch: arch.clone(),
+            arch,
         });
     }
 
@@ -179,7 +175,7 @@ mod tests {
         let version = Version::parse("12.0.0").unwrap();
         let arch = Arch::X64;
         let node_dist_mirror = Url::parse("https://nodejs.org/dist/").unwrap();
-        install_node_dist(&version, &node_dist_mirror, path, &arch, false)
+        install_node_dist(&version, &node_dist_mirror, path, arch, false)
             .expect("Can't install Node 12");
 
         let mut location_path = path.join(version.v_str()).join("installation");

src/log_level.rs

@@ -2,7 +2,7 @@ use std::fmt::Display;
 
 use clap::ValueEnum;
 
-#[derive(Debug, Default, PartialEq, PartialOrd, Eq, Ord, Clone, ValueEnum)]
+#[derive(Debug, Default, PartialEq, PartialOrd, Eq, Ord, Clone, Copy, ValueEnum)]
 pub enum LogLevel {
     Quiet,
     Error,
@@ -11,23 +11,21 @@ pub enum LogLevel {
     Info,
 }
 
-impl Display for LogLevel {
-    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+impl LogLevel {
+    pub fn as_str(self) -> &'static str {
         match self {
-            LogLevel::Quiet => write!(f, "quiet"),
-            LogLevel::Error => write!(f, "error"),
-            LogLevel::Info => write!(f, "info"),
+            Self::Quiet => "quiet",
+            Self::Info => "info",
+            Self::Error => "error",
         }
     }
-}
 
-impl LogLevel {
-    pub fn is_writable(&self, logging: &Self) -> bool {
+    pub fn is_writable(self, logging: Self) -> bool {
         use std::cmp::Ordering;
-        matches!(self.cmp(logging), Ordering::Greater | Ordering::Equal)
+        matches!(self.cmp(&logging), Ordering::Greater | Ordering::Equal)
     }
 
-    pub fn writer_for(&self, logging: &Self) -> Box<dyn std::io::Write> {
+    pub fn writer_for(self, logging: Self) -> Box<dyn std::io::Write> {
         if self.is_writable(logging) {
             match logging {
                 Self::Error => Box::from(std::io::stderr()),
@@ -43,21 +41,17 @@ impl LogLevel {
     }
 }
 
-impl From<LogLevel> for &'static str {
-    fn from(level: LogLevel) -> Self {
-        match level {
-            LogLevel::Quiet => "quiet",
-            LogLevel::Info => "info",
-            LogLevel::Error => "error",
-        }
+impl Display for LogLevel {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        f.write_str(self.as_str())
     }
 }
 
 #[macro_export]
 macro_rules! outln {
     ($config:ident, $level:path, $($expr:expr),+) => {{
         use $crate::log_level::LogLevel::*;
-        writeln!($config.log_level().writer_for(&$level), $($expr),+).expect("Can't write output");
+        writeln!($config.log_level().writer_for($level), $($expr),+).expect("Can't write output");
     }}
 }
 
@@ -67,9 +61,9 @@ mod tests {
 
     #[test]
     fn test_is_writable() {
-        assert!(!LogLevel::Quiet.is_writable(&LogLevel::Info));
-        assert!(!LogLevel::Error.is_writable(&LogLevel::Info));
-        assert!(LogLevel::Info.is_writable(&LogLevel::Info));
-        assert!(LogLevel::Info.is_writable(&LogLevel::Error));
+        assert!(!LogLevel::Quiet.is_writable(LogLevel::Info));
+        assert!(!LogLevel::Error.is_writable(LogLevel::Info));
+        assert!(LogLevel::Info.is_writable(LogLevel::Info));
+        assert!(LogLevel::Info.is_writable(LogLevel::Error));
     }
 }

src/lts.rs

@@ -22,8 +22,8 @@ impl From<&str> for LtsType {
 impl Display for LtsType {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         match self {
-            Self::Latest => write!(f, "latest"),
-            Self::CodeName(s) => write!(f, "{s}"),
+            Self::Latest => f.write_str("latest"),
+            Self::CodeName(s) => f.write_str(s),
         }
     }
 }

src/progress.rs

@@ -23,7 +23,7 @@ impl ProgressConfig {
             Self::Always => true,
             Self::Auto => config
                 .log_level()
-                .is_writable(&crate::log_level::LogLevel::Info),
+                .is_writable(crate::log_level::LogLevel::Info),
         }
     }
 }