jsc: enable useEagerCodeBlockJettisonTiming by cirospaciari · Pull Request #29608 · oven-sh/bun · GitHub
Skip to content

jsc: enable useEagerCodeBlockJettisonTiming#29608

Open
cirospaciari wants to merge 2 commits intomainfrom
claude/jsc-eager-codeblock-jettison
Open

jsc: enable useEagerCodeBlockJettisonTiming#29608
cirospaciari wants to merge 2 commits intomainfrom
claude/jsc-eager-codeblock-jettison

Conversation

@cirospaciari
Copy link
Copy Markdown
Member

Summary

  • Set JSC::Options::useEagerCodeBlockJettisonTiming() = true in JSCInitialize
  • This shrinks the time slices for jettisoning a CodeBlock due to old age, allowing JSC to reclaim memory from cold compiled code more aggressively

Test plan

  • bun bd builds
  • BUN_JSC_dumpOptions=2 ./build/debug/bun-debug -e '0' 2>&1 | grep useEagerCodeBlockJettisonTimingtrue
  • CI green

Shrinks the time slices for jettisoning a CodeBlock due to old age,
allowing JSC to reclaim memory from cold compiled code more aggressively.
@robobun
Copy link
Copy Markdown
Collaborator

robobun commented Apr 22, 2026

Copy link
Copy Markdown
Contributor

@claude claude Bot left a comment

Choose a reason for hiding this comment

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

Code change is mechanically correct, but enabling eager CodeBlock jettisoning is a global JIT/memory-tuning tradeoff (lower RSS vs. more recompilation of cold code) — worth a maintainer sign-off, ideally with a memory/perf datapoint.

Extended reasoning...

Overview

This PR adds a single line to JSCInitialize in src/bun.js/bindings/ZigGlobalObject.cpp, setting JSC::Options::useEagerCodeBlockJettisonTiming() = true. It follows the exact same pattern as the surrounding option assignments and is placed in the correct block (before JSC::Options::AllowUnfinalizedAccessScope ends and before env-var overrides are applied).

Security risks

None. This toggles a documented JSC GC/JIT timing heuristic; it does not touch auth, crypto, permissions, input parsing, or any attack surface.

Level of scrutiny

The code is trivially correct and needs no scrutiny. The decision it encodes, however, is a global runtime behavior change: shrinking the old-age jettison windows means JSC will discard compiled CodeBlocks sooner, which trades lower steady-state memory for potentially more re-parsing/re-JITing of code that goes briefly cold (e.g., periodic handlers, warm-up paths in long-running servers). That tradeoff is reasonable and may well be the right default for Bun, but it's a product-level tuning call rather than a mechanical fix.

Other factors

  • No benchmarks or RSS numbers are included in the PR; the test plan only verifies the flag is set.
  • There are no prior reviewer comments to address and no bugs were found.
  • Because this affects every Bun process's JIT lifecycle, I'd rather a human maintainer confirm the tradeoff than auto-approve a one-liner whose effect is non-local.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 22, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: c13ad43d-450a-4f28-a5a7-93c74bc047f2

📥 Commits

Reviewing files that changed from the base of the PR and between 62e453e and e731b10.

📒 Files selected for processing (1)
  • src/bun.js/bindings/ZigGlobalObject.cpp

Walkthrough

Turns on unlinked code block jettisoning and enables useEagerCodeBlockJettisonTiming in the JavaScriptCore initialization options within Bun's JSC bindings (JSCInitialize callback), adding one new option assignment and changing one previously commented-out setting.

Changes

Cohort / File(s) Summary
JavaScriptCore Initialization
src/bun.js/bindings/ZigGlobalObject.cpp
Uncommented/activated unlinked code block jettisoning (true) and added assignment to enable useEagerCodeBlockJettisonTiming() in the JSCInitialize options callback.
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: enabling useEagerCodeBlockJettisonTiming in JSC, which aligns with the primary modification in the changeset.
Description check ✅ Passed The description includes the summary section explaining what the PR does and a test plan section with specific verification steps, matching the template structure with adequate detail.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


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

@github-actions
Copy link
Copy Markdown
Contributor

Comment thread src/bun.js/bindings/ZigGlobalObject.cpp Outdated
Copy link
Copy Markdown
Contributor

@claude claude Bot left a comment

Choose a reason for hiding this comment

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

No issues with the code itself, but flipping useUnlinkedCodeBlockJettisoning and useEagerCodeBlockJettisonTiming on globally is a runtime-wide memory/re-JIT tradeoff — worth a human sign-off (and CI/benchmark confirmation) rather than a bot approval.

Extended reasoning...

Overview

This PR adds two lines to the JSC options block in JSCInitialize (src/bun.js/bindings/ZigGlobalObject.cpp:298-299): it enables useUnlinkedCodeBlockJettisoning (previously a commented-out = false) and useEagerCodeBlockJettisonTiming. Both make JSC reclaim memory from cold compiled code more aggressively. The author's own inline suggestion to add the useUnlinkedCodeBlockJettisoning line has already been applied and resolved.

Security risks

None. These are JSC GC/JIT tuning booleans with no auth, crypto, input-handling, or attack-surface implications.

Level of scrutiny

The diff is mechanically trivial and follows the exact pattern of the surrounding ~20 option assignments — there's no question the code is correct. However, the effect is a global behavioral change to every Bun process: more aggressive jettisoning trades lower JIT memory residency for potentially more re-compilation of code that goes cold and then warm again. That's a performance tuning judgment call, not a mechanical fix, and it's the kind of change where CI/benchmark results (still pending per the test-plan checklist) and a human maintainer's sign-off are the real review — not line-by-line code inspection.

Other factors

No bugs were found by the bug hunter. The change matches the PR description, builds cleanly per the author, and dumpOptions confirms the flag is set. I'm deferring rather than approving only because runtime-wide engine tuning is outside the "obvious/mechanical" bucket I'm comfortable rubber-stamping.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants