Repository navigation
Conversation
alexkuz
marked this pull request as draft
July 20, 2022 07:43
Jarred-Sumner
added a commit
that referenced
this pull request
Oct 13, 2026
… 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.
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
1 participant
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.
Fixes #645