Repository navigation
Conversation
Options::initialize runs notifyOptionsChanged once after the options customization callback. Verifying every BUN_JSC_ option inside the callback ran that pass once per variable, and each pass multiplied the tier-up thresholds by jitPolicyScale again, so the scale depended on how many BUN_JSC_ variables were set. Set the options without verification, like jsc.cpp does, and drop the coherence assertion that ran before the pass that makes the options coherent.
|
Warning Review limit reached
On-demand reviews are free for the next 25 days. After that, they cost $0.25 per reviewed file. Or wait 11 minutes for your next included review. View limit detailsLimit details: You’ve used all 5 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
Status: one-line runtime change plus two tests in CI (build 106447): |
|
Status: one-line runtime change plus two tests in |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — the follow-up commit reworked the useProfiler test to positively assert a derived option, which addresses my earlier note.
What was reviewed:
JSCInitialize:verify=falsematches howjsc.cppsets--options; the droppedassertOptionsAreCoherent()is redundant with the oneOptions::finalizeruns afternotifyOptionsChanged.- The
jitPolicyScaletest fails on base (125) and passes with the fix (500), satisfying the USE_SYSTEM_BUN check; the second test guards the new invariant that the post-callback pass still runs. - Checked the
$-anchored regexes against Windows CRLF from CRT stderr — ruled out.
Extended reasoning...
Overview
This PR changes two lines in JSCInitialize (src/jsc/bindings/ZigGlobalObject.cpp): the BUN_JSC_* env-var loop now calls JSC::Options::setOption(env + 8, /* verify */ false) instead of the default verify=true, and the trailing JSC::Options::assertOptionsAreCoherent() is removed. Two test.concurrent cases are appended to the existing describe("JSC option environment variables") block in test/cli/run/env.test.ts, reusing the dumpOptions helper already in that block.
Security risks
None. This is startup-time JSC option plumbing driven by process environment variables that were already being read and applied; the change only alters when the coherence/derivation pass runs (once after all options are set, rather than once per option). No new input surface, no auth/crypto/permissions.
Level of scrutiny
Low-to-moderate. The C++ change is two lines and mirrors the pattern WebKit's own jsc.cpp uses for -- command-line options. The rationale is well-documented: Options::initialize calls notifyOptionsChanged once after the customization callback returns, and Options::finalize asserts coherence afterwards, so per-option verification inside the callback is both redundant and actively wrong for jitPolicyScale (which scaleJITPolicy compounds on every pass). The removed assertOptionsAreCoherent() ran before the pass that makes options coherent, so with verify=false it would have crashed valid combinations like useProfiler=1 — removing it is required, not incidental.
Other factors
My earlier inline comment (that the original useProfiler test only asserted "didn't crash" and passed on base) was addressed in a17331c: the test now asserts needDisassemblySupport=true, a value the post-callback verification pass derives. The jitPolicyScale test is the one that fails on the released binary (=125) and passes with the fix (=500), covering the actual bug. Tests follow repo conventions: added to the existing file, test.concurrent, bunEnv spread in the shared helper, output asserted before exit code. The Windows /…$/m CRLF concern was investigated this run and ruled out. No CODEOWNERS entries cover these paths.
Problem
BUN_JSC_jitPolicyScale=0.5scaled the tier-up thresholds once perBUN_JSC_variable in the environment, not once. WithBUN_JSC_useConcurrentJIT=0andBUN_JSC_dumpOptions=2also set,thresholdForOptimizeAfterWarmUpcame out as 125 instead of 500, and adding any otherBUN_JSC_variable changed it again.JSCInitialize(src/jsc/bindings/ZigGlobalObject.cpp:364) calledJSC::Options::setOption(env + 8)with the defaultverify = true. Every verified change runsOptions::notifyOptionsChanged, and that pass multiplies the thresholds byjitPolicyScalewhenever the scale was overridden (Options.cpp:908,scaleJITPolicy).Options::initializethen runs the same pass once more after the callback.Fix
verify = false, asjsc.cppdoes for--options, and let the singlenotifyOptionsChangedthatOptions::initializeruns after the callback verify them all. The scale is applied exactly once.assertOptionsAreCoherent()call at the end of the callback. It ran before the pass that makes the options coherent, so withverify = falseit would crashBUN_JSC_useProfiler=1at startup (the pass turns the concurrent JIT off for the profiler).Options::finalizeruns the same assertion afterwards.bun bd test test/cli/run/env.test.ts(100 pass). The new test fails on the released binary (thresholdForOptimizeAfterWarmUp=125) and passes with the fix (500).test/js/node/buffer-jit.test.tsfrom test(buffer-jit): speed up buffer-jit.test.ts and pin exact compile counts #40542 passes against the fixed binary.Background
BUN_JSC_<name>environment variables at startup. Some options are derived from others:jitPolicyScalescales seven threshold options,useProfilerturnsuseConcurrentJIToff.notifyOptionsChangedcomputes those derived values.Options::initializetakes a customization callback (Bun's lambda inJSCInitialize), runs it, then runsnotifyOptionsChangedonce.setOption(arg, verify)runs the same pass after each option whenverifyis true, which is meant for options set at runtime, after initialization.jitPolicyScale=0.05and its effective thresholds sat at the floor because the scale was applied two or three times.no test proof · iteration 1 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/cli/run/env.test.ts