Skip to content

test(console.table): cut the cell GC test from 64 full GCs to 4 - #47004

Open
robobun wants to merge 1 commit into
mainfrom
robobun/f666cf36/console-table-gc-test-debug-timeout
Open

robobun wants to merge 1 commit into
mainfrom
robobun/f666cf36/console-table-gc-test-debug-timeout

Conversation

@robobun

@robobun robobun commented Oct 26, 2026

Copy link
Copy Markdown
Collaborator

Problem

  • On a debug build, cell values survive a full GC between the width and render passes takes 2.3 to 3.6 s on an idle machine, and fails with this test timed out after 5000ms under load. Six open PRs report that timeout.
  • The test runs Bun.gc(true) in each of its 64 getters. One full GC takes about 50 ms on a debug build.
  • CI does not show it: CI has no debug lane, and its runner passes --timeout=90000 (scripts/runner.node.ts:1167).

Fix

  • Run the GC in 4 of the 64 getters (0, 16, 32, 48). The rows and the assertion do not change.
  • The test still finds a freed cell value. With the values kept in a Vec<JSValue> (a local change), 47 of 64 ids are missing on debug and release builds. The old test: 62.
  • Verified: test/js/bun/console/console-table.test.ts on a debug build, 32 pass in 10 of 10 runs, and the test takes 0.18 to 0.30 s. Also a release build.

Background

  • console.table formats each cell one time, in its width pass (console.table: invoke each cell getter once #32924). An earlier revision kept the cell values for the render pass, and the test guards that design against a GC between the passes.
  • The test sees a freed value through its memory: the object of a later getter takes it, and the earlier row shows a later id.
  • Considered N = 8 with a GC in each getter: 8 GCs, and 6 missing ids. Considered isDebug: a release build finds the bug with 4 GCs too.
Notes

All measurements: Linux x64, 12 cores. The debug build is bun bd of main 449877f (debug, ASAN). The release build for the timings is 367d939.

Timings of the test

before after
debug build, whole file, 10 runs each 2.33 to 3.61 s 0.18 to 0.30 s
debug build, 12 copies of the file at once 7.05 to 9.75 s, 12 of 12 time out 0.25 to 0.77 s, 12 of 12 pass
debug build, 36 busy loops on the 12 cores, 6 runs each 3.51 to 4.41 s 0.45 to 0.88 s
release build, 5 runs each 373 to 450 ms 20 to 31 ms

The whole file on the debug build: 4.45 to 5.70 s before, 2.32 to 2.42 s after.

Cost of one full GC. Bun.gc(true) in the getter takes 24 to 155 ms on the debug build, with a median of 41 to 61 ms (two runs of 64 calls). fullGC() from bun:jsc costs the same. Bun.gc(false) only requests a concurrent collection, so it cannot replace the call.

The local change that frees cell values. TablePrinter::collect_row (src/jsc/ConsoleObject.rs) formats a cell as soon as it reads it. For the experiment it stores the value, and print_table formats the stored values after the last row:

// collect_row, in place of the format_cell call
self.pending.push((self.row_idx, slot, value)); // Vec<(usize, usize, JSValue)>

// print_table, after the width pass
for (row_idx, slot, value) in core::mem::take(&mut self.pending) {
    let cell = self.format_cell::<ENABLE_ANSI_COLORS>(&mut cell_text, value)?;
    columns[slot + 1].width = columns[slot + 1].width.max(cell.width);
    rows[row_idx].cells[slot] = Some(cell);
}

The GC does not scan the Vec, so a full GC in a later getter frees the stored objects. REVIEW.md names this bug: "Root or copy every JSValue held beyond the current call". Two builds have the change: the debug build, and a release build of 449877f (--profile=release --lto=off).

What the new test prints with the local change. Both builds print the same table. Rows 0 to 46 show the id of a row from 47 to 63:

0:48 1:49 2:50 ... 14:62 15:47 16:48 ... 31:63 32:48 ... 46:62 47:47 48:48 ... 63:63

The assertion fails with the ids 0 to 46 in missing. In each build the whole file has 1 fail and 31 pass, so no other test in the file finds this bug.

Ids missing from the table with the local change. One scratch test for each row of this table. Each count is the same in every run (3 or more runs for each build).

GC in getter GCs debug build release build
each of 64 (the old test) 64 62 62
0, 16, 32, 48 (the new test) 4 47 47
0, 8, ..., 56 8 55 55
0, 32 2 31 31
8, 24, 40, 56 4 55 39
15, 31, 47, 63 4 62 32
31, 63 2 62 1
32 only 1 31 0
63 only 1 62 0
each of 8 rows (N = 8) 8 6 6
each of 2 rows (N = 2) 2 0 0

Why getter 0 runs a GC. On the release build, the values that the getters return before the first GC do not show the bug. With GCs in getters 8, 24, 40 and 56, ids 0 to 7 are not missing. One GC in getter 32, or in getter 63, finds nothing. With a GC in getter 0, the two builds give the same count: all the rows before the last GC, less one.

CI.

  • .buildkite/ci.ts has release lanes and one asan lane, which is a release build with ASAN (--asan=on). No lane tests a debug build.
  • The runner passes --timeout=90000 to bun test, and --timeout=270000 on the ASAN lane (scripts/runner.node.ts:1167, :267). The 5 s default applies only to a direct bun test, for example bun bd test.
  • The ASAN lane ran this file in 0.63 s in build 131281. test/expected-durations.json has 1160 ms for the asan column and 140 ms for the default lane.
  • No annotation (error, flaky, or new flaky) names this file in 989 builds: 389 builds from 132751 to 133153 (25 and 26 October), and a sample of 600 builds from 127157 to 132250 (17 to 24 October).

History.

`cell values survive a full GC between the width and render passes`
ran `Bun.gc(true)` in each of its 64 getters. One full GC takes about
50 ms on a debug build, so the test took 2.3 to 3.6 s of the 5 s
default timeout, and timed out when the machine had other load.

Run the GC in getters 0, 16, 32 and 48. The rows and the assertion do
not change.
@coderabbitai

coderabbitai Bot commented Oct 26, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 1 minute.

Check out review usage here.

View limit details

Limit details: You’ve used all 10 included reviews currently available.

Learn how review limits work.

Review configuration:

Run configuration
Commits

Reviewing files that changed from the base of the PR and between 449877f and 085b985.

Files selected for processing: 1
  • test/js/bun/console/console-table.test.ts
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@robobun

robobun commented Oct 26, 2026

Copy link
Copy Markdown
Collaborator Author

Status: ready for review. Waiting for CI.

Reproduction.

  • On main 449877f, with a debug build: bun bd test test/js/bun/console/console-table.test.ts, 10 runs. cell values survive a full GC between the width and render passes takes 2.33 to 3.61 s of the 5 s limit.
  • With 12 copies of the file at once on 12 cores, the test fails in 12 of 12 runs: this test timed out after 5000ms, after 7.05 to 9.75 s.

With this branch.

  • The same command gives 32 pass in 10 of 10 runs, and the test takes 0.18 to 0.30 s.
  • With 12 copies of the file at once, the test passes in 12 of 12 runs, in 0.25 to 0.77 s.

PR: #47004

@robobun

robobun commented Oct 26, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 11:56 AM PT - Oct 26th, 2026

✅ @robobun, your commit 085b985445aeb234579c568f90a6f4eaafde2b91 passed in Build #133180! 🎉


🧪   To try this PR locally:

bunx bun-pr 47004

That installs a local version of the PR into your bun-47004 executable, so you can run:

bun-47004 --bun

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review found no issues

No high-confidence issues detected in this change.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant