Speed up tight loop benchmarks (#8609)

# Description

This does a few speedups for tight loops:
* Caches the DeclId for `table` so we don't look it up. This means users
can't easily replace the default one, we might want to talk about this
tradeoff. The lookup for finding `table` in a tight loop is currently
pretty heavy. Might be another way to speed this up.
* `table` no longer pre-calculates the width. Instead, it only
calculates the width when printing a table or record.
* Use more efficient way of collecting the block of each loop
* When printing output, only get the config when needed

Combined, this drops the runtime from a million loop tight iteration
from 1sec 8ms to 236ms.

# User-Facing Changes

_(List of all changes that impact the user experience here. This helps
us keep track of breaking changes.)_

# 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.
This commit is contained in:
JT 2023-03-26 06:12:57 +13:00 committed by GitHub
parent ad4450f9e8
commit 6872d2ac2a
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
6 changed files with 30 additions and 20 deletions

View file

@ -162,7 +162,8 @@ impl Command for For {
return Err(err); return Err(err);
} }
Ok(pipeline) => { Ok(pipeline) => {
let exit_code = pipeline.print(&engine_state, stack, false, false)?; let exit_code =
pipeline.print_not_formatted(&engine_state, false, false)?;
if exit_code != 0 { if exit_code != 0 {
break; break;
} }

View file

@ -78,7 +78,7 @@ impl Command for While {
} }
Ok(pipeline) => { Ok(pipeline) => {
let exit_code = let exit_code =
pipeline.print(engine_state, stack, false, false)?; pipeline.print_not_formatted(engine_state, false, false)?;
if exit_code != 0 { if exit_code != 0 {
break; break;
} }

View file

@ -462,5 +462,9 @@ pub fn create_default_context() -> EngineState {
eprintln!("Error creating default context: {err:?}"); eprintln!("Error creating default context: {err:?}");
} }
// Cache the table decl id so we don't have to look it up later
let table_decl_id = engine_state.find_decl("table".as_bytes(), &[]);
engine_state.table_decl_id = table_decl_id;
engine_state engine_state
} }

View file

