mirror of
https://github.com/nushell/nushell.git
synced 2025-08-12 04:09:27 +02:00
Copy-on-write for record values (#12305)
# Description This adds a `SharedCow` type as a transparent copy-on-write pointer that clones to unique on mutate. As an initial test, the `Record` within `Value::Record` is shared. There are some pretty big wins for performance. I'll post benchmark results in a comment. The biggest winner is nested access, as that would have cloned the records for each cell path follow before and it doesn't have to anymore. The reusability of the `SharedCow` type is nice and I think it could be used to clean up the previous work I did with `Arc` in `EngineState`. It's meant to be a mostly transparent clone-on-write that just clones on `.to_mut()` or `.into_owned()` if there are actually multiple references, but avoids cloning if the reference is unique. # User-Facing Changes - `Value::Record` field is a different type (plugin authors) # Tests + Formatting - 🟢 `toolkit fmt` - 🟢 `toolkit clippy` - 🟢 `toolkit test` - 🟢 `toolkit test stdlib` # After Submitting - [ ] use for `EngineState` - [ ] use for `Value::List`
This commit is contained in:
Devyn Cairns
GitHub
@ -538,7 +538,7 @@ impl FromValue for Vec<Value> {
|
||||
impl FromValue for Record {
|
||||
fn from_value(v: Value) -> Result<Self, ShellError> {
|
||||
match v {
|
||||
Value::Record { val, .. } => Ok(*val),
|
||||
Value::Record { val, .. } => Ok(val.into_owned()),
|
||||
v => Err(ShellError::CantConvert {
|
||||
to_type: "Record".into(),
|
||||
from_type: v.get_type().to_string(),
|
||||
|
||||
@ -29,7 +29,7 @@ use fancy_regex::Regex;
|
||||
use nu_utils::{
|
||||
contains_emoji,
|
||||
locale::{get_system_locale_string, LOCALE_OVERRIDE_ENV_VAR},
|
||||
IgnoreCaseExt,
|
||||
IgnoreCaseExt, SharedCow,
|
||||
};
|
||||
use serde::{Deserialize, Serialize};
|
||||
use std::{
|
||||
@ -110,7 +110,7 @@ pub enum Value {
|
||||
internal_span: Span,
|
||||
},
|
||||
Record {
|
||||
val: Box<Record>,
|
||||
val: SharedCow<Record>,
|
||||
// note: spans are being refactored out of Value
|
||||
// please use .span() instead of matching this span value
|
||||
#[serde(rename = "span")]
|
||||
@ -538,7 +538,7 @@ impl Value {
|
||||
/// Unwraps the inner [`Record`] value or returns an error if this `Value` is not a record
|
||||
pub fn into_record(self) -> Result<Record, ShellError> {
|
||||
if let Value::Record { val, .. } = self {
|
||||
Ok(*val)
|
||||
Ok(val.into_owned())
|
||||
} else {
|
||||
self.cant_convert_to("record")
|
||||
}
|
||||
@ -1159,7 +1159,7 @@ impl Value {
|
||||
match current {
|
||||
Value::Record { mut val, .. } => {
|
||||
// Make reverse iterate to avoid duplicate column leads to first value, actually last value is expected.
|
||||
if let Some(found) = val.iter_mut().rev().find(|x| {
|
||||
if let Some(found) = val.to_mut().iter_mut().rev().find(|x| {
|
||||
if insensitive {
|
||||
x.0.eq_ignore_case(column_name)
|
||||
} else {
|
||||
@ -1220,13 +1220,15 @@ impl Value {
|
||||
let val_span = val.span();
|
||||
match val {
|
||||
Value::Record { mut val, .. } => {
|
||||
if let Some(found) = val.iter_mut().rev().find(|x| {
|
||||
if insensitive {
|
||||
x.0.eq_ignore_case(column_name)
|
||||
} else {
|
||||
x.0 == column_name
|
||||
}
|
||||
}) {
|
||||
if let Some(found) =
|
||||
val.to_mut().iter_mut().rev().find(|x| {
|
||||
if insensitive {
|
||||
x.0.eq_ignore_case(column_name)
|
||||
} else {
|
||||
x.0 == column_name
|
||||
}
|
||||
})
|
||||
{
|
||||
Ok(std::mem::take(found.1))
|
||||
} else if *optional {
|
||||
Ok(Value::nothing(*origin_span))
|
||||
@ -1332,7 +1334,7 @@ impl Value {
|
||||
for val in vals.iter_mut() {
|
||||
match val {
|
||||
Value::Record { val: record, .. } => {
|
||||
if let Some(val) = record.get_mut(col_name) {
|
||||
if let Some(val) = record.to_mut().get_mut(col_name) {
|
||||
val.upsert_data_at_cell_path(path, new_val.clone())?;
|
||||
} else {
|
||||
let new_col = if path.is_empty() {
|
||||
@ -1344,7 +1346,7 @@ impl Value {
|
||||
.upsert_data_at_cell_path(path, new_val.clone())?;
|
||||
new_col
|
||||
};
|
||||
record.push(col_name, new_col);
|
||||
record.to_mut().push(col_name, new_col);
|
||||
}
|
||||
}
|
||||
Value::Error { error, .. } => return Err(*error.clone()),
|
||||
@ -1359,7 +1361,7 @@ impl Value {
|
||||
}
|
||||
}
|
||||
Value::Record { val: record, .. } => {
|
||||
if let Some(val) = record.get_mut(col_name) {
|
||||
if let Some(val) = record.to_mut().get_mut(col_name) {
|
||||
val.upsert_data_at_cell_path(path, new_val)?;
|
||||
} else {
|
||||
let new_col = if path.is_empty() {
|
||||
@ -1369,7 +1371,7 @@ impl Value {
|
||||
new_col.upsert_data_at_cell_path(path, new_val)?;
|
||||
new_col
|
||||
};
|
||||
record.push(col_name, new_col);
|
||||
record.to_mut().push(col_name, new_col);
|
||||
}
|
||||
}
|
||||
Value::LazyRecord { val, .. } => {
|
||||
@ -1456,7 +1458,7 @@ impl Value {
|
||||
let v_span = val.span();
|
||||
match val {
|
||||
Value::Record { val: record, .. } => {
|
||||
if let Some(val) = record.get_mut(col_name) {
|
||||
if let Some(val) = record.to_mut().get_mut(col_name) {
|
||||
val.update_data_at_cell_path(path, new_val.clone())?;
|
||||
} else {
|
||||
return Err(ShellError::CantFindColumn {
|
||||
@ -1478,7 +1480,7 @@ impl Value {
|
||||
}
|
||||
}
|
||||
Value::Record { val: record, .. } => {
|
||||
if let Some(val) = record.get_mut(col_name) {
|
||||
if let Some(val) = record.to_mut().get_mut(col_name) {
|
||||
val.update_data_at_cell_path(path, new_val)?;
|
||||
} else {
|
||||
return Err(ShellError::CantFindColumn {
|
||||
@ -1548,7 +1550,7 @@ impl Value {
|
||||
let v_span = val.span();
|
||||
match val {
|
||||
Value::Record { val: record, .. } => {
|
||||
if record.remove(col_name).is_none() && !optional {
|
||||
if record.to_mut().remove(col_name).is_none() && !optional {
|
||||
return Err(ShellError::CantFindColumn {
|
||||
col_name: col_name.clone(),
|
||||
span: *span,
|
||||
@ -1568,7 +1570,7 @@ impl Value {
|
||||
Ok(())
|
||||
}
|
||||
Value::Record { val: record, .. } => {
|
||||
if record.remove(col_name).is_none() && !optional {
|
||||
if record.to_mut().remove(col_name).is_none() && !optional {
|
||||
return Err(ShellError::CantFindColumn {
|
||||
col_name: col_name.clone(),
|
||||
span: *span,
|
||||
@ -1628,7 +1630,7 @@ impl Value {
|
||||
let v_span = val.span();
|
||||
match val {
|
||||
Value::Record { val: record, .. } => {
|
||||
if let Some(val) = record.get_mut(col_name) {
|
||||
if let Some(val) = record.to_mut().get_mut(col_name) {
|
||||
val.remove_data_at_cell_path(path)?;
|
||||
} else if !optional {
|
||||
return Err(ShellError::CantFindColumn {
|
||||
@ -1650,7 +1652,7 @@ impl Value {
|
||||
Ok(())
|
||||
}
|
||||
Value::Record { val: record, .. } => {
|
||||
if let Some(val) = record.get_mut(col_name) {
|
||||
if let Some(val) = record.to_mut().get_mut(col_name) {
|
||||
val.remove_data_at_cell_path(path)?;
|
||||
} else if !optional {
|
||||
return Err(ShellError::CantFindColumn {
|
||||
@ -1720,7 +1722,7 @@ impl Value {
|
||||
let v_span = val.span();
|
||||
match val {
|
||||
Value::Record { val: record, .. } => {
|
||||
if let Some(val) = record.get_mut(col_name) {
|
||||
if let Some(val) = record.to_mut().get_mut(col_name) {
|
||||
if path.is_empty() {
|
||||
return Err(ShellError::ColumnAlreadyExists {
|
||||
col_name: col_name.clone(),
|
||||
@ -1747,7 +1749,7 @@ impl Value {
|
||||
)?;
|
||||
new_col
|
||||
};
|
||||
record.push(col_name, new_col);
|
||||
record.to_mut().push(col_name, new_col);
|
||||
}
|
||||
}
|
||||
Value::Error { error, .. } => return Err(*error.clone()),
|
||||
@ -1763,7 +1765,7 @@ impl Value {
|
||||
}
|
||||
}
|
||||
Value::Record { val: record, .. } => {
|
||||
if let Some(val) = record.get_mut(col_name) {
|
||||
if let Some(val) = record.to_mut().get_mut(col_name) {
|
||||
if path.is_empty() {
|
||||
return Err(ShellError::ColumnAlreadyExists {
|
||||
col_name: col_name.clone(),
|
||||
@ -1781,7 +1783,7 @@ impl Value {
|
||||
new_col.insert_data_at_cell_path(path, new_val, head_span)?;
|
||||
new_col
|
||||
};
|
||||
record.push(col_name, new_col);
|
||||
record.to_mut().push(col_name, new_col);
|
||||
}
|
||||
}
|
||||
Value::LazyRecord { val, .. } => {
|
||||
@ -1854,6 +1856,7 @@ impl Value {
|
||||
// Check for contained values
|
||||
match self {
|
||||
Value::Record { ref mut val, .. } => val
|
||||
.to_mut()
|
||||
.iter_mut()
|
||||
.try_for_each(|(_, rec_value)| rec_value.recurse_mut(f)),
|
||||
Value::List { ref mut vals, .. } => vals
|
||||
@ -1989,7 +1992,7 @@ impl Value {
|
||||
|
||||
pub fn record(val: Record, span: Span) -> Value {
|
||||
Value::Record {
|
||||
val: Box::new(val),
|
||||
val: SharedCow::new(val),
|
||||
internal_span: span,
|
||||
}
|
||||
}
|
||||
@ -2432,8 +2435,8 @@ impl PartialOrd for Value {
|
||||
// reorder cols and vals to make more logically compare.
|
||||
// more general, if two record have same col and values,
|
||||
// the order of cols shouldn't affect the equal property.
|
||||
let mut lhs = lhs.clone();
|
||||
let mut rhs = rhs.clone();
|
||||
let mut lhs = lhs.clone().into_owned();
|
||||
let mut rhs = rhs.clone().into_owned();
|
||||
lhs.sort_cols();
|
||||
rhs.sort_cols();
|
||||
|
||||
|
||||
@ -151,7 +151,7 @@ impl Record {
|
||||
///
|
||||
/// fn remove_foo_recursively(val: &mut Value) {
|
||||
/// if let Value::Record {val, ..} = val {
|
||||
/// val.retain_mut(keep_non_foo);
|
||||
/// val.to_mut().retain_mut(keep_non_foo);
|
||||
/// }
|
||||
/// }
|
||||
///
|
||||
|
||||
Reference in New Issue
Block a user