Replace bun_core::hint::cold with core::hint::cold_path - #39601
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (20)
💤 Files with no reviewable changes (2)
Included review availability: Your plan provides up to 5 included reviews per hour; 1 remains after this review. WalkthroughThe PR removes ChangesCold-path hint migration
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — mechanical swap of the in-tree #[cold] #[inline(never)] helper for stdlib core::hint::cold_path(), per the TODO in the deleted file. The inline nit about two remaining stale @branchHint comments is optional cleanup and doesn't block.
Extended reasoning...
Overview
This PR resolves a TODO in src/bun_core/hint.rs: replace the hand-rolled #[cold] #[inline(never)] pub const fn cold() {} helper with the now-stable core::hint::cold_path() (Rust 1.95+; toolchain is on 1.99). It touches 17 files: 25 call sites mechanically rewritten from bun_core::hint::cold() / crate::hint::cold() / $crate::hint::cold() to core::hint::cold_path() (with ::core:: inside the macro_rules! body in env_var.rs, matching the file's other absolute-path ::core usages), the module file deleted, its pub mod hint; line removed from lib.rs, and one adjacent stale // @branchHint(.unlikely) comment dropped in resolver.rs.
Security risks
None. This is a compiler-hint-only change with no runtime semantics — both forms emit identical LLVM branch_weights metadata (verified in the PR description with an IR diff). No user input, no auth/crypto, no data handling.
Level of scrutiny
Low. This is exactly the mechanical TODO-resolution the deleted file's own doc comment requested. Every hunk is a one-line s/bun_core::hint::cold()/core::hint::cold_path()/ (or crate:: / $crate:: variant); no surrounding logic changes. The compiler enforces correctness — a wrong path or missed call site would fail bun bd, which the author ran. A post-PR grep for hint::cold and bun_core::hint over src/ confirms no stragglers (including cfg-gated code).
Other factors
- The one reported finding is a nit: two sibling
// @branchHint … no stable Rust equivalentcomments insrc/sql_jsc/mysql/MySQLRequestQueue.rs:76andsrc/runtime/jsc_hooks.rs:4470are now factually stale. Those sites never calledbun_core::hint::cold()(the porters dropped the hint entirely), so the PR'shint::coldgrep wouldn't have found them. It's a fair same-class cleanup suggestion but comment-only and non-blocking. - No test is added, which is correct per REVIEW.md — there is no observable behavior to test; this is an optimizer hint swap.
- No prior human or bot review comments to address (CodeRabbit was rate-limited).
Delete dead code in the same PR that makes it deadis satisfied: the helper module and itspub modline are removed.
|
Status: ready for review. Rebased onto main (one trivial conflict in
|
There was a problem hiding this comment.
LGTM — thanks for addressing the two stale @branchHint comments in b3eca49; rg 'branchHint' and rg 'hint::cold' over src/ are now both clean.
What was reviewed:
- All 25+2 call sites are 1:1 swaps to
core::hint::cold_path(); no surrounding logic changed. - The
macro_rules!site inenv_var.rsuses the::core::path, matching the neighboring::core::str::from_utf8hygiene. hint.rsand itspub mod hint;line are removed; no remaining references insrc/(including cfg-gated code).- Toolchain is nightly-2026-07-20 (rustc 1.99);
cold_pathstabilized in 1.95.
Extended reasoning...
Overview
This PR replaces the in-tree bun_core::hint::cold() helper (an empty #[cold] #[inline(never)] fn) with the now-stable core::hint::cold_path() intrinsic across 25 call sites in 15 files, deletes src/bun_core/hint.rs and its pub mod line, and — in a follow-up commit responding to the prior review — replaces two stale "no stable Rust equivalent" comments with core::hint::cold_path() at jsc_hooks.rs:4470 and MySQLRequestQueue.rs:76.
Security risks
None. This is a compiler-hint swap with no runtime semantics — both forms emit identical branch_weights metadata and the new form emits strictly fewer instructions (no cross-crate call in the cold block). No user input, no allocation, no control-flow change.
Level of scrutiny
Low. Every hunk is a mechanical one-line substitution inside an existing branch body; the enclosing logic is untouched. The helper's own TODO named this exact replacement. rust-toolchain.toml confirms rustc 1.99 (≥ 1.95 where cold_path stabilized), so availability is verified. The macro-body call correctly uses the ::core:: absolute path matching neighboring ::core:: usage in that macro. bun bd was run per the description, so all touched crates compiled.
Other factors
The prior run's 🟡 nit (two remaining @branchHint comments claiming no stable equivalent) has been addressed in commit b3eca49 — both sites now carry core::hint::cold_path();. Post-PR greps for branchHint and hint::cold over src/ return nothing, confirming the sweep is complete including cfg-gated platform code. No tests are added, which is correct: there is no observable behavior to assert on for an optimizer hint. The dead module is deleted in the same PR per REVIEW.md's "delete dead code in the same PR that makes it dead".
|
Updated 6:45 PM PT - Aug 18th, 2026
✅ @robobun, your commit a61de7f48534919482e4903c828505cb3f54f271 passed in 🧪 To try this PR locally: bunx bun-pr 39601That installs a local version of the PR into your bun-39601 --bun |
There was a problem hiding this comment.
LGTM — mechanical swap of the in-tree #[cold] shim for the now-stable core::hint::cold_path(), plus the two restored hints from the earlier review comment.
What was reviewed:
- Confirmed all 27 call sites now use
core::hint::cold_path()andsrc/has no remaininghint::cold/bun_core::hint/@branchHintreferences;pub mod hint;is gone. - The macro-body call in
env_var.rscorrectly uses the::core::absolute path like its neighbors. - The new lint entry in
port-era-markers.test.tsonly scans comment lines, so it won't false-positive on the intrinsic itself.
Extended reasoning...
Overview
This PR replaces the in-tree bun_core::hint::cold() shim (an empty #[cold] #[inline(never)] fn whose own TODO said to swap it for core::hint::cold_path() once stabilized) with the stdlib intrinsic across 25 call sites in 15 files, deletes src/bun_core/hint.rs and its pub mod line, restores the hint at two sites that dropped it during the Zig→Rust port with stale "no stable Rust equivalent" comments (MySQLRequestQueue::can_pipeline, jsc_hooks.rs force_loader_type branch), and adds a @branchHint entry to the port-era-markers source lint.
Security risks
None. cold_path() is a codegen hint with no runtime semantics; the change is a 1:1 substitution of one branch-weight annotation mechanism for another that produces identical LLVM branch_weights metadata (verified by the author with --emit=llvm-ir). No user input, no I/O, no control-flow changes.
Level of scrutiny
Low. This is the canonical "delete the shim once stdlib stabilizes the real thing" cleanup — mechanical find-and-replace with no observable behavior change. The two newly-hinted branches (b3eca49) are an env-var feature-flag check and an override-loader fast path, both obviously cold and matching their siblings (the Postgres can_pipeline already had the hint). The lint addition follows the existing pattern in the file exactly and is scoped to comment lines only.
Other factors
The one review comment I left on the first revision (two remaining stale @branchHint comments) was addressed in b3eca49 and the thread is resolved. I re-grepped src/ on the current tree: 27 core::hint::cold_path() sites, zero hint::cold/bun_core::hint/branchHint hits, and pub mod hint is gone from bun_core/lib.rs. bun bd builds per the author, and the new lint test was verified to fail on main and pass here.
core::hint::cold_path is stable since Rust 1.95. The toolchain is nightly-2026-07-20. Call it directly at the 25 call sites and delete the bun_core::hint module.
MySQLRequestQueue::can_pipeline now matches its Postgres twin. The jsc_hooks force_loader branch gets the hint its comment said was dropped.
This change removes the last three. The lint keeps them out and names core::hint::cold_path() as the replacement.
eab21d0 to
a61de7f
Compare
There was a problem hiding this comment.
LGTM — mechanical swap of the in-tree #[cold] shim for the now-stable core::hint::cold_path(), plus deletion of the dead module and a lint entry to keep @branchHint comments from reappearing.
What was reviewed:
- Verified all 27 call sites (25 replaced + 2 restored from stale comments) now use
core::hint::cold_path(); nobun_core::hint,hint::cold, or@branchHintremain insrc/. - The macro-body site in
env_var.rscorrectly uses the::core::absolute path. - The two sites flagged in the earlier review round (
MySQLRequestQueue::can_pipeline,jsc_hooks.rsforce_loader_type) now match their siblings and the stale comments are gone.
Extended reasoning...
Overview
This PR replaces the in-tree bun_core::hint::cold() helper (an empty #[cold] #[inline(never)] function whose call marks the surrounding branch cold) with the now-stable core::hint::cold_path() intrinsic. It touches 20 files: 18 Rust source files where the call is swapped 1:1, src/bun_core/hint.rs (deleted), src/bun_core/lib.rs (pub mod hint; removed), and the port-era-markers source-lint test (new @branchHint pattern). Two additional sites in MySQLRequestQueue.rs and jsc_hooks.rs gain a cold_path() call where the port had dropped the hint and left a now-false "no stable Rust equivalent" comment — these were flagged in an earlier review round and addressed.
Security risks
None. This is a compiler branch-prediction hint with no observable runtime semantics beyond code layout. No inputs are parsed, no data flows change, no allocations or lifetimes are affected.
Level of scrutiny
Low. The change is a mechanical find-and-replace whose equivalence the PR author verified at the LLVM IR level (identical branch_weights metadata; the old form additionally emitted a dead cross-crate call in the cold block that LTO would remove). The helper's own doc comment carried a TODO pointing at exactly this replacement once rust-lang/rust#117174 stabilized, which it has (Rust 1.95; toolchain is 1.99). bun bd builds and the touched crates all recompiled per the PR notes.
Other factors
- I grepped
src/post-change: 27core::hint::cold_path()sites, zero remainingbun_core::hint/hint::cold/@branchHintreferences, andlib.rsno longer mentionshint. - The macro expansion in
env_var.rscorrectly uses::core::hint::cold_path()(absolute path) matching the other::core::uses in that macro body. - The added lint entry follows the existing pattern in
port-era-markers.test.tsexactly and only inspects lines containing//, so it cannot false-positive on identifiers or string literals. - The prior inline comment I left about the two sibling
@branchHintcomments has been addressed and the thread resolved. - The mechanical gate confirms the lint test fails on main (3 hits) and passes here, and both debug/ASAN and release builds succeed.
2 participants
Problem
src/bun_core/hint.rshas acold()helper: an empty#[cold] #[inline(never)]function. A call to it marks the calling branch as cold.core::hint::cold_path()once Improve UEFI stdio rust-lang/rust#117174 stabilizes.cold_pathis stable (and const) since Rust 1.95. The toolchain is nightly-2026-07-20 (rustc 1.99).Fix
bun_core::hint::cold()(15 files) withcore::hint::cold_path(). Deletesrc/bun_core/hint.rsand itspub mod hint;line.MySQLRequestQueue::can_pipeline(its Postgres twin has the hint) and theforce_loader_typebranch insrc/runtime/jsc_hooks.rs. Addcold_path()to both and delete the comments.src/now has nobranchHintorhint::coldleft.rustc -O --emit=llvm-iremits the samebranch_weights(1vs2000) either way. The old cross-crate form also leaves acallto the empty function in the cold block. IR is in the notes.bun bdbuilds.test/internal/source-lints/port-era-markers.test.tsgets a@branchHintentry. It fails on main (the three comments above) and passes here. The hint swap itself has no observable behavior to test.bun bd test test/js/node/module/require-extensions.test.tspasses too (more in the notes).Background
#[cold]function. It emits LLVMbranch_weightson each branch into that block. LLVM lays the block out off the hot path.#[cold]function.#[inline(never)]kept the call in place so the rule saw it. Outsidebun_corethat was a real cross-crate call until LTO removed it.core::hint::cold_path()is an#[inline(always)] #[cold]wrapper around thecold_pathintrinsic. The intrinsic is#[cold]too, so the same rule applies. The backend emits no instruction for it.Notes
Sample (
fis the same in both files, only the hint differs):Old (
rustc -O --emit=llvm-ir):New:
Other checks:
bun bd testalso passes fortest/js/bun/resolve/import-query.test.tsandtest/js/bun/shell/env.positionals.test.ts. The call inside themacro_rules!body insrc/bun_core/env_var.rsuses::core::hint::cold_path(), like the other paths in that macro.rustfmt --checkon the touched files is clean. Every touched crate recompiled in thebun bdrun (fresh rlib hashes forbun_core,bun_jsc,bun_resolver,bun_http_jsc,bun_sql_jsc,bun_runtime). The grep oversrc/is cfg-agnostic, so platform-gated code is covered. The first revision of this PR replaced the 25 calls only. The second commit adds the two restored sites after a review comment pointed at their stale comments. Rebased onto main once: #39575 had moved the first-dial block injs_valkey.rsinto a newensure_dialing(), so the hint call there moved with it. Still 25 replaced sites plus the 2 restored ones.[review] gate passed · iteration 1 · 20 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 1 rejected · iteration 1
evidence per changed file