@ -110,7 +110,6 @@ impl Command for Table {
let list: bool = call.has_flag("list"); let list: bool = call.has_flag("list");
let width_param: Option<i64> = call.get_flag(engine_state, stack, "width")?; let width_param: Option<i64> = call.get_flag(engine_state, stack, "width")?;
let term_width = get_width_param(width_param);
let expand: bool = call.has_flag("expand"); let expand: bool = call.has_flag("expand");
let expand_limit: Option<usize> = call.get_flag(engine_state, stack, "expand-deep")?; let expand_limit: Option<usize> = call.get_flag(engine_state, stack, "expand-deep")?;
@ -152,7 +151,7 @@ impl Command for Table {
input, input,
row_offset, row_offset,
table_view, table_view,
term_width, width_param,
) )
} }
@ -232,7 +231,7 @@ fn handle_table_command(
input: PipelineData, input: PipelineData,
row_offset: usize, row_offset: usize,
table_view: TableView, table_view: TableView,
term_width: usize, term_width: Option<i64>,
) -> Result<PipelineData, ShellError> { ) -> Result<PipelineData, ShellError> {
let ctrlc = engine_state.ctrlc.clone(); let ctrlc = engine_state.ctrlc.clone();
let config = engine_state.get_config(); let config = engine_state.get_config();
@ -278,18 +277,22 @@ fn handle_table_command(
ctrlc, ctrlc,
metadata, metadata,
), ),
PipelineData::Value(Value::Record { cols, vals, span }, ..) => handle_record( PipelineData::Value(Value::Record { cols, vals, span }, ..) => {
cols, let term_width = get_width_param(term_width);
vals,
span, handle_record(
engine_state, cols,
stack, vals,
call, span,
table_view, engine_state,
term_width, stack,
ctrlc, call,
config, table_view,
), term_width,
ctrlc,
config,
)
}
PipelineData::Value(Value::LazyRecord { val, .. }, ..) => { PipelineData::Value(Value::LazyRecord { val, .. }, ..) => {
let collected = val.collect()?.into_pipeline_data(); let collected = val.collect()?.into_pipeline_data();
handle_table_command( handle_table_command(

View file

@ -126,6 +126,7 @@ pub struct EngineState {
pub config: Config, pub config: Config,
pub pipeline_externals_state: Arc<(AtomicU32, AtomicU32)>, pub pipeline_externals_state: Arc<(AtomicU32, AtomicU32)>,
pub repl_buffer_state: Arc<Mutex<String>>, pub repl_buffer_state: Arc<Mutex<String>>,
pub table_decl_id: Option<usize>,
// A byte position, as `EditCommand::MoveToPosition` is also a byte position // A byte position, as `EditCommand::MoveToPosition` is also a byte position
pub repl_cursor_pos: Arc<Mutex<usize>>, pub repl_cursor_pos: Arc<Mutex<usize>>,
#[cfg(feature = "plugin")] #[cfg(feature = "plugin")]
@ -180,6 +181,7 @@ impl EngineState {
pipeline_externals_state: Arc::new((AtomicU32::new(0), AtomicU32::new(0))), pipeline_externals_state: Arc::new((AtomicU32::new(0), AtomicU32::new(0))),
repl_buffer_state: Arc::new(Mutex::new("".to_string())), repl_buffer_state: Arc::new(Mutex::new("".to_string())),
repl_cursor_pos: Arc::new(Mutex::new(0)), repl_cursor_pos: Arc::new(Mutex::new(0)),
table_decl_id: None,
#[cfg(feature = "plugin")] #[cfg(feature = "plugin")]
plugin_signatures: None, plugin_signatures: None,
#[cfg(not(windows))] #[cfg(not(windows))]

View file

@ -704,7 +704,7 @@ impl PipelineData {
return print_if_stream(stream, stderr_stream, to_stderr, exit_code); return print_if_stream(stream, stderr_stream, to_stderr, exit_code);
} }
if let Some(decl_id) = engine_state.find_decl("table".as_bytes(), &[]) { if let Some(decl_id) = engine_state.table_decl_id {
let command = engine_state.get_decl(decl_id); let command = engine_state.get_decl(decl_id);
if command.get_block_id().is_some() { if command.get_block_id().is_some() {
return self.write_all_and_flush(engine_state, config, no_newline, to_stderr); return self.write_all_and_flush(engine_state, config, no_newline, to_stderr);
@ -734,8 +734,6 @@ impl PipelineData {
no_newline: bool, no_newline: bool,
to_stderr: bool, to_stderr: bool,
) -> Result<i64, ShellError> { ) -> Result<i64, ShellError> {
let config = engine_state.get_config();
if let PipelineData::ExternalStream { if let PipelineData::ExternalStream {
stdout: stream, stdout: stream,
stderr: stderr_stream, stderr: stderr_stream,
@ -745,6 +743,7 @@ impl PipelineData {
{ {
print_if_stream(stream, stderr_stream, to_stderr, exit_code) print_if_stream(stream, stderr_stream, to_stderr, exit_code)
} else { } else {
let config = engine_state.get_config();
self.write_all_and_flush(engine_state, config, no_newline, to_stderr) self.write_all_and_flush(engine_state, config, no_newline, to_stderr)
} }
} }
@ -835,6 +834,7 @@ pub fn print_if_stream(
) -> Result<i64, ShellError> { ) -> Result<i64, ShellError> {
// NOTE: currently we don't need anything from stderr // NOTE: currently we don't need anything from stderr
// so we just consume and throw away `stderr_stream` to make sure the pipe doesn't fill up // so we just consume and throw away `stderr_stream` to make sure the pipe doesn't fill up
thread::Builder::new() thread::Builder::new()
.name("stderr consumer".to_string()) .name("stderr consumer".to_string())
.spawn(move || stderr_stream.map(|x| x.into_bytes())) .spawn(move || stderr_stream.map(|x| x.into_bytes()))