perf(bytes): skip redundant initialization in concatenation - #4142
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Optimizes Bytes concatenation on non-JS targets by avoiding redundant zero-initialization, while keeping the existing JS implementation for compatibility.
Changes:
- Refactors
Bytes + Bytesto delegate to a target-specificbytes_add_impland uses uninitialized allocation + blits on non-JS targets. - Adds a regression test to ensure the concatenation result is fully initialized and correctly contains both inputs.
- Adds a benchmark covering large
Bytesconcatenation.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| builtin/bytes.mbt | Implements a non-JS fast path for Bytes::add using uninitialized allocation and two blits; keeps the JS path unchanged. |
| builtin/bytes_test.mbt | Adds a test validating Bytes::add initializes and populates the full result correctly. |
| builtin/bytes_add_bench_test.mbt | Introduces a benchmark for large Bytes concatenation. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
bobzhang
force-pushed
the
perf/bytes-add-uninitialized
branch
from
August 31, 2026 03:22
cd35201 to
ca74108
Compare
CAIMEOX
added a commit
that referenced
this pull request
Aug 31, 2026
* Handle minimum signed values in QuickCheck shrinking * Assert complete minimum-value shrink sequences * docs(builtin): expand documentation for integer min/max/clamp `Int64::min`, `Int64::max`, `Int64::clamp`, `UInt::min`, `UInt::max` and `UInt::clamp` carried one-line comments while their documented siblings follow the full structure. Bring all six to the same level: semantics, parameters, return behavior (including the abort when `min > max` in `clamp`) and executable examples. Continues #623 * chore: remove deprecated APIs * perf(bytes): skip redundant initialization in concatenation (#4142) * perf(bytes): skip redundant initialization in concatenation * Limit Bytes benchmark setup to benchmark runs * refactor(bigint): read each from_octets limb with a u32be pattern (#4099) * refactor(bigint): read each limb with a u32be pattern in from_octets The inner loop was a 4-iteration big-endian shift-accumulate, which is exactly one u32be read (RADIX_BIT_LEN = 32). Measured: native -34% at n=1024 / -24% at n=64, wasm-gc -46% / -42%. js is unaffected (bigint_js.mbt has its own from_octets). * bench(bigint): add from_octets benchmarks Covers the limb-fill path at n=64 and n=1024 so the u32be conversion in the previous commit is reproducible. Signed-off-by: Codex CLI <codex@openai.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(bigint): pin RADIX_BIT_LEN so the u32be limb read can't drift Addresses the review note that `from_octets`'s tail now hard-codes a 32-bit read while `byte_per_limb` is still derived from `RADIX_BIT_LEN`. Rather than branch on the constant at runtime, the assumption is pinned by a whitebox test and named in the comment above the loop: narrowing `RADIX_BIT_LEN` now fails a test instead of silently mis-decoding. The pin uses `@test.assert_eq`, not `inspect`. A snapshot would be rewritten by `moon test --update`, which would retire the guard at exactly the moment it is supposed to fire; `assert_eq` is not auto-updatable. Verified both halves by temporarily setting the expected value to 16: the test fails with `32 != 16`, and `moon test --update` leaves it failing rather than rewriting it. A runtime fallback was considered and rejected. Restoring the old shift-accumulate loop as the "generic" arm would not actually be generic: `byte_per_limb` is `RADIX_BIT_LEN / 8`, so for the documented-legal widths 4, 12, 20 and 28 (the file only requires a multiple of 4, at most 32) that loop truncates and mis-decodes just as badly. It would trade a loud failure for a quiet one, plus a permanently dead branch that no test can reach. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Signed-off-by: Codex CLI <codex@openai.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat: improve scalar Arbitrary * use integer literal suffix or constructor * fix: improve test strength * refactor: uniform Byte generation * adjust shrinking order of 0 * perf(bigint): use 64-bit limbs on native and wasm (#4164) Keep wasm-gc on the existing 32-bit limb implementation because wide arithmetic multi-value results regress there. --------- Signed-off-by: Codex CLI <codex@openai.com> Co-authored-by: Li Fengmin <2080291162@qq.com> Co-authored-by: Yu Zhang <yu.zhang.yz862@yale.edu> Co-authored-by: Hongbo Zhang <bobzhang1988@gmail.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: CAIMEO <38813005+CAIMEOX@users.noreply.github.com>
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.
Summary
Avoid zero-initializing the concatenation result on non-JS targets before both inputs overwrite the full allocation. Allocate uninitialized storage, fill the two non-overlapping ranges with blits, then reinterpret the fully initialized buffer as
Bytes. JS keeps the existing implementation because the required intrinsic is unavailable there.Benchmark
Native release, 500,000 + 500,000 bytes, averaged over eight interleaved base/head runs:
Validation
moon test builtin --target allmoon check --target allmoon info(no interface changes)