fmt: route pure-{s} allocPrint through shared concat (-287 KB .text)#30358
fmt: route pure-{s} allocPrint through shared concat (-287 KB .text)#30358robobun wants to merge 2 commits into
Conversation
std.fmt.allocPrint/allocPrintSentinel generate a fresh Writer.print
monomorphization per (fmt, args-tuple) pair. ~35% of the 406+22 call
sites in src/ use format strings composed solely of bare {s}
placeholders and literal text — "{s}@{s}", "_{s}", "workspace:{s}",
"{s}.exe", "X-Amz-Date={s}", and so on — which are plain string
concatenation in disguise.
Add bun.fmt.allocPrint / allocPrintSentinel / allocPrintZ. At comptime
they detect the pure-{s} shape and lower it to a flat
[N][]const u8 array fed to a single shared bun.strings.concat (or a
sentinel-terminated equivalent). Everything else forwards unchanged to
std.fmt. Then mechanically swap all src/ call sites to the bun.fmt
entry points and ban direct std.fmt.allocPrint* use going forward.
linux-x64 release, stripped:
before 91,569,096 B
after 91,257,800 B (-304 KB; .text -287 KB)
Io.Writer.print__anon_* instantiations: 5031 -> 4364
std.fmt.allocPrint__anon_* instantiations: 1111 -> 494
WalkthroughAdds bun.fmt allocPrint/allocPrintZ/allocPrintSentinel with a comptime fast path and replaces std.fmt allocPrint usages across the repository. Documentation and tests ban std.fmt allocPrint variants. One behavioral tweak: stricter permessage-deflate negotiation during WebSocket upgrade. ChangesFormatter migration and enforcement
|
|
Updated 1:20 AM PT - May 7th, 2026
❌ @autofix-ci[bot], your commit 2b55f9b has 2 failures in
🧪 To try this PR locally: bunx bun-pr 30358That installs a local version of the PR into your bun-30358 --bun |
|
CI status (build 52451, all lanes complete): CI's own binary-size check confirms the savings on every target — −249 KB to −368 KB vs The two red annotations are pre-existing flakes unrelated to this PR:
Remaining flaky-annotation entries (jsc-stress mprotect on Windows, hot.test.ts reload counter, fetch-http2 AtomStringImpl assertion) passed on retry. All coderabbit inline findings are pre-existing on |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find correctness issues, but this introduces a new comptime format-string parser plus a codebase-wide convention (enforced via ban-words) across 125 files in core paths — worth a human sign-off, and CI build-zig is currently red on most targets.
Extended reasoning...
Overview
This PR adds bun.fmt.allocPrint / allocPrintSentinel / allocPrintZ in src/bun_core/fmt.zig — wrappers that comptime-parse the format string and, when it consists solely of bare {s} placeholders and literals, lower the call to a shared bun.strings.concat to reduce per-call-site monomorphization. All 400+ std.fmt.allocPrint* call sites across 120+ source files (bundler, js_parser, install, resolver, runtime/server, s3 signing, shell, sql, etc.) are mechanically swapped to the new entry points, test/internal/ban-words.test.ts bans the old spellings, and src/CLAUDE.md documents the convention. One file (s3/client.zig) also drops its now-unused std import.
Security risks
None identified. The fast path is a pure string concatenation that is byte-identical to std.fmt's output for the patterns it accepts; everything else (positional args, named args like {[key]s}, width/precision, escapes, non-{s} specifiers, by-value arrays) falls through to std.fmt unchanged. No new untrusted-input parsing surface is introduced.
Level of scrutiny
High. While the per-call-site edits are mechanical s/std.fmt/bun.fmt/, the new parseConcatPlan comptime parser and concatArgSlice coercion are load-bearing for string formatting throughout the codebase, and the change establishes a new enforced convention. The diff also touches hot/critical paths (bundler linker, JS parser, package installer, S3 request signing, HTTP server). A maintainer should validate that the comptime classifier's accepted-type set exactly matches std.fmt's {s} semantics (e.g. sentinel-terminated pointers, optionals) and that the ban-words limits won't cause friction.
Other factors
CI (robobun) reports build-zig failures on essentially every target for commit 0aec1db, which contradicts the PR description's "zig:check-all passes" — that needs to be reconciled before merge regardless of review outcome. There is a follow-up autofix.ci commit but no green build reported yet. The PR includes a Zig test block asserting fast-path/std.fmt parity and fall-through classification, which is good, but the integration test claims in the description can't be verified from here. Given the breadth, the new convention being locked in via ban-words, and the red CI, this should not be auto-approved.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/cli/update_interactive_command.zig (1)
450-460:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid leaking the temporary
catalog_keybuffer.When
pkg.catalog_nameis present, Line 451 allocatescatalog_key, but Line 459 inserts a duplicated copy into the map and the original allocation is never freed. Each selected catalog update leaks one buffer.Proposed fix
- const catalog_key = if (pkg.catalog_name) |catalog_name| - try bun.fmt.allocPrint(bun.default_allocator, "{s}:{s}", .{ pkg.name, catalog_name }) - else - pkg.name; + const catalog_key = if (pkg.catalog_name) |catalog_name| + try bun.fmt.allocPrint(bun.default_allocator, "{s}:{s}", .{ pkg.name, catalog_name }) + else + try bun.default_allocator.dupe(u8, pkg.name); + errdefer bun.default_allocator.free(catalog_key); @@ - try catalog_updates.put(try bun.default_allocator.dupe(u8, catalog_key), .{ + try catalog_updates.put(catalog_key, .{ .version = try bun.default_allocator.dupe(u8, target_version), .workspace_path = catalog_workspace_path, });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cli/update_interactive_command.zig` around lines 450 - 460, The temporary catalog_key buffer allocated when pkg.catalog_name is present is leaked because you duplicate it again into catalog_updates.put; update the code to avoid leaking by either (a) not allocating a separate temporary (compute the final key directly for insertion), or (b) after calling catalog_updates.put, free the original catalog_key allocation; locate the allocation and use of catalog_key in the block around catalog_key, catalog_workspace_path and the catalog_updates.put call and ensure the original allocation is deallocated (or eliminated) so only the stored copy remains.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/bundler_jsc/JSBundleCompletionTask.zig`:
- Around line 375-383: The current code calls bun.fmt.allocPrint("{s}.map", ...)
and immediately passes it into bun.path.basename which returns a slice into that
heap buffer, then loses the original allocation causing a leak; fix by assigning
the allocPrint result to a temporary (e.g., full_map_path_alloc), call const
map_basename = bun.path.basename(full_map_path_alloc), and immediately defer
freeing full_map_path_alloc via bun.default_allocator.free(full_map_path_alloc)
(use bun.handleOom where appropriate), ensuring map_basename remains valid
because it is a subslice of the still-live buffer until you free at the end of
the function or after use; apply the same pattern for the other
bun.fmt.allocPrint call used to build sourcemap_full_path so all allocPrint
allocations are freed instead of leaked.
In `@src/semver/SemverQuery.zig`:
- Line 176: Group.fmt is being called without the required buf: string argument
in the line creating temp; update the call to pass the Group's input field so
fmt receives the buffer. Locate the Group.fmt method and the call site where
temp is assigned (const temp = try bun.fmt.allocPrint(... "{f}", .{this.fmt()}))
and change the invocation to this.fmt(this.input) so the existing input string
is used as the buf parameter.
In `@src/shell/builtin/cp.zig`:
- Around line 478-484: The error message uses this.src twice; change the second
format argument in the allocPrint call inside the branch that checks
bun.strings.eql(src, tgt) so the format parameters are .{ this.src, this.tgt }
(update the allocPrint invocation that currently passes .{ this.src, this.src
}), ensuring the error string reports both the original src and the
user-supplied tgt; keep usage of bun.handleOom and bun.fmt.allocPrint unchanged.
---
Outside diff comments:
In `@src/cli/update_interactive_command.zig`:
- Around line 450-460: The temporary catalog_key buffer allocated when
pkg.catalog_name is present is leaked because you duplicate it again into
catalog_updates.put; update the code to avoid leaking by either (a) not
allocating a separate temporary (compute the final key directly for insertion),
or (b) after calling catalog_updates.put, free the original catalog_key
allocation; locate the allocation and use of catalog_key in the block around
catalog_key, catalog_workspace_path and the catalog_updates.put call and ensure
the original allocation is deallocated (or eliminated) so only the stored copy
remains.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 08b840a6-4d64-4c24-b1b0-239ff37c2058
📒 Files selected for processing (125)
src/CLAUDE.mdsrc/bake/DevServer/DirectoryWatchStore.zigsrc/bake/production.zigsrc/bun.js.zigsrc/bun_core/fmt.zigsrc/bun_core/output.zigsrc/bundler/Chunk.zigsrc/bundler/LinkerContext.zigsrc/bundler/ParseTask.zigsrc/bundler/ServerComponentParseTask.zigsrc/bundler/bundle_v2.zigsrc/bundler/entry_points.zigsrc/bundler/linker.zigsrc/bundler/linker_context/MetafileBuilder.zigsrc/bundler/linker_context/computeChunks.zigsrc/bundler/linker_context/generateChunksInParallel.zigsrc/bundler/linker_context/generateCodeForLazyExport.zigsrc/bundler/linker_context/generateCompileResultForHtmlChunk.zigsrc/bundler/linker_context/scanImportsAndExports.zigsrc/bundler/linker_context/writeOutputFilesToDisk.zigsrc/bundler/options.zigsrc/bundler_jsc/JSBundleCompletionTask.zigsrc/bundler_jsc/PluginRunner.zigsrc/cli/Arguments.zigsrc/cli/audit_command.zigsrc/cli/build_command.zigsrc/cli/bunfig.zigsrc/cli/bunx_command.zigsrc/cli/create_command.zigsrc/cli/multi_run.zigsrc/cli/pack_command.zigsrc/cli/package_manager_command.zigsrc/cli/pm_pkg_command.zigsrc/cli/pm_version_command.zigsrc/cli/publish_command.zigsrc/cli/repl.zigsrc/cli/run_command.zigsrc/cli/test/ChangedFilesFilter.zigsrc/cli/test/parallel/runner.zigsrc/cli/test_command.zigsrc/cli/update_interactive_command.zigsrc/cli/upgrade_command.zigsrc/cli/why_command.zigsrc/crash_handler/crash_handler.zigsrc/css/css_modules.zigsrc/css/error.zigsrc/css/media_query.zigsrc/dns/dns.zigsrc/http/AsyncHTTP.zigsrc/http_jsc/websocket_client.zigsrc/http_jsc/websocket_client/WebSocketUpgradeClient.zigsrc/ini/ini.zigsrc/install/PackageInstaller.zigsrc/install/PackageManager/PackageJSONEditor.zigsrc/install/PackageManager/PackageManagerLifecycle.zigsrc/install/PackageManager/UpdateRequest.zigsrc/install/PackageManager/patchPackage.zigsrc/install/PackageManager/runTasks.zigsrc/install/hosted_git_info.zigsrc/install/lockfile/Package.zigsrc/install/lockfile/bun.lock.zigsrc/install/npm.zigsrc/install/patch_install.zigsrc/install/yarn.zigsrc/install_jsc/install_binding.zigsrc/interchange/toml/lexer.zigsrc/js_parser/ast/E.zigsrc/js_parser/ast/P.zigsrc/js_parser/ast/Parser.zigsrc/js_parser/ast/lowerDecorators.zigsrc/js_parser/ast/maybe.zigsrc/js_parser/ast/parseStmt.zigsrc/js_parser/ast/visitExpr.zigsrc/js_parser/lexer.zigsrc/jsc/AsyncModule.zigsrc/jsc/BuildMessage.zigsrc/jsc/JSGlobalObject.zigsrc/jsc/ResolveMessage.zigsrc/jsc/VirtualMachine.zigsrc/jsc/ZigStackFrame.zigsrc/jsc/web_worker.zigsrc/logger/logger.zigsrc/options_types/schema.zigsrc/patch_jsc/testing.zigsrc/resolver/fs.zigsrc/resolver/resolver.zigsrc/runtime/api/BunObject.zigsrc/runtime/api/JSTranspiler.zigsrc/runtime/api/bun/js_bun_spawn_bindings.zigsrc/runtime/api/cron.zigsrc/runtime/api/html_rewriter.zigsrc/runtime/crypto/PasswordObject.zigsrc/runtime/dns_jsc/dns.zigsrc/runtime/ffi/ffi.zigsrc/runtime/node/node_fs.zigsrc/runtime/server/NodeHTTPResponse.zigsrc/runtime/server/RequestContext.zigsrc/runtime/server/ServerConfig.zigsrc/runtime/server/server.zigsrc/runtime/socket/tls_socket_functions.zigsrc/runtime/webcore/Blob.zigsrc/runtime/webcore/Request.zigsrc/runtime/webcore/fetch.zigsrc/runtime/webcore/fetch/FetchTasklet.zigsrc/runtime/webcore/s3/client.zigsrc/runtime/webview/ChromeProcess.zigsrc/s3_signing/credentials.zigsrc/semver/SemverQuery.zigsrc/shell/Builtin.zigsrc/shell/builtin/cp.zigsrc/shell/builtin/seq.zigsrc/shell/interpreter.zigsrc/shell/shell.zigsrc/shell/states/Expansion.zigsrc/shell/subproc.zigsrc/shell_parser/braces.zigsrc/sql_jsc/mysql/JSMySQLConnection.zigsrc/sql_jsc/postgres/PostgresSQLConnection.zigsrc/sql_jsc/postgres/Signature.zigsrc/standalone_graph/StandaloneModuleGraph.zigsrc/string/string.zigsrc/test_runner/jest.zigsrc/url/url.zigtest/internal/ban-limits.jsontest/internal/ban-words.test.ts
|
Closing: this PR is obsolete after #30412 (Rewrite Bun in Rust), which landed on main on 2026-05-14. The The |
1 participant
What
std.fmt.allocPrint/std.fmt.allocPrintSentinelgenerate a freshWriter.printmonomorphization per(fmt, args-tuple)pair. Of the 406 + 22 call sites insrc/, ~35% use format strings composed solely of bare{s}placeholders and literal text —"{s}@{s}","_{s}","workspace:{s}","{s}.exe","X-Amz-Date={s}","./{s}","{s}/{s}", and so on — which are plain string concatenation in disguise.How
Adds
bun.fmt.allocPrint/bun.fmt.allocPrintSentinel/bun.fmt.allocPrintZ. At comptime they parse the format string; when it's purely{s}+ literals (no width/precision/positional/escapes) they lower the call to a flat[N][]const u8array fed to a single sharedbun.strings.concat(or a sentinel-terminated equivalent). Any other format string forwards unchanged tostd.fmt.allocPrint*.All
src/call sites are mechanically swapped to thebun.fmtentry points, andtest/internal/ban-words.test.tsnow bans directstd.fmt.allocPrint(/std.fmt.allocPrintSentinel(so the convention sticks.Binary size (linux-x64 release, stripped)
bun.textIo.Writer.print__anon_*std.fmt.allocPrint__anon_*Verification
zig:check-allpasses (all targets, debug + release).bun.fmt.allocPrintoutput is byte-identical tostd.fmt.allocPrint(Zig test block infmt.zigasserts parity for the fast-path patterns and verifies non-{s}formats fall through).bun-add(same pass/fail as main — only pre-existing git-network failures),bun-pm(12/12),bun-audit+bun-pm-why(44/44),bun-pack(70/70), es-decorators (147/147), bundler edgecase (103/103), s3-signature-order (1/1), bunshell (354/355 — same pre-existing EACCES-as-root failure as main).bun install/bun outdated/bun builderror output manually.