Repository navigation
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. ℹ️ Recent review info
Walkthrough
Changesconsole.table Column Lookup
Suggested reviewers: Priority: Merge Risk:
 |
| 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 names the core change: console.table columns are now found by the address of the property-name atom. The PR objectives describe indexing columns by name with a hash map and scanning up to 8 … |
| Description check | |
The description is complete and relevant. It explains the problem, implementation, trade-offs, scope, and verification results. It does not use the template headings exactly, but it provides the requi… |
Full details: Title check
-
Explanation
The title names the core change: console.table columns are now found by the address of the property-name atom. The PR objectives describe indexing columns by name with a hash map and scanning up to 8 columns inline. The title names the lookup key (atom address) rather than the performance goal, but the title still refers to the main change in column lookup in TablePrinter. It is technically specific and does not describe anything absent from the changeset.
Full details: Description check
-
Explanation
The description is complete and relevant. It explains the problem, implementation, trade-offs, scope, and verification results. It does not use the template headings exactly, but it provides the required information in equivalent sections.
- 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.
Comment @coderabbitai help to get the list of available commands.
|
Status: the head is c5ef958 and I do not plan to move it. No review thread is open. CI on this head (build 133238) passed 180 of 181 jobs. Every test lane is green, and What changed since df6bc17: a cell finds its column by the address of its property-name atom. The index keyed on a hash of the name's text is gone. Names chosen to share one hash kept the n times n cost there and ran up to 2.5 times slower than main. The PR body has the numbers. Reproduced on a release build of main 179664e: Overlap: #46825 and #44268 touch the same files. The PR body says what each merge order needs. |
![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 didn't find any bugs. It's a carefully-built performance fix, but it introduces ~100 lines of new native code — an encoding-aware hash that must stay consistent with BunString::eql, and a refcount lifecycle moved from a scopeguard into a Drop impl — so a human look at the memory/hashing invariants would still be worthwhile.
What was reviewed:
ColumnNameContext::ctx_hashvsZigString::eql: traced the mixed-encoding path and confirmed a UTF-16 name equal to an 8-bit name always narrows to the same bytes (no unit >0xFF can surviveeql), so equal names hash equal.- Refcount balance:
createrefs once,Columns::dropderefs eachlistname once (same as the old scopeguard); map keys areCopyhandles with noDrop, so no double-deref when the map drops after the loop. put_no_clobbercan't fire its debug-assert: the scan phase already dedupes viaeql, and the map path only callscreateafter a miss.find_or_createis unreachable from the Map andpropertiesbranches, soFIRST_PROPERTY_COLUMN = 1holds; the index/"Key"/"Values" columns correctly stay out of the map.
Extended reasoning...
Overview
The PR fixes quadratic column lookup in console.table/Bun.inspect.table by wrapping the column Vec in a Columns struct that scans linearly up to 8 property columns and switches to a bun_collections::HashMap<BunString, usize, ColumnNameContext> beyond that. Two files: src/jsc/ConsoleObject.rs (new Columns, ColumnNameContext, and Drop; collect_row and the driver rewired to use them) and test/js/bun/console/console-table.test.ts (a new describe covering placement at 300 cols, a linear-time budget test at 64k/10k cols, and per-strategy semantic tests for Latin-1/UTF-16 name identity, non-ASCII, empty name, and collision with the index/"Values" columns).
Security risks
None. This is output formatting for console.table; input is JS objects the caller already controls. No parsing of untrusted data, no filesystem, no network.
Level of scrutiny
Medium-high. Output semantics are unchanged (existing snapshots untouched, new semantic tests pass on both strategies), so the functional risk is low. But the change lives in native code and touches two areas the repo review rules single out: refcount balance (the ref_/deref pair moves from a scopeguard to a Drop impl, and the map holds unref'd Copy aliases of the same handles) and a custom hash that must satisfy eql(a,b) ⟹ hash(a)==hash(b) across JSC's Latin-1 and UTF-16 storages. I traced both and they check out, but these are exactly the invariants a maintainer should confirm.
Other factors
put_no_clobberreturnsOk(())unconditionally inzig_hash_map.rs, so the.expect("unreachable")is genuinely unreachable rather than an OOM path.JSPropertyIterator::next()returns abun_core::Stringdirectly, so passingcol_keystraight intofind_or_create(dropping the oldBunString::initwrapper) preserves the WTF-backed handle thatref_()actually acts on.- The perf test branches on
isDebugand keeps ~10x headroom under the 5 s budget, matching the repo's guidance for timing-based regression tests. The PR description's own note about ±5-15% jitter on narrow (scan-path) tables is a design tradeoff a human can weigh; it doesn't affect the indexed path. - No prior human reviews or outstanding comments; CodeRabbit was rate-limited and left no findings.
aa8cb3c to
4fd1fb0
Compare
|
Updated 2:40 PM PT - Oct 26th, 2026
❌ @robobun, your commit c5ef958 has 1 failures in
The baseline build crashed or failed tests under 🧪 To try this PR locally: bunx bun-pr 38472That installs a local version of the PR into your bun-38472 --bun |
|
Rebased onto main (4fd1fb0). The only conflict was the unused |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
4fd1fb0 to
6c19024
Compare
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it introduces a custom encoding-aware hash context that must stay consistent with BunString::eql, and swaps the scopeguard-based column-name deref for a Drop impl, a human look at those two pieces would still be worthwhile.
What was reviewed
ColumnNameContext::ctx_hashvsZigString::eql: verified that every case whereeqlholds (both 8-bit, both 16-bit, mixed with all units ≤ 0xFF) hashes to the same value; a 16-bit unit > 0xFF cannoteqlany 8-bit name, so thecast_slicefallback is sound.- Ref-count lifecycle:
createrefs before pushing,Columns::Dropderefs everylistentry, and the map's aliasedBunStringhandles areCopywith noDrop, so no double-deref. put_no_clobber's.expect("unreachable"):zig_hash_map::put_no_clobberunconditionally returnsOk(()), so the error arm is genuinely unreachable.FIRST_PROPERTY_COLUMN = 1:find_or_createis only reached on the property-iterator path (not Map, notproperties), where index 0 is the sole pre-populated column.
Extended reasoning...
Overview
This PR fixes quadratic-time column lookup in console.table / Bun.inspect.table by wrapping the Vec<Column> in a Columns struct that switches from a linear scan to a bun_collections::HashMap<BunString, usize, ColumnNameContext> once a row exceeds 8 property columns. Two files: ~120 net lines in src/jsc/ConsoleObject.rs (new Columns / ColumnNameContext / Drop, plus call-site plumbing) and ~140 lines of new tests in test/js/bun/console/console-table.test.ts.
Security risks
None. The input is JS objects the caller already controls; the change only affects how column names are indexed during rendering. No new syscalls, no untrusted parsing, no allocation sizing driven by attacker-controlled headers.
Level of scrutiny
Medium-high. console.table itself is not a hot or security-sensitive path, but the change touches two areas the repo's review guidelines call out explicitly: (1) a hand-written hash that must agree with an existing encoding-aware equality (BunString::eql compares Latin-1 and UTF-16 by content, so the hash must narrow UTF-16 to Latin-1 bytes when possible — I traced ZigString::eql and the mixed-encoding to_slice() path to confirm the hash is consistent), and (2) BunString refcount management, where the scopeguard that deref'd column names is replaced by impl Drop for Columns. Both look correct to me, and the tests cover the Latin-1/UTF-16 crossover, empty names, and collision with the " " / "Values" special columns in both scan and indexed modes. But these are exactly the shapes where a subtle mistake produces a UAF or a silently duplicated column, so a maintainer eye on ctx_hash and the Drop swap is warranted.
Other factors
The tests are thorough: a 300-column exact-output test, a budget test that fails on the unpatched tree (~22 s vs 5 s budget) and passes patched (~0.4 s), plus semantic tests each run once narrow (scan) and once behind 10 filler columns (indexed). Existing snapshots are unchanged. The comment-cop bot flagged verbose comments; those were trimmed in 6c19024 and the threads are resolved. put_no_clobber(...).expect("unreachable") initially looked like it would misreport OOM, but zig_hash_map::put_no_clobber never returns Err, so the label is accurate. The one minor inefficiency I noted — ctx_hash allocating a Vec<u8> per lookup for narrowable UTF-16 names — is measured in the PR's own benchmarks (10k UTF-16 Latin-1 columns: 5.7 ms) and is dwarfed by the win, so not worth blocking on.
|
Rebased onto main (558ed43). Both conflicts came from #40238, which made
No change to the lookup itself. |
6c19024 to
558ed43
Compare
There was a problem hiding this comment.
LGTM — self-contained O(N²)→O(N) fix for wide console.table, with tests that pin both lookup strategies to identical output.
What was reviewed:
ColumnNameContext::ctx_hashis consistent withBunString::eql: an 8-bit name and a UTF-16 name with the same code points hash identically (the UTF-16 side narrows to Latin-1 when every unit ≤ 0xFF, which is exactly when an 8-bit twin can exist); covered by the Latin-1/UTF-16 symbol-key test.put_no_clobbercannot see a duplicate:createis only reached after a miss, and the crossover populates from a list that was itself built viaeql-scanned uniqueness. ItsResultisAllocErroronly, matching the panickingVec::pushjust above.- The map is only ever built from the property-iterator path, so
FIRST_PROPERTY_COLUMN = 1is correct and the Map "Key" /properties-argument columns never enter it. - Ownership: map keys and list entries each hold their own
BunStringclone; both release viaDropsince #40238.
Extended reasoning...
Overview
Two files: src/jsc/ConsoleObject.rs wraps the column list in a Columns struct that switches from a linear scan to a name-keyed bun_collections::HashMap once more than 8 property columns exist, and test/js/bun/console/console-table.test.ts adds a describe block covering exact output at 300 columns, a 5s time budget at 64k/10k columns, and five semantic tests each run in both scan and indexed mode. No user-visible behavior changes; only the algorithmic complexity of column resolution.
Security risks
None. This is a diagnostic/output path (console.table / Bun.inspect.table) with no parsing of untrusted structured data, no I/O, and no privilege boundaries. The only new allocation is a Vec<u8> per UTF-16 name during hashing and the map itself, both bounded by the number of columns the caller already asked to render.
Level of scrutiny
Moderate. The core risk in a change like this is a hash/equality mismatch producing duplicate columns or wrong cell placement. I traced ctx_hash against BunString::eql (which delegates to ZigString::eql, encoding-aware): the narrowing branch guarantees that any UTF-16 name that could compare equal to an 8-bit name (all units ≤ 0xFF) hashes as its Latin-1 bytes; names with a unit > 0xFF cannot equal any 8-bit name so hashing raw UTF-16 bytes is safe. All names entering the map come from JSPropertyIterator, so they are WTF-backed (Latin-1 or UTF-16, never UTF-8-tagged ZigStrings), which is the only input space the hash needs to handle. put_no_clobber returns Result<(), AllocError> — the .expect fires only on OOM, same as the adjacent Vec::push, and the debug assertion inside it would catch a genuine clobber in ASAN CI.
Other factors
Existing snapshot tests are unchanged and still pass. The new tests exercise both the scan path (narrow) and the indexed path (behind a 10-column filler row) for each semantic case, so a divergence between the two strategies would show up. The budget test branches on isDebug and has >10x headroom per the PR's measurements. The comment-cop bot flags have all been resolved (comments trimmed to one line). Memory management was simplified after the #40238 rebase to plain clone()/Drop, which I verified matches the current BunString ownership model.
558ed43 to
d0ec14d
Compare
|
Rebased onto main (d0ec14d). The only conflict was the import block, where #40374 changed the |
There was a problem hiding this comment.
I reviewed this PR and the bug-hunting pass found no issues. The change is self-contained to TablePrinter and the tests pin both lookup strategies, but ~100 lines of new native code with a custom HashContext that must mirror BunString::eql's cross-encoding semantics is worth a maintainer glance.
What was reviewed:
ColumnNameContext::ctx_hashvsBunString::eql: traced Latin-1↔UTF-16 cases — narrowing units ≤0xFF to bytes matcheseql'sto_utf8()mixed-encoding path for the WTF-backed property namesJSPropertyIteratoryields.- Ownership:
name.clone()in both list and map takes independent WTF refs;Columnsdrops both via RAII on every path including?returns. put_no_clobber(...).expect("unreachable"):zig_hash_map::put_no_clobbernever returnsErr, and duplicates are ruled out (scan/lookup precedes everycreate; crossover populates from a list already free of duplicates).FIRST_PROPERTY_COLUMN = 1: confirmedfind_or_createis only reached when neither the Map "Key" column norpropertiescolumns were pushed.
Extended reasoning...
Overview
The PR wraps TablePrinter's Vec<Column> in a Columns struct that keeps a name→index bun_collections::HashMap once more than 8 property columns exist, turning the per-cell column lookup from O(N) scan to O(1). A new ColumnNameContext supplies the hash/eql for BunString keys. ~130 lines added in src/jsc/ConsoleObject.rs and ~140 lines of new tests in test/js/bun/console/console-table.test.ts.
Security risks
None. This is output formatting for console.table / Bun.inspect.table; no untrusted-input parsing, no I/O, no auth. The only user-controlled input is property names, which are hashed and compared — worst case is hash collisions degrading to eql calls, not a correctness or safety issue.
Level of scrutiny
Medium. The change is contained to one file and one code path (collect_row's discovered-columns branch), and existing snapshots plus the new scan-vs-index tests pin output identically. The subtle part is ColumnNameContext::ctx_hash: it must give equal hashes wherever BunString::eql returns true. I traced this against EncodedSlice::eql (src/bun_core/string/mod.rs:1360): 8-bit names hash their Latin-1 bytes; UTF-16 names with all units ≤0xFF narrow to those same bytes; UTF-16 names with a unit >0xFF hash raw UTF-16 bytes and can never eql an 8-bit name. Since property names from JSPropertyIterator are always WTF-backed (never UTF-8-tagged EncodedSlice), the hash is consistent with eql over the actual input domain. That said, someone familiar with bun_core::String encoding invariants should confirm the reasoning.
Other factors
- Memory:
StringisClone-refs /Drop-derefs since #40238; both the list entry and the map key take their own clone, so refcounts balance on every exit includingJsResultearly returns. No manual ref/deref remains. put_no_clobber(...).expect("unreachable"):zig_hash_map::put_no_clobberunconditionally returnsOk(())(src/collections/zig_hash_map.rs:470), so this is not a user-reachable panic; the duplicate-key debug_assert holds becausecreateis only reached after a miss, and the crossover loop populates from a list that was itself built via find-then-create.FIRST_PROPERTY_COLUMN = 1:find_or_createis only called from the!is_map && properties.is_undefined()branch ofcollect_row, where only the index column was pre-pushed — the Map "Key" andproperties-argument columns never coexist with this lookup.- Tests: the new suite exercises both strategies via a 10-column filler row, covers Latin-1/UTF-16 name equivalence, empty names, and collision with the reserved
" "/Valuescolumns. The 5 s budget test has >10× headroom per the PR's measurements and branches onisDebug. The comment-cop feedback on long comments was addressed and all threads are resolved.
Deferring because a custom hash context paired with an existing equality contract is the kind of thing a maintainer should sign off on, even though I found nothing wrong with it.
d0ec14d to
7dc0656
Compare
robobun
changed the title
|
Rebased onto main (7dc0656) and reworked after #41910.
The PR body has the full table. The numbers are wall clock from interleaved runs, because |
7dc0656 to
5baab32
Compare
5baab32 to
617b86d
Compare
There was a problem hiding this comment.
I reviewed the new commit and found no bugs; it also addresses both inline notes from the previous run (ASAN now takes the 10,000-column size, and the border is built with Buffer.alloc). Because the hash's correctness rests on a JSC invariant the compiler cannot check (equal property names always share one atom, hence one storage encoding), a human look at that design choice would still be worthwhile.
What was reviewed:
Columns::find_or_create/index/createin src/jsc/ConsoleObject.rs: the narrowed scan[1..row_start]and the whole-map lookup are both behavior-preserving because aPropertyNameArraynever yields a name twice per row; Map rows return before any lookup, and theproperties-arg path never uses the index, soFIRST_PROPERTY_COLUMN = 1holds on every path that reaches it.- The two
.expect("unreachable")calls:bun_collections::HashMap::put_no_clobberalways returnsOk(itsResultexists for call-site symmetry), so neither is an OOM or user-reachable panic. - Hashing
byte_slice()vs encoding-awareeql: names come fromBun::toString(prop.impl())onIdentifiers (atoms), andColumn.nameis a refcounted clone that pins the atom, so two encodings of one text cannot coexist while a column holds it; the empty name hashes/compares consistently as well. - Test file: the timing budget now branches on
isASAN, and the 300-column exact-output test plus the scan-vs-indexedexpectColumnsmatrix (non-ASCII, empty name," ",Values) exercise both lookup paths.
Extended reasoning...
The change rewrites column lookup in the console.table TablePrinter (src/jsc/ConsoleObject.rs, about 145 lines of Rust) to scan at most 8 prior-row columns and otherwise consult a lazily built bun_collections HashMap keyed on raw name bytes, plus 155 lines of new tests. It touches no security-sensitive surface (no auth, injection, or data exposure), and neither file is covered by CODEOWNERS. No findings were reported and the prior run's two inline nits are addressed in commit 617b86d, but the hash context deliberately hashes storage bytes while comparing with encoding-aware eql, relying on a JSC atom-table invariant rather than anything enforced in code, which is a design judgment a maintainer should confirm rather than an automated approval.
TablePrinter found the column of each cell by scanning every column made so far, so a row with N properties cost O(N^2) comparisons: one row with 40k keys took about 4 seconds to print. A cell now searches only the columns that earlier rows made (the names of one object are distinct), by a scan while they are few and through a name-to-index map once they are not. The first row searches nothing.
120fcd9 to
954fd20
Compare
|
Pushed 954fd20. Two changes since 617b86d.
Output is unchanged: 20,000 random tables render byte-identical on release builds of main and of this head. |
There was a problem hiding this comment.
I reviewed the latest push (954fd20) and didn't find any bugs; the two points raised on the previous version (fixed wall-clock budget, .repeat in the wide-table helper) are addressed by the ratio test and Buffer.alloc. Because this replaces the lookup on a native hot path and its correctness rests on a JSC invariant rather than on the code alone, a human look is still worthwhile.
What was reviewed:
- The hash/eql agreement for
ColumnNameContext: every name reachingfind_or_createcomes fromBun::toString(prop.impl())on aPropertyNameArrayIdentifier(an atom of the VM's table, which is keyed by content), and the Map andpropertiespaths never call it, so the byte hash cannot split one name across two columns. - The scan-only-earlier-rows shortcut:
PropertyNameArray::adddedups within one iterator (own and prototype-chain walks share the array), and ProxyownKeysduplicates throw before the printer runs. - The two
put_no_clobber(...).expect("unreachable")calls:bun_collections::HashMap::put_no_clobberalways returnsOk(itsVecstorage aborts on OOM), so the panic arm is dead rather than a user-reachable path. - The ratio test has no absolute time cap, branches sizes on
isDebug || isASAN, and takes the min of repeated runs, so a slow lane cannot fail it on a correct build.
Extended reasoning...
The change touches only src/jsc/ConsoleObject.rs (the console.table TablePrinter column lookup, ~150 lines: a lazily built name-to-index HashMap once a table exceeds 8 columns, plus an initial_columns() split) and test/js/bun/console/console-table.test.ts (a new describe block with exact-output, ratio-timing, and cross-encoding name tests). It touches no security-sensitive surface: no user input parsing beyond property names already handled by the existing printer, no auth, crypto, or I/O. Deferring rather than approving because the correctness of the byte-level hash depends on an engine-level invariant (one StringImpl per atomized property name in a VM) that a maintainer familiar with JSC's atom table should confirm, and because the file is native hot-path code with no CODEOWNERS entry. The bug hunt ran dry, and the two prior inline findings are addressed in the current commit.
There was a problem hiding this comment.
I reviewed the latest push and found no bugs; the earlier timing-cap and .repeat concerns are addressed by the ratio-based test and Buffer.alloc. Since this swaps the column lookup data structure in a native hot path on a hashing premise, a human look is still worthwhile before merge.
What was reviewed:
- The "atoms share one byte run" premise behind hashing
byte_slice(): names come fromBun::toString(prop.impl())onIdentifiers out of aPropertyNameArray, which are always atomic, and each column clones (refs) its name so the atom stays in the table — equal text resolves to oneStringImplacross rows. put_no_clobber(..).expect("unreachable"): thisHashMapalways returnsOk(())(growth goes throughVec), so no user-reachable panic; a duplicate would only trip a debug assert.- Scan/index switching:
row_startis monotonic across rows, so onceby_nameexists every later lookup goes through it andcreatekeeps it in sync; the Map,properties, and primitive-row paths never callfind_or_createand are byte-for-byte the old logic. - Tests exercise both the scanned and indexed paths (10 filler columns), exact 300-column output, empty/" "/"Values"/non-ASCII names.
Extended reasoning...
The diff replaces the per-cell linear column scan in TablePrinter (src/jsc/ConsoleObject.rs) with a Columns struct that scans up to 8 earlier-row columns and otherwise lazily builds a wyhash-keyed HashMap<BunString, usize>, plus a new describe block in test/js/bun/console/console-table.test.ts. It touches no security-sensitive surface (no unsafe, no FFI changes, no parsing of untrusted bytes beyond property names already handled). The code is correct as far as I could verify, prior review feedback was addressed in commit 954fd20, and no CODEOWNER covers the files; I deferred rather than approved because the change is a non-trivial data-structure swap whose correctness rests on JSC atom identity, and the ratio-based timing test still carries some CI-noise risk that a maintainer may want to weigh.
TablePrinter::collect_row found the column of a cell by comparing the text of its property name with every column made so far, so a row of n keys cost n times n. One row of 40,000 keys took 2.4 to 2.6 s of CPU. A property name from JSPropertyIterator is a JSC atom: one StringImpl for each distinct text on a thread. The lookup now compares and hashes that address, and reads no text. A Column holds a ref on its name, so the address stays unique while the table is built. - A cell checks only the columns of earlier rows, because one object has distinct names. A first row does no lookups. - A row that starts behind 8 columns or fewer compares addresses inline. A wider table uses a HashMap<usize, usize> that the first lookup builds. - String::atom_address returns the address. - JSPropertyIterator::next asserts in debug builds that a name is empty or an atom. An earlier head of this branch indexed the columns by a wyhash of the name's text. Names chosen to share one hash kept the n times n cost there and ran slower than on main. The ratio tests cover that case.
robobun
changed the title
There was a problem hiding this comment.
I reviewed the current head (atom-address index) and didn't find any bugs; the earlier wall-clock and .repeat concerns on the timing test are addressed by the ratio-with-retry design and Buffer.alloc. Because this changes the identity rule for console.table columns from text equality to JSC atom identity and adds a timing-based assertion, a human look at the invariant and the test's CI stability is still worthwhile.
What was reviewed:
- Column identity: every name reaches
collect_rowviaBun::toString(Identifier::impl()), so non-empty names are atoms and empty names areTag::Empty(key 0) on both the scan and index paths; eachColumnholds aclone()ref, so an indexed address cannot be reused by a different atom during the call. - Index lifecycle: it is built only on the first row with more than 9 earlier columns, every later column insert goes through
indexed_column, and it is unreachable on thepropertiesand Map paths where columns have static or user-supplied non-atom names. .expect("unreachable")onget_or_put/put_no_clobbermatches the file's existingCounter/visited-map usage; CODEOWNERS covers none of the changed files.
Extended reasoning...
The change touches src/jsc/ConsoleObject.rs (TablePrinter column lookup, ~50 lines), adds a public String::atom_address accessor in src/bun_core/string/mod.rs, adds a debug assertion in src/jsc/JSPropertyIterator.rs, and adds a new describe block of correctness and CPU-time linearity tests in test/js/bun/console/console-table.test.ts. It touches no security-sensitive surface; the only unsafe-adjacent piece is reading the WTFStringImpl pointer address, which is kept alive by the column's own ref. Release correctness depends on a JSC invariant (property names from PropertyNameArray are atoms) that is only debug-asserted, and the new test asserts a wide/narrow timing ratio under 4 with a 1.5 s retry window, both of which merit a maintainer's judgment rather than an automated approve. The hunt exited on dry_streak with no findings, and no outstanding third-party objections appear in the timeline.
Problem
console.tableandBun.inspect.tablecost cells times columns:console.table([new Uint8Array(65536)])takes 7.0 to 8.4 s of CPU (Node: 0.2 s).TablePrinter::collect_row(src/jsc/ConsoleObject.rs) compares each cell's property name with every column so far:columns[1..].iter().position(|col| col.name.eql(..)).Fix
HashMap<usize, usize>.StringImplfor each distinct property-name text, and each column holds a ref on its name.test/js/bun/console/console-table.test.ts(three ratio tests fail on main: 56 to 104, limit 4),test/js/bun/console/,test/js/web/console/. 20,000 random tables are byte-identical to main.ArrayHashMapvariant.Background
JSPropertyIteratoryields only atoms and the empty string (JSPropertyIterator: do not yield Symbol keys as strings (macros, http2 sensitiveHeaders, spawn env) #41910), and now asserts it in debug builds.Downsides
.textgrows by 2,560 bytes.bun_core::Stringgains a public accessor.Notes
All numbers are from linux x64. "main" is 179664e. "this PR" is c5ef958. Release builds are of one tree and differ only in this PR's
src/changes. "previous head" is df6bc17, the design this PR had before (an index keyed on a wyhash of the name's text).Reach. No issue reports a slow
console.tableorBun.inspect.table. The scan dates from #7866. The cost shows from about 200 columns in a grid, or a few thousand keys in one row. CPU time of one call:[new Uint8Array(65536)]The last line is not a lookup cost. The output is rows times columns (503 MB there). This PR does not make
console.tablecheap for input of any shape.Lookup instructions for one
Bun.inspect.tablecall. Guest instructions insidecollect_row,String::eql,indexed_columnand theHashMap<usize, usize>functions, counted withqemu-x86_64 -one-insn-per-tb -d exec,nochain -dfilter, JIT off, for R rows x C string-keyed columns.String::eqlcalls in the lookup: 0 on this PR. On main: 28 for 1x8, 3,592 for 100x8, 40,000 for 2x200.All instructions of one
print_tablecall (gdb single-step from entry to return, JIT off), main to this PR:[]propertiesof 1 nameThe last three lines are the shape where the index costs more than main's scan: main finds the first column with one compare. It needs 9 to about 16 columns and more than about 60 narrow rows.
Allocator calls for one table (mimalloc entry points, qemu exec filter). No change when every row starts behind 8 columns or fewer (1x1, 1x8, 100x8, 1x512, an 8-key row then 200 rows). 2 more allocations and frees when a row starts behind more columns (2x9, 2x200, a 9-key row then one cell). 2 more at each doubling of the index (2 rows of 10 keys with no name in common: 4, 3 such rows: 6).
The index. A slot is 24 bytes and 1 metadata byte. The capacity is 1.25 to 2.5 times the number of columns: 31 to 62 bytes for a column, and up to 94 while it doubles. The first lookup of a row behind more than 8 columns builds it over every column, however few lookups follow. It is dropped when the rows are read, before the table is printed. On real atom addresses a lookup inspects 1.8 to 2.1 slots at a load of 61% (20,000 and 40,000 names of 1, 16, 64 and 1,000 characters, and array rows).
Why not another key or container.
bun_wyhash::hash. Names chosen to share one hash kept the n times n cost and ran slower than on main: for 20,000 such names, a 9-name row then a row of new names took 1,762 to 1,908 ms (main: 750 to 773 ms, this PR: 13 to 15 ms), and two rows with the same names took 1,723 to 1,888 ms (main: 1,520 to 1,579 ms, this PR: 14 to 17 ms). A seed does not help, and a keyed hash costs 143 to 192 more instructions for each indexed cell.bun_collections::ArrayHashMap<usize, ()>. It also scans up to 8 entries and then builds an index. But it hashes and stores every cell of a first row, and it allocates its key and hash vectors for every table with an object row. Here a first row does no lookup (one row of 40,000 keys builds no index), and a table of 8 columns or fewer allocates nothing more than on main. I did not build it.PropertyNameArrayBuilder::addscans by pointer below 20 names and fills a pointer set above.HTTPHeaderMapand was closed. This index is not a field of any object. It is a local of oneprint_tablecall, only grows, and cannot go stale.Similar scans that this PR does not change.
TablePrinteris the only column lookup ofconsole.tableandBun.inspect.table.new console.Console(stream).tableis a JS implementation and is linear on main (20,000 keys: 95 ms).util.parseArgsfinds an option by a scan of the option list (src/runtime/node/util/parse_args.rs:177). The key is text from argv, not an atom. 20,000 options and 20,000 arguments take 686 ms (Node: 54 ms).Headersfinds a name that is not a common header by a scan with a case-insensitive compare (HTTPHeaderMap.cpp). Headers: hash-index uncommon names past 64 entries so per-name lookup stays O(1) #35087 was closed.Bun.CookieMapcompares text. Give Bun.CookieMap a name->index hash so per-key ops are O(1) #35909 was closed.console.countandconsole.timekey their maps by a hash of the label (ConsoleObject.rs:88). That is a different question (two labels can share an entry) and this PR does not touch it.Overlap with open PRs.
bun_core::Stringaroundborrow_utf8.atom_addressis placed afterleak_wtf_implfor that reason.git merge-treeof this head against 8716ecb has no conflict.collect_row. It keeps the scan line as it is. Whichever lands second adds thecolumn_indexargument to thecollect_rowcalls. Both PRs append tests at the end ofconsole-table.test.ts.Tests (
console.table column lookup). A ratio test renders N narrow tables and one table N times as wide, and requires the wide one to take less than 4 times as long (CPU time, the fastest of several runs).&columns[1..]), releaseEach variant fails at least one test on a release build. On a debug build the last two variants pass all three, because the tables there are 8 to 16 times narrower to keep the test short. The block passed 100 of 100 runs on a release build, 20 of 20 on a debug build, and 80 of 80 on a release build with 24 and with 48 busy processes on 12 cores.
The other tests check the output: a 2-row, 300-column table spelled out, rows that share some columns and add their own across the scan limit, narrow rows after a wide row, Latin-1 and UTF-16 names, the empty name, names
" "andValues, index names of arrays, objects and typed arrays, one name spelled by different strings (a flat 16-bit string, a concatenation,JSON.parseof separate texts, a Proxy whoseownKeysmakes new strings), and names whose earlier rows were collected.Output. 20,000 pseudo-random tables render byte-identical on release builds of main and of this PR (objects, arrays, typed arrays, primitives, proxies, class instances, getters, as an array, an object, a Map, a Set or an iterator, with and without
properties). 3,000 more ran on the debug build, where the new assertion is on.Self-review. Two things were kept against it. The ratio case for names that share one hash stays: it is the only test that fails on an index keyed on a hash of the text (78 on the previous head). An
ArrayHashMapvariant was not built: the reason above is from its layout, not from a measurement. The deferred index build it offered as optional is not taken, and its cost is in Downsides.Not in scope. Names that collide in JavaScriptCore's own string hash. The engine's property read for each cell is slow for them with or without this PR.
no test proof · iteration 7 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/bun/console/console-table.test.ts