Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Boxes record for smaller Value enum. #12252

Merged
merged 8 commits into from Mar 26, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
138 changes: 116 additions & 22 deletions benches/benchmarks.rs
Expand Up @@ -2,8 +2,9 @@ use nu_cli::{eval_source, evaluate_commands};
use nu_parser::parse;
use nu_plugin::{Encoder, EncodingType, PluginCallResponse, PluginOutput};
use nu_protocol::{
engine::EngineState, eval_const::create_nu_constant, PipelineData, Span, Spanned, Value,
NU_VARIABLE_ID,
engine::{EngineState, Stack},
eval_const::create_nu_constant,
PipelineData, Span, Spanned, Value, NU_VARIABLE_ID,
};
use nu_std::load_standard_library;
use nu_utils::{get_default_config, get_default_env};
Expand Down Expand Up @@ -54,6 +55,61 @@ fn setup_engine() -> EngineState {
engine_state
}

fn bench_command(bencher: divan::Bencher, scaled_command: String) {
bench_command_with_custom_stack_and_engine(
bencher,
scaled_command,
Stack::new(),
setup_engine(),
)
}

fn bench_command_with_custom_stack_and_engine(
bencher: divan::Bencher,
scaled_command: String,
stack: nu_protocol::engine::Stack,
mut engine: EngineState,
) {
load_standard_library(&mut engine).unwrap();
let commands = Spanned {
span: Span::unknown(),
item: scaled_command,
};

bencher
.with_inputs(|| engine.clone())
.bench_values(|mut engine| {
evaluate_commands(
&commands,
&mut engine,
&mut stack.clone(),
PipelineData::empty(),
None,
)
.unwrap();
})
}

fn setup_stack_and_engine_from_command(command: &str) -> (Stack, EngineState) {
let mut engine = setup_engine();
let commands = Spanned {
span: Span::unknown(),
item: command.to_string(),
};

let mut stack = Stack::new();
evaluate_commands(
&commands,
&mut engine,
&mut stack,
PipelineData::empty(),
None,
)
.unwrap();

(stack, engine)
}

// FIXME: All benchmarks live in this 1 file to speed up build times when benchmarking.
// When the *_benchmarks functions were in different files, `cargo bench` would build
// an executable for every single one - incredibly slowly. Would be nice to figure out
Expand All @@ -70,30 +126,68 @@ fn load_standard_lib(bencher: divan::Bencher) {
}

#[divan::bench_group]
mod eval_commands {
mod record {

use super::*;

fn bench_command(bencher: divan::Bencher, scaled_command: String) {
let mut engine = setup_engine();
load_standard_library(&mut engine).unwrap();
let commands = Spanned {
span: Span::unknown(),
item: scaled_command,
};
fn create_flat_record_string(n: i32) -> String {
let mut s = String::from("let record = {");
for i in 0..n {
s.push_str(&format!("col_{}: {}", i, i));
if i < n - 1 {
s.push_str(", ");
}
}
s.push('}');
s
}

bencher
.with_inputs(|| engine.clone())
.bench_values(|mut engine| {
evaluate_commands(
&commands,
&mut engine,
&mut nu_protocol::engine::Stack::new(),
PipelineData::empty(),
None,
)
.unwrap();
})
fn create_nested_record_string(depth: i32) -> String {
let mut s = String::from("let record = {");
for _ in 0..depth {
s.push_str("col: {{");
}
s.push_str("col_final: 0");
for _ in 0..depth {
s.push('}');
}
s.push('}');
s
}

#[divan::bench(args = [1, 10, 100, 1000])]
fn create(bencher: divan::Bencher, n: i32) {
bench_command(bencher, create_flat_record_string(n));
}

#[divan::bench(args = [1, 10, 100, 1000])]
fn flat_access(bencher: divan::Bencher, n: i32) {
let (stack, engine) = setup_stack_and_engine_from_command(&create_flat_record_string(n));
bench_command_with_custom_stack_and_engine(
bencher,
"$record.col_0 | ignore".to_string(),
FilipAndersson245 marked this conversation as resolved.
Show resolved Hide resolved
stack,
engine,
);
}

#[divan::bench(args = [1, 2, 4, 8, 16, 32, 64, 128])]
fn nest_access(bencher: divan::Bencher, depth: i32) {
let (stack, engine) =
setup_stack_and_engine_from_command(&create_nested_record_string(depth));
let nested_access = ".col".repeat(depth as usize);
bench_command_with_custom_stack_and_engine(
bencher,
format!("$record{} | ignore", nested_access),
stack,
engine,
);
}
}

#[divan::bench_group]
mod eval_commands {
use super::*;

#[divan::bench(args = [100, 1_000, 10_000])]
fn interleave(bencher: divan::Bencher, n: i32) {
Expand Down
2 changes: 1 addition & 1 deletion crates/nu-cli/src/completions/variable_completions.rs
Expand Up @@ -292,7 +292,7 @@ fn recursive_value(val: Value, sublevels: Vec<Vec<u8>>) -> Value {
let span = val.span();
match val {
Value::Record { val, .. } => {
for item in val {
for item in *val {
// Check if index matches with sublevel
if item.0.as_bytes().to_vec() == next_sublevel {
// If matches try to fetch recursively the next
Expand Down
2 changes: 1 addition & 1 deletion crates/nu-cmd-base/src/hook.rs
Expand Up @@ -17,7 +17,7 @@ pub fn eval_env_change_hook(
if let Some(hook) = env_change_hook {
match hook {
Value::Record { val, .. } => {
for (env_name, hook_value) in &val {
for (env_name, hook_value) in &*val {
let before = engine_state
.previous_env_vars
.get(env_name)
Expand Down
Expand Up @@ -164,7 +164,7 @@ impl NuDataFrame {
conversion::insert_record(&mut column_values, record, &maybe_schema)?
}
Value::Record { val: record, .. } => {
conversion::insert_record(&mut column_values, record, &maybe_schema)?
conversion::insert_record(&mut column_values, *record, &maybe_schema)?
}
_ => {
let key = "0".to_string();
Expand Down
4 changes: 2 additions & 2 deletions crates/nu-cmd-lang/src/core_commands/describe.rs
Expand Up @@ -319,7 +319,7 @@ fn describe_value(
record!(
"type" => Value::string("record", head),
"lazy" => Value::bool(false, head),
"columns" => Value::record(val, head),
"columns" => Value::record(*val, head),
),
head,
)
Expand Down Expand Up @@ -410,7 +410,7 @@ fn describe_value(
)?);
}

record.push("columns", Value::record(val, head));
record.push("columns", Value::record(*val, head));
} else {
let cols = val.column_names();
record.push("length", Value::int(cols.len() as i64, head));
Expand Down
2 changes: 1 addition & 1 deletion crates/nu-cmd-lang/src/example_support.rs
Expand Up @@ -238,7 +238,7 @@ impl<'a> std::fmt::Debug for DebuggableValue<'a> {
Value::Record { val, .. } => {
write!(f, "{{")?;
let mut first = true;
for (col, value) in val.into_iter() {
for (col, value) in (&**val).into_iter() {
if !first {
write!(f, ", ")?;
}
Expand Down
2 changes: 1 addition & 1 deletion crates/nu-command/src/charting/histogram.rs
Expand Up @@ -182,7 +182,7 @@ fn run_histogram(
match v {
// parse record, and fill valid value to actual input.
Value::Record { val, .. } => {
for (c, v) in val {
for (c, v) in *val {
if &c == col_name {
if let Ok(v) = HashableValue::from_value(v, head_span) {
inputs.push(v);
Expand Down
2 changes: 1 addition & 1 deletion crates/nu-command/src/conversions/into/record.rs
Expand Up @@ -135,7 +135,7 @@ fn into_record(
.collect(),
span,
),
Value::Record { val, .. } => Value::record(val, span),
Value::Record { val, .. } => Value::record(*val, span),
Value::Error { .. } => input,
other => Value::error(
ShellError::TypeMismatch {
Expand Down
2 changes: 1 addition & 1 deletion crates/nu-command/src/env/load_env.rs
Expand Up @@ -58,7 +58,7 @@ impl Command for LoadEnv {
}
None => match input {
PipelineData::Value(Value::Record { val, .. }, ..) => {
for (env_var, rhs) in val {
for (env_var, rhs) in *val {
let env_var_ = env_var.as_str();
if ["FILE_PWD", "CURRENT_FILE"].contains(&env_var_) {
return Err(ShellError::AutomaticEnvVarSetManually {
Expand Down
4 changes: 2 additions & 2 deletions crates/nu-command/src/env/with_env.rs
Expand Up @@ -95,7 +95,7 @@ fn with_env(
// single row([[X W]; [Y Z]])
match &table[0] {
Value::Record { val, .. } => {
for (k, v) in val {
for (k, v) in &**val {
env.insert(k.to_string(), v.clone());
}
}
Expand Down Expand Up @@ -123,7 +123,7 @@ fn with_env(
}
// when get object by `open x.json` or `from json`
Value::Record { val, .. } => {
for (k, v) in val {
for (k, v) in &**val {
env.insert(k.clone(), v.clone());
}
}
Expand Down
2 changes: 1 addition & 1 deletion crates/nu-command/src/filters/default.rs
Expand Up @@ -112,7 +112,7 @@ fn default(
record.push(column.item.clone(), value.clone());
}

Value::record(record, span)
Value::record(*record, span)
}
_ => item,
}
Expand Down
2 changes: 1 addition & 1 deletion crates/nu-command/src/filters/drop/column.rs
Expand Up @@ -129,7 +129,7 @@ fn drop_cols(
} => {
let len = record.len().saturating_sub(columns);
record.truncate(len);
Ok(Value::record(record, span).into_pipeline_data_with_metadata(metadata))
Ok(Value::record(*record, span).into_pipeline_data_with_metadata(metadata))
}
// Propagate errors
Value::Error { error, .. } => Err(*error),
Expand Down
6 changes: 3 additions & 3 deletions crates/nu-command/src/filters/flatten.rs
Expand Up @@ -170,17 +170,17 @@ fn flat_value(columns: &[CellPath], item: Value, all: bool) -> Vec<Value> {
match value {
Value::Record { val, .. } => {
if need_flatten {
for (col, val) in val {
for (col, val) in *val {
if out.contains_key(&col) {
out.insert(format!("{column}_{col}"), val);
} else {
out.insert(col, val);
}
}
} else if out.contains_key(&column) {
out.insert(format!("{column}_{column}"), Value::record(val, span));
out.insert(format!("{column}_{column}"), Value::record(*val, span));
} else {
out.insert(column, Value::record(val, span));
out.insert(column, Value::record(*val, span));
}
}
Value::List { vals, .. } => {
Expand Down
2 changes: 1 addition & 1 deletion crates/nu-command/src/filters/rename.rs
Expand Up @@ -228,7 +228,7 @@ fn rename(
}
}

Value::record(record, span)
Value::record(*record, span)
}
// Propagate errors by explicitly matching them before the final case.
Value::Error { .. } => item.clone(),
Expand Down
2 changes: 1 addition & 1 deletion crates/nu-command/src/filters/sort.rs
Expand Up @@ -149,7 +149,7 @@ impl Command for Sort {
// Records have two sorting methods, toggled by presence or absence of -v
PipelineData::Value(Value::Record { val, .. }, ..) => {
let sort_by_value = call.has_flag(engine_state, stack, "values")?;
let record = sort_record(val, span, sort_by_value, reverse, insensitive, natural);
let record = sort_record(*val, span, sort_by_value, reverse, insensitive, natural);
Ok(record.into_pipeline_data())
}
// Other values are sorted here
Expand Down
2 changes: 1 addition & 1 deletion crates/nu-command/src/filters/values.rs
Expand Up @@ -111,7 +111,7 @@ pub fn get_values<'a>(
for item in input {
match item {
Value::Record { val, .. } => {
for (k, v) in val {
for (k, v) in &**val {
if let Some(vec) = output.get_mut(k) {
vec.push(v.clone());
} else {
Expand Down
2 changes: 1 addition & 1 deletion crates/nu-command/src/formats/from/xml.rs
Expand Up @@ -417,7 +417,7 @@ mod tests {
content_tag(
"nu",
indexmap! {},
&vec![
&[
content_tag("dev", indexmap! {}, &[content_string("Andrés")]),
content_tag("dev", indexmap! {}, &[content_string("JT")]),
content_tag("dev", indexmap! {}, &[content_string("Yehuda")])
Expand Down
2 changes: 1 addition & 1 deletion crates/nu-command/src/formats/to/json.rs
Expand Up @@ -134,7 +134,7 @@ pub fn value_to_json_value(v: &Value) -> Result<nu_json::Value, ShellError> {
}
Value::Record { val, .. } => {
let mut m = nu_json::Map::new();
for (k, v) in val {
for (k, v) in &**val {
m.insert(k.clone(), value_to_json_value(v)?);
}
nu_json::Value::Object(m)
Expand Down
2 changes: 1 addition & 1 deletion crates/nu-command/src/formats/to/nuon.rs
Expand Up @@ -252,7 +252,7 @@ pub fn value_to_string(
)),
Value::Record { val, .. } => {
let mut collection = vec![];
for (col, val) in val {
for (col, val) in &**val {
collection.push(if needs_quotes(col) {
format!(
"{idt_po}\"{}\": {}",
Expand Down
2 changes: 1 addition & 1 deletion crates/nu-command/src/formats/to/toml.rs
Expand Up @@ -60,7 +60,7 @@ fn helper(engine_state: &EngineState, v: &Value) -> Result<toml::Value, ShellErr
Value::String { val, .. } | Value::Glob { val, .. } => toml::Value::String(val.clone()),
Value::Record { val, .. } => {
let mut m = toml::map::Map::new();
for (k, v) in val {
for (k, v) in &**val {
m.insert(k.clone(), helper(engine_state, v)?);
}
toml::Value::Table(m)
Expand Down
4 changes: 2 additions & 2 deletions crates/nu-command/src/formats/to/xml.rs
Expand Up @@ -331,7 +331,7 @@ impl Job {
// content: null}, {tag: a}. See to_xml_entry for more
let attrs = match attrs {
Value::Record { val, .. } => val,
Value::Nothing { .. } => Record::new(),
Value::Nothing { .. } => Box::new(Record::new()),
_ => {
return Err(ShellError::CantConvert {
to_type: "XML".into(),
Expand All @@ -355,7 +355,7 @@ impl Job {
}
};

self.write_tag(entry_span, tag, tag_span, attrs, content)
self.write_tag(entry_span, tag, tag_span, *attrs, content)
}
}

Expand Down
2 changes: 1 addition & 1 deletion crates/nu-command/src/formats/to/yaml.rs
Expand Up @@ -57,7 +57,7 @@ pub fn value_to_yaml_value(v: &Value) -> Result<serde_yaml::Value, ShellError> {
}
Value::Record { val, .. } => {
let mut m = serde_yaml::Mapping::new();
for (k, v) in val {
for (k, v) in &**val {
m.insert(
serde_yaml::Value::String(k.clone()),
value_to_yaml_value(v)?,
Expand Down
2 changes: 1 addition & 1 deletion crates/nu-command/src/help/help_.rs
Expand Up @@ -186,7 +186,7 @@ pub fn highlight_search_in_table(
)?;

if has_match {
matches.push(Value::record(record, record_span));
matches.push(Value::record(*record, record_span));
}
}

Expand Down