Repository navigation
perf: avoid cloning binary values - #18572
Merged
Merged
Conversation
Exercise copy-on-write mutation and plugin serialization through JSON and MessagePack. Cover short signed and unsigned integer conversions in both byte orders.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR changes Nushell’s Value::Binary storage to use SharedCow<Vec<u8>> so binary values become cheap to clone and can share allocations until mutation, and it threads that through byte streaming and conversions to avoid unnecessary copying.
Changes:
- Store binary
Values asSharedCow<Vec<u8>>and update call sites to useas_slice()/into_owned()appropriately. - Teach byte streams to accept generic binary backing storage (
AsRef<[u8]>) and add regression tests for sharing / CoW behavior. - Optimize
into intfor binary input by removing clone/padding paths and adding endian test coverage.
Reviewed changes
Copilot reviewed 29 out of 29 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/nuon/src/to.rs | Iterate binary bytes via iter() for new backing type |
| crates/nu-utils/src/shared_cow.rs | Add AsRef<[T]> for SharedCow<Vec<T>> |
| crates/nu-protocol/src/value/mod.rs | Switch Value::Binary to SharedCow + add CoW tests |
| crates/nu-protocol/src/value/from_value.rs | Ensure binary extraction uses into_owned() |
| crates/nu-protocol/src/pipeline/pipeline_data.rs | Update binary iteration / stdout writes to use slices |
| crates/nu-protocol/src/pipeline/byte_stream.rs | Generalize read_binary and add sharing test |
| crates/nu-protocol/src/engine/pattern_match.rs | Compare binary via as_slice() |
| crates/nu-plugin/src/plugin/interface/mod.rs | Return owned binary via into_owned() |
| crates/nu-plugin-core/src/serializers/tests.rs | Add binary round-trip serialization test |
| crates/nu-explore/src/explore/mod.rs | Convert binary value to owned bytes for view |
| crates/nu-explore/src/explore_config/conversion.rs | Fix binary assertion for shared backing |
| crates/nu-command/tests/commands/conversions/into/int.rs | Expand endian/signed coverage for into int |
| crates/nu-command/src/strings/encode_decode/decode.rs | Decode binary with explicit ownership when needed |
| crates/nu-command/src/strings/base/mod.rs | Convert binary to owned bytes for base conversions |
| crates/nu-command/src/network/http/client.rs | Send binary request bodies via as_slice() |
| crates/nu-command/src/formats/from/sheets.rs | Deserialize binary via into_owned() |
| crates/nu-command/src/filters/take/take_.rs | Update take binary handling for shared backing |
| crates/nu-command/src/filters/skip/skip_.rs | Update skip binary handling for shared backing |
| crates/nu-command/src/filters/last.rs | Update last binary handling for shared backing |
| crates/nu-command/src/filters/first.rs | Update first binary handling for shared backing |
| crates/nu-command/src/filesystem/save.rs | Save binary values via into_owned() |
| crates/nu-command/src/database/values/sqlite.rs | Store binary sqlite values via into_owned() |
| crates/nu-command/src/conversions/into/int.rs | Avoid cloning/padding in binary→int conversion |
| crates/nu-command/src/charting/hashable_value.rs | Hash binary by taking owned bytes |
| crates/nu-command/src/bytes/collect.rs | Adjust separator handling for shared binary chunks |
| crates/nu-command/src/bytes/build_.rs | Append binary args using owned extraction |
| crates/nu-cmd-extra/src/extra/strings/format/bits.rs | Iterate bits via iter() for new backing |
| crates/nu_plugin_formats/src/to/plist.rs | Convert binary to vec for plist output |
| benches/benchmarks.rs | Add Tango benchmarks for binary clone / slice→int |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
|
Thanks |
fdncred
pushed a commit
that referenced
this pull request
Jul 15, 2026
## Description This PR fixes unsigned binary-to-integer conversions for values that do not fit in an `i64`. `into int` previously converted the `u64` returned by `read_uint` using `as i64`. Eight-byte values greater than `i64::MAX` therefore wrapped into negative integers, despite `--signed` not being specified. The conversion now uses `i64::try_from` and returns `ShellError::IncorrectValue` when the unsigned value is too large. Signed conversions are unchanged. Regression tests cover `i64::MAX` and the first overflowing value in both little-endian and big-endian representations. ## User-facing changes (Release notes) Error early instead of letting `into int` silently wrap unsigned 8-byte binary values greater than `i64::MAX` into negative values. ## Additional notes Follow-up to the correctness issue deferred from #18572: #18572 (comment)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Store binary
Values inSharedCow<Vec<u8>>so clones share their backing allocation until mutation.We use that storage when creating byte streams, and read binary input directly in
into intwithout cloning or padding it.Also add tests for copy-on-write behavior, shared byte streams, plugin serialization, endian conversions, and generic Tango benchmarks.
User-facing changes (Release notes)
Faster large binary value processing
Large binary values are now substantially cheaper to clone, stream, slice, and convert.
Commands that repeatedly process large binary values now avoid copying the entire value at each step, with repeated slicing and integer conversion roughly 3.4x faster.
Additional notes
Tango results from 100 paired samples against upstream
main(e8424fe).binary_value_clone_2097152bbinary_slice_into_int_2097152b_100_readsbinary_skip_shared_2097152b_half_100_readsA separate fixed-seed run produced consistent results: 43.1 µs → 12.3 ns and 13.9 ms → 4.3 ms.