Repository: reubeno/brush
Pull request #362 (2025-01-26 05:21:38)

perf: minor optimizations for shell create + command run

github-actions[bot]: # Performance Benchmark Report
| Benchmark name                         | Baseline (μs) |  Test/PR (μs) |    Delta (μs) | Delta %         |
| -------------------------------------- | ------------- | ------------- | ------------- | --------------- |
| `clone_shell_object                  ` | `   18.13 μs` | `   18.31 μs` | `    0.18 μs` | `⚪  Unchanged` |
| `eval_arithmetic                     ` | `    0.17 μs` | `    0.17 μs` | `   -0.00 μs` | `⚪  Unchanged` |
| `expand_one_string                   ` | `    1.59 μs` | `    1.60 μs` | `    0.01 μs` | `⚪  Unchanged` |
| `for_loop                            ` | `   34.03 μs` | `   35.25 μs` | `    1.23 μs` | `🟠 +3.61%    ` |
| `function_call                       ` | `    2.87 μs` | `    2.93 μs` | `    0.06 μs` | `⚪  Unchanged` |
| `instantiate_shell                   ` | `   58.81 μs` | `   57.98 μs` | `   -0.83 μs` | `🟢 -1.41%    ` |
| `instantiate_shell_with_init_scripts ` | `22226.13 μs` | `21948.97 μs` | ` -277.17 μs` | `🟢 -1.25%    ` |
| `parse_bash_completion               ` | ` 1656.19 μs` | ` 1645.68 μs` | `  -10.51 μs` | `⚪  Unchanged` |
| `parse_sample_script                 ` | `    1.72 μs` | `    1.72 μs` | `   -0.00 μs` | `⚪  Unchanged` |
| `run_echo_builtin_command            ` | `   16.16 μs` | `   16.45 μs` | `    0.29 μs` | `⚪  Unchanged` |
| `run_one_external_command            ` | ` 3876.06 μs` | ` 2331.47 μs` | `-1544.59 μs` | `🟢 -39.85%   ` |
| `tokenize_sample_script              ` | `    2.91 μs` | `    2.80 μs` | `   -0.11 μs` | `🟢 -3.78%    ` |

# Code Coverage Report: Only Changed Files listed

| Package | Base Coverage | New Coverage | Difference |
| ------- | ------------- | ------------ | ---------- |
| brush-core/src/arithmetic.rs | 🟢 98.47% | 🟢 98.48% | 🟢 0.01% |
| brush-core/src/builtins/declare.rs | 🟢 82.5% | 🟢 83.15% | 🟢 0.65% |
| brush-core/src/builtins/dot.rs | 🟢 95.83% | 🟢 95.45% | 🔴 -0.38% |
| brush-core/src/builtins/getopts.rs | 🟢 95.12% | 🟢 95.06% | 🔴 -0.06% |
| brush-core/src/builtins/set.rs | 🟢 77.17% | 🟢 76.82% | 🔴 -0.35% |
| brush-core/src/env.rs | 🟢 87.45% | 🟢 87.91% | 🟢 0.46% |
| brush-core/src/expansion.rs | 🟢 96.23% | 🟢 96.24% | 🟢 0.01% |
| brush-core/src/interp.rs | 🟢 91.47% | 🟢 91.43% | 🔴 -0.04% |
| brush-core/src/options.rs | 🟢 90.38% | 🟢 93.48% | 🟢 3.1% |
| brush-core/src/shell.rs | 🟢 78.73% | 🟢 82.65% | 🟢 3.92% |
| brush-core/src/sys/unix/terminal.rs | 🔴 32.65% | 🔴 38.78% | 🟢 6.13% |
| brush-core/src/sys/unix/users.rs | 🔴 28.95% | 🔴 38.64% | 🟢 9.69% |
| brush-core/src/variables.rs | 🟢 82.3% | 🟢 83.3% | 🟢 1% |
| brush-interactive/src/interactive_shell.rs | 🟠 74.68% | 🟠 72.84% | 🔴 -1.84% |
| **Overall Coverage** | **🟢 75.2%** | **🟢 75.64%** | **🟢 0.44%** |

_Minimum allowed coverage is_ `70%`_, this run produced_ `75.64%`
<!-- Sticky Pull Request Comment -->
github-actions[bot]: ## Test Results
    2 files      9 suites   1m 34s ⏱️
  663 tests   663 ✅ 0 💤 0 ❌
1 312 runs  1 312 ✅ 0 💤 0 ❌

Results for commit 77de77da.

[test-results]:data:application/gzip;base64,H4sIACXJlWcC/13MTQ6DIBCG4asY1l3IQID2Mg1FSCZVafhZmd69g9FWu5jF807yLSzg6DO7dXDpWK5YVlwJQ022YJwbJZk+pf2UErvuuTr3n574otR/Q7A4noJPKaatpDq3TS44bNo3T+m3ufowufq46OI0YSEwrQdPZ8GYB/cWwFthlJPgDICTCgCCELpn7w+sRRzoBgEAAA==


