perf(bigint): use 64-bit limbs on native and wasm - #4164
Merged
Conversation
Collaborator
Coverage Report for CI Build 6424Coverage decreased (-0.02%) to 90.912%Details
Uncovered Changes
Coverage Regressions1 previously-covered line in 1 file lost coverage.
Coverage Stats
💛 - Coveralls |
Yu-zh
marked this pull request as draft
August 28, 2026 02:55
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ac4ed6d8d5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Yu-zh
force-pushed
the
Yu-zh/bigint
branch
5 times, most recently
from
August 31, 2026 03:28
33fbd26 to
a876bfe
Compare
Yu-zh
marked this pull request as ready for review
August 31, 2026 03:34
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Keep wasm-gc on the existing 32-bit limb implementation because wide arithmetic multi-value results regress there.
bobzhang
enabled auto-merge (squash)
August 31, 2026 07:05
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
3 participants
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
FixedArray[UInt64]limbs for BigInt on native and wasm1Implementation
%u64.mul_wideon native while keeping a portable wasm1 fallback that can adopt wide arithmetic laterTesting
moon check bigint --target all --deny-warnmoon test bigint --target all --releasemoon test --target allmoon test --release --target js,wasm,wasm-gcmoon test --target native --releasemoon info --target wasm,wasm-gc,js,nativewith no.mbtidiffmoon bundle --all