Box ShellError in Value::Error (#8375)

# Description

Our `ShellError` at the moment has a `std::mem::size_of<ShellError>` of
136 bytes (on AMD64). As a result `Value` directly storing the struct
also required 136 bytes (thanks to alignment requirements).

This change stores the `Value::Error` `ShellError` on the heap.

Pro:
- Value now needs just 80 bytes
- Should be 1 cacheline less (still at least 2 cachelines)

Con:
- More small heap allocations when dealing with `Value::Error`
  - More heap fragmentation
  - Potential for additional required memcopies

# Further code changes

Includes a small refactor of `try` due to a type mismatch in its large
match.

# User-Facing Changes

None for regular users.

Plugin authors may have to update their matches on `Value` if they use
`nu-protocol`

Needs benchmarking to see if there is a benefit in real world workloads.
**Update** small improvements in runtime for workloads with high volume
of values. Significant reduction in maximum resident set size, when many
values are held in memory.

# Tests + Formatting
This commit is contained in:
Stefan Holderbach
2023-03-12 09:57:27 +01:00
committed by GitHub
parent c26d91fb61
commit a52386e837
153 changed files with 648 additions and 520 deletions

View File

@ -379,13 +379,13 @@ fn interactive_copy(
);
if let Err(e) = interaction {
Value::Error {
error: ShellError::GenericError(
error: Box::new(ShellError::GenericError(
e.to_string(),
e.to_string(),
Some(span),
None,
Vec::new(),
),
)),
}
} else if !confirmed {
let msg = format!("{:} not copied to {:}", src.display(), dst.display());
@ -518,13 +518,13 @@ fn copy_symlink(
Ok(p) => p,
Err(err) => {
return Value::Error {
error: ShellError::GenericError(
error: Box::new(ShellError::GenericError(
err.to_string(),
err.to_string(),
Some(span),
None,
vec![],
),
)),
}
}
};
@ -551,7 +551,13 @@ fn copy_symlink(
Value::String { val: msg, span }
}
Err(e) => Value::Error {
error: ShellError::GenericError(e.to_string(), e.to_string(), Some(span), None, vec![]),
error: Box::new(ShellError::GenericError(
e.to_string(),
e.to_string(),
Some(span),
None,
vec![],
)),
},
}
}
@ -593,5 +599,7 @@ fn convert_io_error(error: std::io::Error, src: PathBuf, dst: PathBuf, span: Spa
_ => ShellError::IOErrorSpanned(message_src, span),
};
Value::Error { error: shell_error }
Value::Error {
error: Box::new(shell_error),
}
}

View File

@ -255,10 +255,14 @@ impl Command for Ls {
);
match entry {
Ok(value) => Some(value),
Err(err) => Some(Value::Error { error: err }),
Err(err) => Some(Value::Error {
error: Box::new(err),
}),
}
}
Err(err) => Some(Value::Error { error: err }),
Err(err) => Some(Value::Error {
error: Box::new(err),
}),
}
}
_ => Some(Value::Nothing { span: call_span }),

View File

@ -193,7 +193,9 @@ impl Command for Mv {
interactive,
);
if let Err(error) = result {
Some(Value::Error { error })
Some(Value::Error {
error: Box::new(error),
})
} else if verbose {
let val = match result {
Ok(true) => format!(

View File

@ -405,13 +405,13 @@ fn rm(
if let Err(e) = result {
let msg = format!("Could not delete because: {e:}");
Value::Error {
error: ShellError::GenericError(
error: Box::new(ShellError::GenericError(
msg,
e.to_string(),
Some(span),
None,
Vec::new(),
),
)),
}
} else if verbose {
let msg = if interactive && !confirmed {
@ -427,25 +427,25 @@ fn rm(
} else {
let msg = format!("Cannot remove {:}. try --recursive", f.to_string_lossy());
Value::Error {
error: ShellError::GenericError(
error: Box::new(ShellError::GenericError(
msg,
"cannot remove non-empty directory".into(),
Some(span),
None,
Vec::new(),
),
)),
}
}
} else {
let msg = format!("no such file or directory: {:}", f.to_string_lossy());
Value::Error {
error: ShellError::GenericError(
error: Box::new(ShellError::GenericError(
msg,
"no such file or directory".into(),
Some(span),
None,
Vec::new(),
),
)),
}
}
})

View File

@ -250,7 +250,7 @@ fn value_to_bytes(value: Value) -> Result<Vec<u8>, ShellError> {
Ok(val.into_bytes())
}
// Propagate errors by explicitly matching them before the final case.
Value::Error { error } => Err(error),
Value::Error { error } => Err(*error),
other => Ok(other.as_string()?.into_bytes()),
}
}
@ -376,7 +376,7 @@ fn stream_to_file(
Value::String { val, .. } => val.into_bytes(),
Value::Binary { val, .. } => val,
// Propagate errors by explicitly matching them before the final case.
Value::Error { error } => return Err(error),
Value::Error { error } => return Err(*error),
other => {
return Err(ShellError::OnlySupportsThisInputType {
exp_input_type: "string or binary".into(),