Patch:
diff --git a/brush-core/src/builtins/dot.rs b/brush-core/src/builtins/dot.rs
index b4aa7a5..98d14b4 100644
--- a/brush-core/src/builtins/dot.rs
+++ b/brush-core/src/builtins/dot.rs
@@ -21,14 +21,12 @@ impl builtins::Command for DotCommand {
         context: commands::ExecutionContext<'_>,
     ) -> Result<crate::builtins::ExitCode, crate::error::Error> {
         // TODO: Handle trap inheritance.
-        let script_args: Vec<_> = self.script_args.iter().map(|a| a.as_str()).collect();
-
         let params = context.params.clone();
         let result = context
             .shell
             .source_script(
                 Path::new(&self.script_path),
-                script_args.as_slice(),
+                self.script_args.iter(),
                 &params,
             )
             .await?;
diff --git a/brush-core/src/env.rs b/brush-core/src/env.rs
index 739cc2f..2dd2f44 100644
--- a/brush-core/src/env.rs
+++ b/brush-core/src/env.rs
@@ -1,4 +1,5 @@
 use std::borrow::Cow;
+use std::collections::hash_map;
 use std::collections::HashMap;
 
 use crate::error;
@@ -36,6 +37,8 @@ pub struct ShellEnvironment {
     scopes: Vec<(EnvironmentScope, ShellVariableMap)>,
     /// Whether or not to auto-export variables on creation or modification.
     export_variables_on_modification: bool,
+    /// Count of total entries (may include duplicates with shadowed variables).
+    entry_count: usize,
 }
 
 impl Default for ShellEnvironment {
@@ -50,6 +53,7 @@ impl ShellEnvironment {
         Self {
             scopes: vec![(EnvironmentScope::Global, ShellVariableMap::new())],
             export_variables_on_modification: false,
+            entry_count: 0,
         }
     }
 
@@ -94,7 +98,10 @@ impl ShellEnvironment {
         &self,
         lookup_policy: EnvironmentLookup,
     ) -> impl Iterator<Item = (&String, &ShellVariable)> {
-        let mut visible_vars: HashMap<&String, &ShellVariable> = HashMap::new();
+        // We won't actually need to store all entries, but we expect it should be
+        // within the same order.
+        let mut visible_vars: HashMap<&String, &ShellVariable> =
+            HashMap::with_capacity(self.entry_count);
 
         let mut local_count = 0;
         for (scope_type, var_map) in self.scopes.iter().rev() {
@@ -122,8 +129,9 @@ impl ShellEnvironment {
             }
 
             for (name, var) in var_map.iter() {
-                if !visible_vars.contains_key(name) {
-                    visible_vars.insert(name, var);
+                // Only insert the variable if it hasn't been seen yet.
+                if let hash_map::Entry::Vacant(entry) = visible_vars.entry(name) {
+                    entry.insert(var);
                 }
             }
 
@@ -226,6 +234,9 @@ impl ShellEnvironment {
                         name,
                         ShellVariable::new(ShellValue::Unset(ShellValueUnsetType::Untyped)),
                     );
+                } else if self.entry_count > 0 {
+                    // Entry count should never be 0 here, but we're being defensive.
+                    self.entry_count -= 1;
                 }
 
                 return Ok(unset_result);
@@ -459,7 +470,11 @@ impl ShellEnvironment {
 
         for (scope_type, map) in self.scopes.iter_mut().rev() {
             if *scope_type == target_scope {
-                map.set(name, var);
+                let prev_var = map.set(name, var);
+                if prev_var.is_none() {
+                    self.entry_count += 1;
+                }
+
                 return Ok(());
             }
         }
@@ -543,7 +558,7 @@ impl ShellVariableMap {
     ///
     /// * `name` - The name of the variable to set.
     /// * `var` - The variable to set.
-    pub fn set<N: Into<String>>(&mut self, name: N, var: ShellVariable) {
-        self.variables.insert(name.into(), var);
+    pub fn set<N: Into<String>>(&mut self, name: N, var: ShellVariable) -> Option<ShellVariable> {
+        self.variables.insert(name.into(), var)
     }
 }
diff --git a/brush-core/src/shell.rs b/brush-core/src/shell.rs
index a08b8d2..16cbb58 100644
--- a/brush-core/src/shell.rs
+++ b/brush-core/src/shell.rs
@@ -639,7 +639,7 @@ impl Shell {
         // we inherited an out-of-sync version of the variable. Future updates
         // will be handled by set_working_dir().
         //
-        let pwd = std::env::current_dir()?.to_string_lossy().to_string();
+        let pwd = self.working_dir.to_string_lossy().to_string();
         let mut pwd_var = ShellVariable::new(pwd.into());
         pwd_var.export();
         self.env.set_global("PWD", pwd_var)?;
@@ -739,8 +739,8 @@ impl Shell {
         params: &ExecutionParameters,
     ) -> Result<bool, error::Error> {
         if path.exists() {
-            let args: Vec<String> = vec![];
-            self.source_script(path, &args, params).await?;
+            self.source_script(path, std::iter::empty::<String>(), params)
+                .await?;
             Ok(true)
         } else {
             tracing::debug!("skipping non-existent file: {}", path.display());
@@ -755,10 +755,10 @@ impl Shell {
     /// * `path` - The path to the file to source.
     /// * `args` - The arguments to pass to the script as positional parameters.
     /// * `params` - Execution parameters.
-    pub async fn source_script<S: AsRef<str>>(
+    pub async fn source_script<S: AsRef<str>, I: Iterator<Item = S>>(
         &mut self,
         path: &Path,
-        args: &[S],
+        args: I,
         params: &ExecutionParameters,
     ) -> Result<ExecutionResult, error::Error> {
         self.parse_and_execute_script_file(path, args, params, ScriptCallType::Sourced)
@@ -773,10 +773,10 @@ impl Shell {
     /// * `args` - The arguments to pass to the script as positional parameters.
     /// * `params` - Execution parameters.
     /// * `call_type` - The type of script call being made.
-    async fn parse_and_execute_script_file<S: AsRef<str>>(
+    async fn parse_and_execute_script_file<S: AsRef<str>, I: Iterator<Item = S>>(
         &mut self,
         path: &Path,
-        args: &[S],
+        args: I,
         params: &ExecutionParameters,
         call_type: ScriptCallType,
     ) -> Result<ExecutionResult, error::Error> {
@@ -809,11 +809,11 @@ impl Shell {
     /// * `args` - The arguments to pass to the script as positional parameters.
     /// * `params` - Execution parameters.
     /// * `call_type` - The type of script call being made.
-    async fn source_file<F: Read, S: AsRef<str>>(
+    async fn source_file<F: Read, S: AsRef<str>, I: Iterator<Item = S>>(
         &mut self,
         file: F,
         source_info: &brush_parser::SourceInfo,
-        args: &[S],
+        args: I,
         params: &ExecutionParameters,
         call_type: ScriptCallType,
     ) -> Result<ExecutionResult, error::Error> {
@@ -824,7 +824,7 @@ impl Shell {
         tracing::debug!(target: trace_categories::PARSE, "Parsing sourced file: {}", source_info.source);
         let parse_result = parser.parse();
 
-        let mut other_positional_parameters = args.iter().map(|s| s.as_ref().to_owned()).collect();
+        let mut other_positional_parameters = args.map(|s| s.as_ref().to_owned()).collect();
         let mut other_shell_name = Some(source_info.source.clone());
 
         // TODO: Find a cleaner way to change args.
@@ -975,10 +975,10 @@ impl Shell {
     ///
     /// * `script_path` - The path to the script file to execute.
     /// * `args` - The arguments to pass to the script as positional parameters.
-    pub async fn run_script<S: AsRef<str>>(
+    pub async fn run_script<S: AsRef<str>, I: Iterator<Item = S>>(
         &mut self,
         script_path: &Path,
-        args: &[S],
+        args: I,
     ) -> Result<ExecutionResult, error::Error> {
         self.parse_and_execute_script_file(
             script_path,
diff --git a/brush-shell/src/main.rs b/brush-shell/src/main.rs
index 15cd5f6..d563dc1 100644
--- a/brush-shell/src/main.rs
+++ b/brush-shell/src/main.rs
@@ -164,7 +164,7 @@ async fn run_impl(
         shell
             .shell_mut()
             .as_mut()
-            .run_script(Path::new(&script_path), args.script_args.as_slice())
+            .run_script(Path::new(&script_path), args.script_args.iter())
             .await?;
     } else {
         // In all other cases, run interactively, taking input from stdin.


Test patch:
diff --git a/brush-shell/tests/cases/well_known_vars.yaml b/brush-shell/tests/cases/well_known_vars.yaml
index 1bbd62f..691759f 100644
--- a/brush-shell/tests/cases/well_known_vars.yaml
+++ b/brush-shell/tests/cases/well_known_vars.yaml
@@ -39,7 +39,7 @@ cases:
       echo "Final BASH_ALIASES: " ${BASH_ALIASES[@]}
 
   - name: "BASHOPTS"
-    known_failure: true # Need to normalize which options are enabled in oracle
+    skip: true # Need to normalize which options are enabled in oracle
     stdin: |
       # Workaround for brush forcing on extglob
       shopt -u extglob
diff --git a/brush-shell/tests/completion_tests.rs b/brush-shell/tests/completion_tests.rs
index 395ff53..a841774 100644
--- a/brush-shell/tests/completion_tests.rs
+++ b/brush-shell/tests/completion_tests.rs
@@ -30,7 +30,11 @@ impl TestShellWithBashCompletion {
         let mut shell = brush_core::Shell::new(&create_options).await?;
         let exec_params = shell.default_exec_params();
         let source_result = shell
-            .source_script::<String>(bash_completion_script_path.as_path(), &[], &exec_params)
+            .source_script(
+                bash_completion_script_path.as_path(),
+                std::iter::empty::<String>(),
+                &exec_params,
+            )
             .await?;
 
         if source_result.exit_code != 0 {