Repository navigation
Bump WebKit: a transition from a dictionary structure takes its property table instead of copying it #45774
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
2
commits into
main
Choose a base branch
from
robobun/a5aa7b8b/dictionary-delete-in-place
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.
Open
Bump WebKit: a transition from a dictionary structure takes its property table instead of copying it #45774
Changes from 1 commit
Commits
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,80 @@ | ||
| import { expect, test } from "bun:test"; | ||
| import { bunEnv, bunExe } from "harness"; | ||
|
|
||
| // An object with more than 128 named properties gets a dictionary structure in JavaScriptCore, and a | ||
| // delete, an attribute change or a prototype change moves the object to a new structure. That transition | ||
| // used to copy the dictionary's property table: O(n) time and a table of n entries allocated per delete, | ||
| // which made the delete and re-add loop of issue #44583 about 100x slower than node. The transition now | ||
| // takes the table. `heapStats().objectTypeCounts` counts the cells in the heap, the unswept ones included, | ||
| // so the count of PropertyTable cells says how many tables a loop allocated. A loop that only adds or only | ||
| // changes attributes allocates 8 to 12 tables per 100 iterations on any build (the table grows, and the | ||
| // inline caches flatten the dictionary once). Before the fix each loop below allocated one more per iteration. | ||
| const tableBound = 30; | ||
| async function propertyTablesAllocatedBy( | ||
| loopBody: string, | ||
| { object = 'const object = {}; for (let i = 0; i < 1000; i++) object["key" + i] = i;', env = bunEnv } = {}, | ||
| ): Promise<{ propertyTables: number; keys: number; value: unknown }> { | ||
| await using proc = Bun.spawn({ | ||
| cmd: [ | ||
| bunExe(), | ||
| "-e", | ||
| ` | ||
| const { heapStats, fullGC } = require("bun:jsc"); | ||
| ${object} | ||
| const body = (object, i) => { ${loopBody} }; | ||
| for (let i = 0; i < 8; i++) body(object, i); | ||
| fullGC(); | ||
| const before = heapStats().objectTypeCounts.PropertyTable ?? 0; | ||
| for (let i = 0; i < 100; i++) body(object, i); | ||
| const after = heapStats().objectTypeCounts.PropertyTable ?? 0; | ||
| console.log(JSON.stringify({ propertyTables: after - before, keys: Object.keys(object).length, value: object.key0 })); | ||
| `, | ||
| ], | ||
| env, | ||
| stdout: "pipe", | ||
| stderr: "pipe", | ||
| }); | ||
| const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | ||
| expect(stderr).toBe(""); | ||
| expect(exitCode).toBe(0); | ||
| return JSON.parse(stdout); | ||
| } | ||
|
|
||
|
claude[bot] marked this conversation as resolved.
Resolved
|
||
| test("deleting and re-adding a property on a large object does not copy its property table", async () => { | ||
|
coderabbitai[bot] marked this conversation as resolved.
Resolved
|
||
| const result = await propertyTablesAllocatedBy("delete object.key0; object.key0 = i;"); | ||
| // Before the fix: 109 for 100 iterations. | ||
| expect(result.propertyTables).toBeLessThan(tableBound); | ||
| expect(result).toMatchObject({ keys: 1000, value: 99 }); | ||
| }); | ||
|
|
||
| test("changing the attributes of a property on a large object does not copy its property table", async () => { | ||
| // An add pins the table in place. The attribute change after it used to copy the table. | ||
| const result = await propertyTablesAllocatedBy( | ||
| 'object["added" + i] = i; Object.defineProperty(object, "key0", { value: i, writable: i % 2 === 0, enumerable: true, configurable: true });', | ||
| ); | ||
| expect(result.propertyTables).toBeLessThan(tableBound); | ||
| expect(result).toMatchObject({ keys: 1100, value: 99 }); | ||
| }); | ||
|
coderabbitai[bot] marked this conversation as resolved.
Resolved
|
||
|
|
||
| test("changing the prototype of a large object does not copy its property table", async () => { | ||
| // A prototype change pins the table in the new structure. It took a copy of the dictionary's table. | ||
| const result = await propertyTablesAllocatedBy("Object.setPrototypeOf(object, prototypes[i & 1]);", { | ||
| object: | ||
| 'const object = {}; for (let i = 0; i < 1000; i++) object["key" + i] = i; const prototypes = [{ a: 1 }, { b: 2 }];', | ||
| }); | ||
| expect(result.propertyTables).toBeLessThan(tableBound); | ||
| expect(result).toMatchObject({ keys: 1000, value: 0 }); | ||
| }); | ||
|
coderabbitai[bot] marked this conversation as resolved.
Resolved
|
||
|
|
||
| test("deleting and re-adding a variable in a large process.env does not copy its property table", async () => { | ||
| // process.env is the same kind of object once the environment has more than 128 variables. | ||
| const env = { ...bunEnv }; | ||
| for (let i = 0; i < 200; i++) env[`ROBOBUN_VAR_${i}`] = String(i); | ||
| const result = await propertyTablesAllocatedBy("delete object.key0; object.key0 = String(i);", { | ||
| object: "const object = process.env;", | ||
| env, | ||
| }); | ||
| expect(result.propertyTables).toBeLessThan(tableBound); | ||
| expect(result).toMatchObject({ value: "99" }); | ||
| expect(result.keys).toBeGreaterThan(200); | ||
|
claude[bot] marked this conversation as resolved.
Resolved
|
||
| }); | ||
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.
🔴 Every developer and CI build of Bun will fail to download JavaScriptCore the moment oven-sh/WebKit#803 merges or closes, which the base pin never does. scripts/build/deps/webkit.ts:6 pins WEBKIT_VERSION to the preview tag "autobuild-preview-pr-803-cf96a639"; scripts/build/download.ts:319-321 documents that GitHub deletes an autobuild-preview-pr-* release when its PR merges or closes, so every prebuilt fetch 404s at once. Fix: before merging, land oven-sh/WebKit#803 and set WEBKIT_VERSION to the merged main commit sha (or its autobuild- tag), confirming the release carries every platform x flavor tarball prebuiltSuffix() can name. The description says to do this later, but a merged Bun pin to a deleted release breaks main for everyone until someone re-bumps.
Why this was flagged
The trigger is the oven-sh/WebKit PR #803 merging or closing, which happens outside this repo and is the stated plan. After that, cfg.webkitVersion defaults to "autobuild-preview-pr-803-cf96a639" (scripts/build/config.ts:59, :1221) and prebuiltUrl() at scripts/build/deps/webkit.ts:81-82 builds https://github.com/oven-sh/WebKit/releases/download/autobuild-preview-pr-803-cf96a639/bun-webkit--.tar.gz. GitHub removes the preview release with the PR, so the download returns 404 and prebuiltDownloadError at scripts/build/download.ts:328-337 throws "WebKit preview release is gone" for every
bun run buildand every CI build-bun step on every platform. On the base branch WEBKIT_VERSION is the 40-hex merged sha 87ee10da087b63e011bd1ada64d1814a850a4395 whose autobuild release is permanent. The repo's own upgrade procedure (.claude/commands/upgrade-webkit.md:34) says the bump to the merged sha must happen before the Bun PR merges; nothing in this diff enforces that, and the only safeguard is the error message after the breakage.Verification: scripts/build/deps/webkit.ts:6 sets
WEBKIT_VERSION = "autobuild-preview-pr-803-cf96a639"(base was a merged sha whose release never expires). scripts/build/download.ts:319-322 documents the trigger: GitHub deletes the preview release when the PR merges or closes, so every default prebuilt build 404s and hard-fails. No in-repo check prevents merging anautobuild-preview-pr-*pin.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.
Agreed. The preview tag is for the build and the CI of this PR only. WEBKIT_VERSION moves to the merged main sha of oven-sh/WebKit#803 before this PR merges. Left open until that bump lands.