extern/nushell
1
0
Fork 1
mirror of https://github.com/nushell/nushell.git synced 2025-01-10 16:28:50 +01:00
Code Issues Packages Projects Releases Wiki Activity

Use Arc for environment variables on the stack (#13333)

Browse Source 
# Description

This is another easy performance lift that just changes `env_vars` and
`env_hidden` on `Stack` to use `Arc`. I noticed that these were being
cloned on essentially every closure invocation during captures
gathering, so we're paying the cost for all of that even when we don't
change anything. On top of that, for `env_vars`, there's actually an
entirely fresh `HashMap` created for each child scope, so it's highly
unlikely that we'll modify the parent ones.

Uses `Arc::make_mut` instead to take care of things when we need to
mutate something, and most of the time nothing has to be cloned at all.

# Benchmarks

The benefits are greater the more calls there are to env-cloning
functions like `captures_to_stack()`. Calling custom commands in a loop
is basically best case for a performance improvement. Plain `each` with
a literal block isn't so badly affected because the stack is set up
once.

## random_bytes.nu

```nushell
use std bench
do {
  const SCRIPT = ../nu_scripts/benchmarks/random-bytes.nu
  let before_change = bench { nu $SCRIPT }
  let after_change = bench { target/release/nu $SCRIPT }
  {
    before: ($before_change | reject times),
    after: ($after_change | reject times)
  }
}
```

```
╭────────┬──────────────────────────────╮
│        │ ╭──────┬───────────────────╮ │
│ before │ │ mean │ 603ms 759µs 727ns │ │
│        │ │ min  │ 593ms 298µs 167ns │ │
│        │ │ max  │ 648ms 612µs 291ns │ │
│        │ │ std  │ 9ms 335µs 251ns   │ │
│        │ ╰──────┴───────────────────╯ │
│        │ ╭──────┬───────────────────╮ │
│ after  │ │ mean │ 518ms 400µs 557ns │ │
│        │ │ min  │ 507ms 762µs 583ns │ │
│        │ │ max  │ 566ms 695µs 166ns │ │
│        │ │ std  │ 9ms 554µs 767ns   │ │
│        │ ╰──────┴───────────────────╯ │
╰────────┴──────────────────────────────╯
```

## gradient_benchmark_no_check.nu

```nushell
use std bench
do {
  const SCRIPT = ../nu_scripts/benchmarks/gradient_benchmark_no_check.nu
  let before_change = bench { nu $SCRIPT }
  let after_change = bench { target/release/nu $SCRIPT }
  {
    before: ($before_change | reject times),
    after: ($after_change | reject times)
  }
}
```

```
╭────────┬──────────────────────────────╮
│        │ ╭──────┬───────────────────╮ │
│ before │ │ mean │ 146ms 543µs 380ns │ │
│        │ │ min  │ 142ms 416µs 166ns │ │
│        │ │ max  │ 189ms 595µs       │ │
│        │ │ std  │ 7ms 140µs 342ns   │ │
│        │ ╰──────┴───────────────────╯ │
│        │ ╭──────┬───────────────────╮ │
│ after  │ │ mean │ 134ms 211µs 678ns │ │
│        │ │ min  │ 132ms 433µs 125ns │ │
│        │ │ max  │ 135ms 722µs 583ns │ │
│        │ │ std  │ 793µs 134ns       │ │
│        │ ╰──────┴───────────────────╯ │
╰────────┴──────────────────────────────╯
```

# User-Facing Changes
Better performance, particularly for custom commands, especially if
there are a lot of environment variables. Nothing else.

# Tests + Formatting
All passing.
This commit is contained in:
Devyn Cairns 2024-07-10 17:34:50 -07:00 committed by GitHub
parent d7392f1f3b
commit 1a5bf2447a
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
3 changed files with 19 additions and 17 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
crates/nu-engine/src/closure_eval.rs
View File

@ -62,8 +62,8 @@ pub struct ClosureEval {
stack: Stack,
block: Arc<Block>,
arg_index: usize,
env_vars: Vec<EnvVars>,
env_hidden: HashMap<String, HashSet<String>>,
env_vars: Vec<Arc<EnvVars>>,
env_hidden: Arc<HashMap<String, HashSet<String>>>,
eval: EvalBlockWithEarlyReturnFn,
}

2
crates/nu-protocol/src/engine/engine_state.rs
View File

@ -304,7 +304,7 @@ impl EngineState {
let mut config_updated = false;
for mut scope in stack.env_vars.drain(..) {
for (overlay_name, mut env) in scope.drain() {
for (overlay_name, mut env) in Arc::make_mut(&mut scope).drain() {
if let Some(env_vars) = Arc::make_mut(&mut self.env_vars).get_mut(&overlay_name) {
// Updating existing overlay
for (k, v) in env.drain() {

30
crates/nu-protocol/src/engine/stack.rs
View File

@ -36,9 +36,9 @@ pub struct Stack {
/// Variables
pub vars: Vec<(VarId, Value)>,
/// Environment variables arranged as a stack to be able to recover values from parent scopes
pub env_vars: Vec<EnvVars>,
pub env_vars: Vec<Arc<EnvVars>>,
/// Tells which environment variables from engine state are hidden, per overlay.
pub env_hidden: HashMap<String, HashSet<String>>,
pub env_hidden: Arc<HashMap<String, HashSet<String>>>,
/// List of active overlays
pub active_overlays: Vec<String>,
/// Argument stack for IR evaluation
@ -72,7 +72,7 @@ impl Stack {
Self {
vars: Vec::new(),
env_vars: Vec::new(),
env_hidden: HashMap::new(),
env_hidden: Arc::new(HashMap::new()),
active_overlays: vec![DEFAULT_OVERLAY_NAME.to_string()],
arguments: ArgumentStack::new(),
error_handlers: ErrorHandlerStack::new(),
@ -131,8 +131,8 @@ impl Stack {
pub fn with_env(
&mut self,
env_vars: &[EnvVars],
env_hidden: &HashMap<String, HashSet<String>>,
env_vars: &[Arc<EnvVars>],
env_hidden: &Arc<HashMap<String, HashSet<String>>>,
) {
// Do not clone the environment if it hasn't changed
if self.env_vars.iter().any(|scope| !scope.is_empty()) {
@ -219,23 +219,24 @@ impl Stack {
pub fn add_env_var(&mut self, var: String, value: Value) {
if let Some(last_overlay) = self.active_overlays.last() {
if let Some(env_hidden) = self.env_hidden.get_mut(last_overlay) {
if let Some(env_hidden) = Arc::make_mut(&mut self.env_hidden).get_mut(last_overlay) {
// if the env var was hidden, let's activate it again
env_hidden.remove(&var);
}
if let Some(scope) = self.env_vars.last_mut() {
let scope = Arc::make_mut(scope);
if let Some(env_vars) = scope.get_mut(last_overlay) {
env_vars.insert(var, value);
} else {
scope.insert(last_overlay.into(), [(var, value)].into_iter().collect());
}
} else {
self.env_vars.push(
self.env_vars.push(Arc::new(
[(last_overlay.into(), [(var, value)].into_iter().collect())]
.into_iter()
.collect(),
);
));
}
} else {
// TODO: Remove panic
@ -257,9 +258,8 @@ impl Stack {
}
pub fn captures_to_stack_preserve_out_dest(&self, captures: Vec<(VarId, Value)>) -> Stack {
// FIXME: this is probably slow
let mut env_vars = self.env_vars.clone();
env_vars.push(HashMap::new());
env_vars.push(Arc::new(HashMap::new()));
Stack {
vars: captures,
@ -292,7 +292,7 @@ impl Stack {
}
let mut env_vars = self.env_vars.clone();
env_vars.push(HashMap::new());
env_vars.push(Arc::new(HashMap::new()));
Stack {
vars,
@ -462,6 +462,7 @@ impl Stack {
pub fn remove_env_var(&mut self, engine_state: &EngineState, name: &str) -> bool {
for scope in self.env_vars.iter_mut().rev() {
let scope = Arc::make_mut(scope);
for active_overlay in self.active_overlays.iter().rev() {
if let Some(env_vars) = scope.get_mut(active_overlay) {
if env_vars.remove(name).is_some() {
@ -474,10 +475,11 @@ impl Stack {
for active_overlay in self.active_overlays.iter().rev() {
if let Some(env_vars) = engine_state.env_vars.get(active_overlay) {
if env_vars.get(name).is_some() {
if let Some(env_hidden) = self.env_hidden.get_mut(active_overlay) {
env_hidden.insert(name.into());
let env_hidden = Arc::make_mut(&mut self.env_hidden);
if let Some(env_hidden_in_overlay) = env_hidden.get_mut(active_overlay) {
env_hidden_in_overlay.insert(name.into());
} else {
self.env_hidden
env_hidden
.insert(active_overlay.into(), [name.into()].into_iter().collect());
}