From c0648a83bea71ddc0e3198cc4e704db7b7e28e51 Mon Sep 17 00:00:00 2001 From: JT <547158+jntrnr@users.noreply.github.com> Date: Sat, 25 Mar 2023 12:56:45 +1300 Subject: [PATCH] Move variables to var stack (#8604) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit # Description This moves the representation of variables on the stack to a Vec, which more closely resembles a stack. For small numbers of variables live at any one point, this tends to be more efficient than a HashMap. Having a stack-like vector also allows us to remember a stack position, temporarily push variables on, then quickly drop the stack back to the original size when we're done. We'll need this capability to allow matching inside of conditions. On this mac, a simple run of: `timeit { mut x = 1; while $x < 1000000 { $x += 1 } }` Went from 1 sec 86 ms, down to 1 sec 2 ms. Clearly, we have a lot more ground we can make up in looping speed 😅 but it's nice that for fixing this to make matching easier, we also get a win in terms of lookup speed for small numbers of variables. # User-Facing Changes Likely users won't (hopefully) see any negative impact and may even see a small positive impact. # Tests + Formatting Don't forget to add tests that cover your changes. Make sure you've run and fixed any issues with these commands: - `cargo fmt --all -- --check` to check standard code formatting (`cargo fmt --all` applies these changes) - `cargo clippy --workspace -- -D warnings -D clippy::unwrap_used -A clippy::needless_collect` to check that you're using the standard code style - `cargo test --workspace` to check that all tests pass > **Note** > from `nushell` you can also use the `toolkit` as follows > ```bash > use toolkit.nu # or use an `env_change` hook to activate it automatically > toolkit check pr > ``` # After Submitting If your PR had any user-facing changes, update [the documentation](https://github.com/nushell/nushell.github.io) after the PR is merged, if necessary. This will help us keep the docs up to date. --- .../nu-cmd-lang/src/core_commands/collect.rs | 4 +- .../nu-cmd-lang/src/core_commands/match_.rs | 2 +- crates/nu-command/src/hook.rs | 2 +- crates/nu-engine/src/eval.rs | 4 +- crates/nu-protocol/src/engine/stack.rs | 46 +++++++++++++++---- 5 files changed, 42 insertions(+), 16 deletions(-) diff --git a/crates/nu-cmd-lang/src/core_commands/collect.rs b/crates/nu-cmd-lang/src/core_commands/collect.rs index bef61981b4..ee827512e4 100644 --- a/crates/nu-cmd-lang/src/core_commands/collect.rs +++ b/crates/nu-cmd-lang/src/core_commands/collect.rs @@ -72,10 +72,10 @@ impl Command for Collect { // for when we support `data | let x = $in;` // remove the variables added earlier for var_id in capture_block.captures.keys() { - stack_captures.vars.remove(var_id); + stack_captures.remove_var(*var_id); } if let Some(u) = saved_positional { - stack_captures.vars.remove(&u); + stack_captures.remove_var(u); } // add any new variables to the stack stack.vars.extend(stack_captures.vars); diff --git a/crates/nu-cmd-lang/src/core_commands/match_.rs b/crates/nu-cmd-lang/src/core_commands/match_.rs index a7f9f8e41a..eea098ef16 100644 --- a/crates/nu-cmd-lang/src/core_commands/match_.rs +++ b/crates/nu-cmd-lang/src/core_commands/match_.rs @@ -113,7 +113,7 @@ impl Command for Match { }, Example { description: "Match against pipeline input", - example: "{a: {b: 3}} | match $in.a { { $b } => ($b + 10) }", + example: "{a: {b: 3}} | match $in {{a: { $b }} => ($b + 10) }", result: Some(Value::test_int(13)), }, ] diff --git a/crates/nu-command/src/hook.rs b/crates/nu-command/src/hook.rs index 45e7672057..3d57386959 100644 --- a/crates/nu-command/src/hook.rs +++ b/crates/nu-command/src/hook.rs @@ -202,7 +202,7 @@ pub fn eval_hook( } for var_id in var_ids.iter() { - stack.vars.remove(var_id); + stack.remove_var(*var_id); } } Value::Block { diff --git a/crates/nu-engine/src/eval.rs b/crates/nu-engine/src/eval.rs index f3ad15dd02..73e0a85727 100644 --- a/crates/nu-engine/src/eval.rs +++ b/crates/nu-engine/src/eval.rs @@ -459,7 +459,7 @@ pub fn eval_expression( Expr::Var(var_id) | Expr::VarDecl(var_id) => { let var_info = engine_state.get_var(*var_id); if var_info.mutable { - stack.vars.insert(*var_id, rhs); + stack.add_var(*var_id, rhs); Ok(Value::nothing(lhs.span)) } else { Err(ShellError::AssignmentRequiresMutableVar { lhs_span: lhs.span }) @@ -499,7 +499,7 @@ pub fn eval_expression( } } } else { - stack.vars.insert(*var_id, lhs); + stack.add_var(*var_id, lhs); } Ok(Value::nothing(cell_path.head.span)) } else { diff --git a/crates/nu-protocol/src/engine/stack.rs b/crates/nu-protocol/src/engine/stack.rs index 11ba821434..9e4dc7b477 100644 --- a/crates/nu-protocol/src/engine/stack.rs +++ b/crates/nu-protocol/src/engine/stack.rs @@ -65,7 +65,7 @@ impl ProfilingConfig { #[derive(Debug, Clone)] pub struct Stack { /// Variables - pub vars: HashMap, + pub vars: Vec<(VarId, Value)>, /// Environment variables arranged as a stack to be able to recover values from parent scopes pub env_vars: Vec, /// Tells which environment variables from engine state are hidden, per overlay. @@ -79,7 +79,7 @@ pub struct Stack { impl Stack { pub fn new() -> Stack { Stack { - vars: HashMap::new(), + vars: vec![], env_vars: vec![], env_hidden: HashMap::new(), active_overlays: vec![DEFAULT_OVERLAY_NAME.to_string()], @@ -104,23 +104,43 @@ impl Stack { } pub fn get_var(&self, var_id: VarId, span: Span) -> Result { - if let Some(v) = self.vars.get(&var_id) { - return Ok(v.clone().with_span(span)); + for (id, val) in &self.vars { + if var_id == *id { + return Ok(val.clone().with_span(span)); + } } Err(ShellError::VariableNotFoundAtRuntime { span }) } pub fn get_var_with_origin(&self, var_id: VarId, span: Span) -> Result { - if let Some(v) = self.vars.get(&var_id) { - return Ok(v.clone()); + for (id, val) in &self.vars { + if var_id == *id { + return Ok(val.clone()); + } } Err(ShellError::VariableNotFoundAtRuntime { span }) } pub fn add_var(&mut self, var_id: VarId, value: Value) { - self.vars.insert(var_id, value); + //self.vars.insert(var_id, value); + for (id, val) in &mut self.vars { + if *id == var_id { + *val = value; + return; + } + } + self.vars.push((var_id, value)); + } + + pub fn remove_var(&mut self, var_id: VarId) { + for (idx, (id, _)) in self.vars.iter().enumerate() { + if *id == var_id { + self.vars.remove(idx); + return; + } + } } pub fn add_env_var(&mut self, var: String, value: Value) { @@ -162,8 +182,14 @@ impl Stack { let mut env_vars = self.env_vars.clone(); env_vars.push(HashMap::new()); + // FIXME make this more efficient + let mut vars = vec![]; + for (id, val) in captures { + vars.push((*id, val.clone())); + } + Stack { - vars: captures.clone(), + vars, env_vars, env_hidden: self.env_hidden.clone(), active_overlays: self.active_overlays.clone(), @@ -173,7 +199,7 @@ impl Stack { } pub fn gather_captures(&self, captures: &[VarId]) -> Stack { - let mut vars = HashMap::new(); + let mut vars = vec![]; let fake_span = Span::new(0, 0); @@ -181,7 +207,7 @@ impl Stack { // Note: this assumes we have calculated captures correctly and that commands // that take in a var decl will manually set this into scope when running the blocks if let Ok(value) = self.get_var(*capture, fake_span) { - vars.insert(*capture, value); + vars.push((*capture, value)); } }