Repository navigation
[JSC] The streaming JSON parser finds the end of a value that , ] or } follows - #799
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and did not find any bugs; since it edits the shared non-reviver JSON lexer hot path in Source/JavaScriptCore (a CODEOWNERS-covered area) and the regression tests live in the Bun repo rather than here, a human look is still worthwhile.
What was reviewed:
- Checked that the two setters agree:
lex()storesm_ptrbefore skipping whitespace andnextAfterValue()stores it withm_ptralready on the delimiter, so both record the position immediately after the previous token; whitespace-after-value cases still fall through tonext()/lex(). - Confirmed every value-completing path (
parsePrimitiveValue, array/object close inparseRecursivelyincluding the cached-shape branch, and the iterativeparse()fallback) ends in eithernextAfterValue()orlex(), sotryStreamingParse()always sees a fresh end-of-value position. - Traced
tryStreamingParse()on{"id":1},\n: it now returns the value withread= 8 and an "Unexpected content after JSON value" error, matching the described 1.4.2 behavior; the returned-value-plus-Error shape on invalid JSONL is pre-existing, not introduced here. - The store is gated by
USE(BUN_JSC_ADDITIONS)and skipped entirely in reviver mode, so upstream JSON.parse behavior is unchanged.
Extended reasoning...
The change adds a single m_positionAfterLastToken = m_ptr; store (gated by USE(BUN_JSC_ADDITIONS)) in the fast-path Lexer::nextAfterValue() in Source/JavaScriptCore/runtime/LiteralParser.cpp plus a one-line comment in LiteralParser.h; it touches no security-sensitive surface (no auth, crypto, or untrusted-memory handling beyond the existing bounded lexer). The fix is minimal and I verified from the code that it makes the two producers of the position consistent and that every value-completion path sets it, so the described duplicate-value/quadratic bug in Bun.JSONL.parseChunk is addressed. I deferred rather than approved because /Source/JavaScriptCore is covered by .github/CODEOWNERS, the edit sits in ALWAYS_INLINE hot-path lexer code shared with JSON.parse, and no tests accompany the change in this repository (the author states they are in oven-sh/bun, which cannot be verified here).
|
Preview build of dd3b20d: |
… or `}` follows tryStreamingParse() (Bun.JSONL) takes the end of each value from Lexer::positionAfterLastToken(), which only Lexer::lex() sets. Since the merge of upstream dbdca75 the token after a value comes from Lexer::nextAfterValue(), which consumes `,` `}` and `]` without a call to lex(). With one of the three right after a top-level value the position was the one of an earlier lex() call: in front of the value, or at the last key lexed inside it. The consumed count was reported from there, in front of a value that had already been returned. Where only newlines lay between that position and the value, the check for content after the value passed, the lexer went back to the next line, and the same value was parsed again, once per blank line: Bun.JSONL.parseChunk("\n\n7,\n") // values [7, 7, 7], read 2; expected [7], read 3 For these three tokens tryStreamingParse() now walks back from the token, which is one character long, over the whitespace in front of it. That holds whichever way the token was lexed. Each of them is an error there, so the walk happens once per call. Setting the position in nextAfterValue() fixes it as well, but every value JSON.parse sees goes through there: 1.45% more instructions on arrays of integers and of small objects. This way the lexer is not touched. * Source/JavaScriptCore/runtime/LiteralParser.cpp: (JSC::LiteralParser<CharType, reviverMode>::tryStreamingParse):
97c108d to
dd3b20d
Compare
There was a problem hiding this comment.
I reviewed the updated version of this PR and didn't find any bugs; since the approach changed after the earlier review (the fix now lives in tryStreamingParse rather than in Lexer::nextAfterValue as the PR description still says), a human look at the final shape would still be worthwhile, and the description could be refreshed to match.
What was reviewed:
- Checked that both Lexer::lex() and nextAfterValue() consume
,}]as exactly one character, som_lexer.ptr() - 1is the token start whichever path produced it. - Checked the unbounded backward whitespace walk: the token is only reached after a value parsed successfully, and every value form ends in a non-whitespace character, so the loop stops at the value's last character before reaching start().
- Walked the examples from the description (
"\n\n7,\n","[\n7]}\n") through the new code: read offsets land after the value and the error/newline check sees the stray token, so values are no longer re-parsed per blank line. - Confirmed the non-recursive parse() path and the TokEnd/newline path still use positionAfterLastToken(), which lex() sets correctly there.
Extended reasoning...
The change is a 12-line edit inside the USE(BUN_JSC_ADDITIONS)-only tryStreamingParse in Source/JavaScriptCore/runtime/LiteralParser.cpp, computing the end of a top-level JSONL value by walking back from a , } or ] token instead of relying on a lexer field that nextAfterValue() does not update; the lexer itself is untouched. It touches no security-sensitive surface (no auth, injection, or data exposure), only JSONL parse offsets. The code reads as correct, but the PR description describes a different fix (in nextAfterValue) than what the commit implements, the approach changed after a prior review whose content is not visible here, tests live only in the Bun repo, and the path is covered by the repository's CODEOWNERS entry for Source/JavaScriptCore, so a human confirmation is the safer call rather than an automated approval.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
Walkthrough
ChangesStreaming parse value-end tracking
Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Linked Issues check |  |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
| Title check | |
The title clearly identifies the main change: finding the end of a JSON value when followed by a comma, closing bracket, or closing brace. |
| Description check | |
The description is detailed and directly explains the bug, fix, performance tradeoff, scope, and test results. It does not include the required Bugzilla link, reviewer line, or explicit changed-file/f… |
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Warning
Git: CodeRabbit could not clone the repository, so clone-backed analysis was skipped and this review may be incomplete. Verify repository clone access, such as SSH credentials, before requesting another full review. If clone access is intentionally unavailable, use path_filters to narrow the review scope.
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
… a value (#45190) ### What does this PR do? Fixes a regression in Bun 1.4.3. When `,` `]` or `}` came right after a complete value: - `Bun.JSONL.parseChunk` reported a `read` offset in front of a value it had already returned. The streaming loop in the docs (`buffer = buffer.slice(result.read)`) then got the same value again on the next call. - `Bun.JSONL.parseChunk` and `Bun.JSONL.parse` returned the value once more for each blank line in front of it, in quadratic time. Strings and typed arrays were affected alike. The error for such input is intended and stays. Input with one value per line was never affected. | input | 1.4.2 and this PR | 1.4.3 | |---|---|---| | `'{"id":1},\n{"id":2}\n'` | `[{"id":1}]`, read 8 | `[{"id":1}]`, read 1 | | `"1,2\n"` | `[1]`, read 1 | `[1]`, read 0 | | `"[1]]\n[2]\n"` | `[[1]]`, read 3 | `[[1]]`, read 1 | | `"\n\n7,\n"` | `[7]`, read 3 | `[7,7,7]`, read 2 | | `"[\n7]}\n"` | `[[7]]`, read 4 | `[[7],7]`, read 2 | | 100,000 blank lines, then `7,` | `[7]` | 100,001 copies of `7`, 2.7 s | **Cause.** The streaming parser behind `Bun.JSONL` takes the end of each value from a position that only `Lexer::lex()` records. The WebKit upgrade in #44615 brought in `Lexer::nextAfterValue()`, which consumes `,` `}` `]` after a value without `lex()`. The position then still pointed in front of the value, or at the last key inside it. When only newlines lay between that position and the value, the parser took them for the end of the line, moved on one line, and parsed the same value again. **Fix.** oven-sh/WebKit#799 (merged): for these three tokens `tryStreamingParse()` walks back from the token over the whitespace in front of it. Only Bun's own function changes. The lexer and `JSON.parse` are not touched. This PR moves `WEBKIT_VERSION` from `0c06faad` to `3d325715`, the merge commit of #799, and adds the tests. **The bump also brings in** the two changes that merged to the fork's `main` after the current pin: - oven-sh/WebKit#774: `addErrorInfo()` drops what the host's stack formatter throws, so `new Function("(")` passes `validateExceptionChecks`. - oven-sh/WebKit#787: the heap checks for a collection once per block of reported extra memory, not on every report. ### How did you verify your code works? **Tests.** 46 new tests in `test/js/bun/jsonl/jsonl-parse.test.ts`. They run 17 values (numbers, strings, literals, objects, arrays, non-ASCII, multi-line) after 6 kinds of preceding lines, as a string, a UTF-16 string, a `Uint8Array` and a non-ASCII `Uint8Array`, and compare the whole `parseChunk` result and the `parse` result. There are also cases for 10,000 blank lines, an object whose shape the JSON key cache knows, nesting deeper than the native stack, a `start` offset, and slicing at `read`. | build | whole file | |---|---| | 1.4.2 | 315 pass | | 1.4.3 | 289 pass, 26 fail | | debug build of `main` (c69a636) | same failures as 1.4.3 (run on an earlier version of the tests) | | debug build of this PR | 315 pass | 20 of the 46 pass on 1.4.3 too: whitespace before the token, the token on a later line, and `x` in its place. They cover the walk over whitespace that the fix adds, and the path that still uses the recorded position. **Comparison with 1.4.2.** Every sequence of up to 5 tokens out of 18 (`[ ] { } , : "a" 1 \n space true x {"a":1} [1] " - 1.5 {"a":"b","c":2}`), 2,000,718 inputs. For each one the `parseChunk` and `parse` results, and up to 4 tokens also as UTF-16, as bytes, as non-ASCII bytes and with a `start` offset. | build | inputs that differ from 1.4.2 | |---|---| | 1.4.3 | 161,568 | | this PR | 0 | **`JSON.parse` cost.** `bin/jsc` of `bun-webkit-linux-amd64-lto`, an array of 20,000 integers and an array of 4,000 small objects parsed 400 times, `perf stat -e instructions:u`, three runs each. The first version of the WebKit fix recorded the position in `nextAfterValue()`. It gave the same 0 differences, but every value `JSON.parse` sees goes through there, so it was dropped. | WebKit build | instructions | |---|---| | fork `main` (5d2ceba2) | 4.405 to 4.411 G | | position recorded in `nextAfterValue()` (97c108d8) | 4.470 to 4.475 G | | oven-sh/WebKit#799 (dd3b20d5) | 4.404 to 4.408 G | All of the above ran on Linux x64. In CI the file also passes on macOS arm64 (315 pass, build 125228). **Memory tests and oven-sh/WebKit#787.** In build 125228 `structured-clone-fastpath.test.ts` ("constant amount of memory for simple object inputs", limit 1,024 KB) measured 1,776 KB once on macOS arm64, inside the parallel batch, and passed when the runner ran it alone. It is the only time that file shows up in the annotations of the last 100 builds, so I compared CI's own `bun-linux-x64` binaries: `main` at c69a636 (build 125192), which is the base of this branch, and this branch (build 125228). The WebKit bump is the only difference between them. Real test file, with the measured value printed, CI's environment variables, runs interleaved. RSS growth in the object test, the one that failed (limit 1,024 KB): | | main | this PR | |---|---|---| | alone, 150 runs: median | -36 KB | -36 KB | | alone, 150 runs: max | 512 KB | 512 KB | | after 20 other test files in the same process, 24 runs: max | 512 KB | 512 KB | | runs at or over the limit | 0 | 0 | RSS growth in the string test next to it (limit 8,192 KB): | | main | this PR | |---|---|---| | alone, 150 runs: median | 512 KB | 512 KB | | alone, 150 runs: max | 1,596 KB | 1,536 KB | | alone, 150 runs at 1,024 KB or more | 20 | 48 | So the bump does keep one more 512 KB string alive more often in the string test, which fits what #787 says about itself (a collection can start a little later). The maximum does not move and stays far under that test's limit. The object test does not change on Linux. I could not reproduce the macOS result and have no macOS machine, so it is not explained. One-off failures in the parallel batch are common: 65 different test and platform pairs in those 100 builds. **The failed job in build 125228** (1 of 181) was `darwin any aarch64 - test-bun`. `s3-list-objects.test.ts` failed all 4 attempts with `Secret not found: S3_R2_ACCESS_KEY`, because the agent had `no route to host` for agent.buildkite.com from 22:35:34 to 22:36:40 UTC. The test body never ran.
1 participant
Problem
Bun.JSONL.parseChunkis wrong in Bun 1.4.3 when,]or}comes right after a complete value.readpoints in front of a value that was already returned, and the value is returned once more per blank line in front of it, in quadratic time.Bun.JSONL.parsereturns the same extra copies. Bun 1.4.2 is right.'{"id":1},\n{"id":2}\n'[{"id":1}], read 8[{"id":1}], read 1"1,2\n"[1], read 1[1], read 0"\n\n7,\n"[7], read 3[7,7,7], read 2"[\n7]}\n"[[7]], read 4[[7],7], read 27,[7]7, over 2 stryStreamingParse()takes the end of each value fromLexer::positionAfterLastToken(). OnlyLexer::lex()sets it.Since Bun/upgrade to dbdca7545d #773 (upstream dbdca75) the token after a value comes from
Lexer::nextAfterValue(). It consumes,}]without a call tolex(), so the position stays where an earlierlex()call left it: in front of the value, or at the last key lexed inside it.When only newlines lie between that position and the value, the check for content after the value passes, the lexer goes back to the next line, and the same value is parsed again.
Fix
,}or],tryStreamingParse()walks back from the token, which is one character long, over the whitespace in front of it. That gives the end of the value whichever way the token was lexed. For every other tokennextAfterValue()goes throughlex(), and the recorded position is right.tryStreamingParse()changes, which is Bun's own. The lexer andJSON.parseare not touched.Checks
Bun built against the preview build of this branch (dd3b20d), Linux x64, in oven-sh/bun#45190:
test/js/bun/jsonl/jsonl-parse.test.tswith 46 new tests: 315 pass. On Bun 1.4.3, 26 of the new ones fail. On 1.4.2 all pass.parseChunkandparse, compared with Bun 1.4.2: 161,568 inputs differ on 1.4.3, none with this branch.The alternative, and the cost for
JSON.parseThe first version of this branch (97c108d) set the position in
nextAfterValue()instead. It fixes the bug too (the same comparison has no differences), but every value thatJSON.parsesees goes through there.bin/jscofbun-webkit-linux-amd64-lto,JSON.parseof an array of 20,000 integers and an array of 4,000 small objects, 400 times,perf stat -e instructions:u, three runs each:bin/jscnextAfterValue()Tests
The parser is only reachable from Bun, so the tests are in oven-sh/bun#45190, with the pin bump.
This change is for the fork only.