refactor(bigint): read each from_octets limb with a u32be pattern - #4099
refactor(bigint): read each from_octets limb with a u32be pattern#4099bobzhang wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Refactors a few multi-byte integer assembly sites to use MoonBit bits patterns (u32be/u32le/u64be/u64le) where benchmarking showed neutral-or-better performance across native/wasm-gc/js backends, and adds a reproducible benchmark for the BigInt::from_octets change.
Changes:
BytesView::to_uint_be/leandto_uint64_be/lenow use bits-pattern matching instead of manual byte-shift assembly.BigInt::from_octetstail limb construction now uses a singleu32beread per limb (matchingRADIX_BIT_LEN = 32today).- Adds a benchmark test file for
BigInt::from_octetsperformance reproducibility.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| builtin/bytesview.mbt | Replaces manual byte shifting with u32be/u32le/u64be/u64le pattern reads in deprecated converters. |
| bigint/bigint_nonjs.mbt | Refactors from_octets tail loop to a u32be pattern read per 32-bit limb. |
| bigint/from_octets_bench_test.mbt | Adds benchmark tests for BigInt::from_octets on representative input sizes. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| let byte_per_limb = RADIX_BIT_LEN / 8 | ||
| // tail | ||
| // tail; each limb is exactly one big-endian RADIX_BIT_LEN-bit word | ||
| for i in 0..<div { | ||
| for j in 0..<byte_per_limb { | ||
| let bytes_idx = len - byte_per_limb - i * byte_per_limb + j | ||
| limbs[i] = (limbs[i] << 8) | input[bytes_idx].to_uint() | ||
| } | ||
| guard! input[len - byte_per_limb - i * byte_per_limb:] is [u32be(word), ..] | ||
| limbs[i] = word | ||
| } |
There was a problem hiding this comment.
Good catch on the assumption, but I don't think the suggested remedy holds up — the fallback would not actually be generic.
byte_per_limb is RADIX_BIT_LEN / 8, and the file only requires RADIX_BIT_LEN to be a multiple of 4 and at most 32 (lines 79-80). So the legal widths are 4, 8, 12, ... 32, and for 4, 12, 20 and 28 that division truncates — the old shift-accumulate loop reads 0, 1, 2 or 3 bytes into a limb that is 4, 12, 20 or 28 bits wide and mis-decodes just as badly as a hard-coded u32be would. Restoring it as the "safe" arm would convert a loud failure into a quiet one, and add a branch no test can reach (patch coverage here is currently 100%).
So I pinned the assumption instead, in 09f2b3c:
bigint_nonjs_wbtest.mbtgets a test assertingRADIX_BIT_LEN == 32, with a comment explaining thatfrom_octets'su32beread is what depends on it.- The comment above the tail loop now names the assumption and points at that test.
Narrowing the constant now fails a test rather than silently mis-decoding octets — which is the outcome you were after.
Worth adding: the new code cannot silently mis-decode even without the test. At any byte_per_limb < 4 the very first tail iteration slices fewer than four bytes, so guard! ... is [u32be(word), ..] fails and it aborts.
moon test --target all is green: wasm 7444, wasm-gc 7445, js 7388, native 7361.
There was a problem hiding this comment.
Follow-up: the pin now uses @test.assert_eq(RADIX_BIT_LEN, 32) rather than inspect(..., content="32") (ba03090).
inspect would have made the guard self-defeating. Its expected value is a snapshot, so a routine moon test --update after narrowing the constant would rewrite content="32" to the new value and the test would go green — retiring 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:
moon testfails withFAILED: 32 != 16, so the pin is live and not skipped by backend gating.moon test --updateleaves it failing and does not touch the source, confirming the durability claim.
Coverage Report for CI Build 6167Coverage decreased (-0.001%) to 90.681%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
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).
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>
f43a8d3 to
09f2b3c
Compare
bobzhang
changed the title
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>
09f2b3c to
ba03090
Compare
Summary
BigInt::from_octets's tail loop assembles each limb frombyte_per_limbindexed byte reads plus constant shifts. SinceRADIX_BIT_LEN = 32, that inner loop is exactly one big-endian 32-bit read, so it collapses to a singleu32bebits pattern. Five lines become two.Plus
bigint/from_octets_bench_test.mbtso the result is reproducible.Why this site and not others
The pattern's cost is constructing a view, and the payoff scales with how many byte operations that view replaces:
u32/u64at an offset)u16at an offset)That last row is why
builtin/bitstring.mbt'sunsafe_extract_byteis not here. One site that regressed badly is split into #4100 and deliberately not in this PR.Performance
moon bench --release, 3 runs per backend per revision, means reported.BigInt::from_octetsjs is a genuine control:
bigint_js.mbtcarries its ownfrom_octets, which this PR does not touch, and it duly did not move.Review response — the hard-coded 32-bit read
Copilot flagged that the tail now hard-codes
u32bewhilebyte_per_limbis still derived fromRADIX_BIT_LEN, and suggested guarding the fast path behindRADIX_BIT_LEN == 32with the old shift-accumulate loop as a generic fallback.The concern is right; the suggested remedy is not, because that fallback would not actually be generic.
byte_per_limbisRADIX_BIT_LEN / 8, and the file only requiresRADIX_BIT_LENto be a multiple of 4 and at most 32. For the documented-legal widths 4, 12, 20 and 28 the division truncates and the old loop mis-decodes exactly as badly — it would trade a loud failure for a quiet one, and add a branch no test can ever reach (this PR's patch coverage is currently 100%).So the assumption is pinned instead: a whitebox test asserts
RADIX_BIT_LEN == 32, and the comment above the loop names it. Narrowing the constant now fails a test rather than silently mis-decoding octets.The pin deliberately uses
@test.assert_eq, notinspect. A snapshot would be rewritten bymoon test --update, which would retire the guard at exactly the moment it is supposed to fire. Both halves are verified: temporarily setting the expected value to 16 fails with32 != 16, andmoon test --updateleaves it failing rather than quietly rewriting it.For what it's worth, the new code cannot silently mis-decode even without the test: at any
byte_per_limb < 4the first tail iteration slices fewer than four bytes and theguard!panics.Test plan
moon test --target all— wasm 7444, wasm-gc 7445, js 7388, native 7361, zero failuresmoon check --deny-warncleanmoon fmtandmoon info --target wasm,wasm-gc,js,nativeproduce no diff — no public signature is touchedDropped:
BytesViewconverter measurements (superseded by #4101, which deleted these functions)4096-byte sweep,
moon bench --release:to_uint_beto_uint_leto_uint64_beto_uint64_leSigned-off-by: Codex CLI codex@openai.com
🤖 Generated with Claude Code