-
Notifications
You must be signed in to change notification settings - Fork 5k
BigInt("0x…"/"0b…"/"0o…"): linear-time string parse #35899
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
robobun
wants to merge
3
commits into
main
Choose a base branch
from
farm/5ff878ec/bigint-parse-pow2-linear
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+115
−1
Open
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,114 @@ | ||
| import { describe, expect, test } from "bun:test"; | ||
|
|
||
| // JSBigInt::parseInt historically routed every radix through a loop that | ||
| // calls multiplyAdd over the full-length digit vector for each small group | ||
| // of characters, which is O(n^2) in the number of characters. For | ||
| // power-of-two radixes (0b/0o/0x prefixes) the parse is a straight bit-pack | ||
| // and should be O(n), matching toStringBasePowerOfTwo in the other direction. | ||
|
|
||
| // Buffer.alloc semantics: `len` is the OUTPUT length in bytes; `fill` is tiled | ||
| // into it. Not `fill.repeat(len)`. | ||
| const fill = (len: number, pattern: string) => Buffer.alloc(len, pattern).toString(); | ||
|
|
||
| describe("BigInt string parse, power-of-two radix", () => { | ||
| const roundtrip = (prefix: string, radix: number, body: string) => { | ||
| const v = BigInt(prefix + body); | ||
| expect(v.toString(radix)).toBe(body.toLowerCase().replace(/^0+(?=.)/, "")); | ||
| return v; | ||
| }; | ||
|
|
||
| test("hex correctness", () => { | ||
| roundtrip("0x", 16, "1"); | ||
| roundtrip("0x", 16, "F"); | ||
| roundtrip("0x", 16, "DeadBeef"); | ||
| roundtrip("0x", 16, fill(16, "f")); // exactly one 64-bit word | ||
| roundtrip("0x", 16, fill(17, "f")); // one word + 4 bits | ||
| roundtrip("0x", 16, "1" + fill(100, "0")); | ||
| roundtrip("0x", 16, fill(1000, "f")); | ||
| roundtrip("0x", 16, fill(3200, "123456789abcdef0")); // 200 full words, mixed digits | ||
| // Non-16-aligned lengths to hit partial high digits. | ||
| for (let n = 1; n <= 40; n++) roundtrip("0x", 16, fill(n, "a")); | ||
| }); | ||
|
|
||
| test("binary correctness", () => { | ||
| roundtrip("0b", 2, "1"); | ||
| roundtrip("0b", 2, fill(63, "1")); | ||
| roundtrip("0b", 2, fill(64, "1")); | ||
| roundtrip("0b", 2, fill(65, "1")); | ||
| roundtrip("0b", 2, fill(1000, "1")); | ||
| roundtrip("0b", 2, fill(1000, "10")); | ||
| for (let n = 1; n <= 130; n++) roundtrip("0b", 2, fill(n, "1")); | ||
| }); | ||
|
|
||
| test("octal correctness (3 bits/char, spans digit boundaries)", () => { | ||
| roundtrip("0o", 8, "7"); | ||
| roundtrip("0o", 8, fill(21, "7")); | ||
| roundtrip("0o", 8, fill(22, "7")); | ||
| roundtrip("0o", 8, fill(23, "7")); | ||
| roundtrip("0o", 8, fill(1400, "1234567")); | ||
| roundtrip("0o", 8, fill(1000, "7")); | ||
| // The msb of the leading char can land in the low bits of the next | ||
| // 64-bit word, leaving that word zero. Exercise every alignment. | ||
| for (let n = 1; n <= 70; n++) roundtrip("0o", 8, fill(n, "1")); | ||
| for (let n = 1; n <= 70; n++) roundtrip("0o", 8, fill(n, "7")); | ||
| }); | ||
|
|
||
| test("cross-radix agreement on large values", () => { | ||
| const hex = fill(4096, "f"); | ||
| const v16 = BigInt("0x" + hex); | ||
| const v2 = BigInt("0b" + fill(4096 * 4, "1")); | ||
| expect(v16 === v2).toBe(true); | ||
| expect(v16.toString(16)).toBe(hex); | ||
| expect(v16).toBe((1n << BigInt(4096 * 4)) - 1n); | ||
| }); | ||
|
|
||
| test("leading zeros, whitespace, and 16-bit string storage", () => { | ||
| expect(BigInt("0x" + fill(1000, "0") + "ff")).toBe(255n); | ||
| expect(BigInt(" 0x" + fill(100, "f") + " ")).toBe(BigInt("0x" + fill(100, "f"))); | ||
| expect(BigInt("0x" + fill(1000, "0"))).toBe(0n); | ||
| // Leading zeros pushing the raw character count past the 2^20-bit cap must | ||
| // still parse: the length check runs after zeros are stripped. | ||
| expect(BigInt("0x" + fill(300_000, "0") + "ff")).toBe(255n); | ||
| // U+2003 EM SPACE is a legal StrWhiteSpaceChar and forces 16-bit string | ||
| // storage, so this routes through the UChar instantiation of parseInt. | ||
| const body = fill(1000, "f"); | ||
| expect(BigInt("\u2003" + "0x" + body + "\u2003")).toBe(BigInt("0x" + body)); | ||
| }); | ||
|
|
||
| test("maxLength boundary (2^20 bits)", () => { | ||
| const atLimit = BigInt("0x" + fill(262_144, "f")); | ||
| expect(atLimit.toString(16).length).toBe(262_144); | ||
| expect(() => BigInt("0x" + fill(262_145, "f"))).toThrow(RangeError); | ||
| }); | ||
|
|
||
| test("invalid characters still throw SyntaxError", () => { | ||
| expect(() => BigInt("0x" + fill(1000, "f") + "g")).toThrow(SyntaxError); | ||
| expect(() => BigInt("0b" + fill(1000, "1") + "2")).toThrow(SyntaxError); | ||
| expect(() => BigInt("0o" + fill(1000, "7") + "8")).toThrow(SyntaxError); | ||
| expect(() => BigInt("0xg" + fill(1000, "f"))).toThrow(SyntaxError); | ||
| }); | ||
|
|
||
| test("parse is linear, not quadratic", () => { | ||
| // JSC caps BigInt at 2^20 bits (262144 hex chars / 349525 octal chars). | ||
| // Use 250000 chars: the O(n^2) path takes ~570ms hex and ~300ms octal on a | ||
| // release build (seconds under debug), while the O(n) path stays under | ||
| // ~10ms even under debug+ASAN. | ||
| const n = 250_000; | ||
| const hex = "0x" + fill(n, "f"); | ||
| const oct = "0o" + fill(n, "7"); | ||
|
|
||
| const t0 = performance.now(); | ||
| const vHex = BigInt(hex); | ||
| const hexMs = performance.now() - t0; | ||
|
|
||
| const t1 = performance.now(); | ||
| const vOct = BigInt(oct); | ||
| const octMs = performance.now() - t1; | ||
|
|
||
| expect(vHex.toString(16)).toBe(fill(n, "f")); | ||
| expect(vOct.toString(8)).toBe(fill(n, "7")); | ||
|
|
||
| expect(hexMs).toBeLessThan(250); | ||
| expect(octMs).toBeLessThan(250); | ||
| }); | ||
| }); |
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴
WEBKIT_VERSIONis pointing atautobuild-preview-pr-353-4b51ec68, an ephemeral preview-build release for the still-unmerged oven-sh/WebKit#353. Preview releases are deleted once the upstream PR merges/closes, at which point every fresh prebuilt-mode build (CI and local) will 404 on the WebKit download. Per the description this is a known TODO — flagging so it doesn't slip: bump to the merged commit sha before landing.Extended reasoning...
What the bug is
scripts/build/deps/webkit.ts:6sets:replacing the previous 40-hex commit sha. The
autobuild-preview-pr-*tag is the CI preview release that oven-sh/WebKit publishes for open PRs so downstream Bun PRs can test against them before the WebKit change lands. These preview releases are ephemeral — they are deleted (or become stale/unreferenced) once the WebKit PR is merged or closed and the realautobuild-<sha>release is published for the merged commit.Code path that triggers it
prebuiltUrl()in the same file builds the download URL directly from this constant:so every
cfg.webkit === "prebuilt"build — which is the default for CI and for anyone not running a local WebKit checkout — fetcheshttps://github.com/oven-sh/WebKit/releases/download/autobuild-preview-pr-353-4b51ec68/bun-webkit-<os>-<arch><suffix>.tar.gz.prebuiltDestDir()also keys the cache dir on this string, so there is no fallback to a previously-cached sha.Why nothing else prevents it
There is no guard in the build system that rejects preview tags or falls back to a pinned release;
prebuiltUrl()explicitly accommodates theautobuild-prefix so preview tags work during development. The only thing preventing this from breaking builds today is that oven-sh/WebKit#353 is still open and its preview artifacts still exist.Impact
If this PR merges as-is and oven-sh/WebKit#353 subsequently merges (which it must, since this PR depends on it), the preview release will be cleaned up. From that point every fresh clone / cache-cold CI runner hits a 404 downloading WebKit and the build fails outright — a regression in something that currently works on
main. Even before deletion, pinningmainto an unmerged, force-pushable PR branch is fragile.Step-by-step proof
WEBKIT_VERSION = "autobuild-preview-pr-353-4b51ec68".autobuild-preview-pr-353-4b51ec68is deleted per the preview-release lifecycle.bun bd.resolveDepfor WebKit computesprebuiltUrl(cfg)→.../releases/download/autobuild-preview-pr-353-4b51ec68/bun-webkit-linux-amd64.tar.gz.WEBKIT_VERSION.Fix
Before merging, land oven-sh/WebKit#353 first, then update this line to the merged commit's 40-hex sha (matching the
autobuild-<sha>release), e.g.:The PR description already states this intent ("will be updated to the merged sha once oven-sh/WebKit#353 lands"); this comment is the merge-blocking reminder so it can't be forgotten.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, intentional and tracked: PR is in draft until oven-sh/WebKit#353 lands, then this line gets swapped to the merged 40-hex sha. Leaving this thread open as the merge-blocker.