[JSC] Stop forcing the generic typed array prototype functions inline - #465
Conversation
Every genericTypedArrayViewProto*<ViewClass> is reached from a 12-way type switch (CALL_GENERIC_TYPEDARRAY_PROTOTYPE_FUNCTION) that tail-calls it and returns. ALWAYS_INLINE on the entry points gains nothing there and puts twelve copies of each body (and, for sort/toSorted, twelve copies of the whole stable-sort implementation) into one host function: typedArrayViewProtoFuncSort and ToSorted are 290 KB each in a release JavaScriptCore, the two largest functions in the binary. Demote the entry points and SortImpl to plain inline so the per-ViewClass instantiations are separate functions again; the loop helpers that specialise on a functor (typedArrayViewForEachImpl, typedArrayIndexOfImpl, typedArrayLastIndexOfImpl) stay ALWAYS_INLINE. Same for ArrayPrototype's sortImpl, which was being duplicated into both arrayProtoFuncSort and arrayProtoFuncToSorted (~64 KB each).
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 12 minutes Limit details: You’ve used all 1 included review currently available under your plan. You completed 74 included PR reviews in the past 7 days; at that activity level, included reviews refill at 1 review per hour. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — mechanical ALWAYS_INLINE → inline demotion on function signatures only.
What was reviewed:
- Confirmed every changed line touches only the inline specifier; no function body or logic changed.
- Verified each
genericTypedArrayViewProto*is reached viaCALL_GENERIC_TYPEDARRAY_PROTOTYPE_FUNCTIONin theJSTypedArrayViewPrototypeFunctions*.cppdispatch shims, so weakening the hint doesn't strand any caller. - Checked that the hot-loop helpers (
typedArrayViewForEachImpl,typedArrayIndexOfImpl,typedArrayLastIndexOfImpl) keepALWAYS_INLINE, andsortImplin ArrayPrototype.cpp staysstaticso linkage is unchanged.
Extended reasoning...
Overview
This PR changes 31 function signatures in JSGenericTypedArrayViewPrototypeFunctions.h and one in ArrayPrototype.cpp from ALWAYS_INLINE (i.e. __attribute__((always_inline))) to plain inline. Every hunk is a one-token substitution on the declarator line; no function body, control flow, or type is touched.
Security risks
None. This is a compiler inlining hint change only. It cannot alter observable JS semantics, memory safety, or bounds checking — the generated code is the same instructions, just partitioned into separate symbols instead of being force-inlined into the 12-way type-dispatch host function.
Level of scrutiny
Low. The diff is purely mechanical and the intent (recover ~3–4 MB of binary bloat introduced when #403 restored ALWAYS_INLINE's force semantics) is well-documented with per-function .text size measurements. I confirmed via grep that the affected templates are called only through CALL_GENERIC_TYPEDARRAY_PROTOTYPE_FUNCTION in the split JSTypedArrayViewPrototypeFunctions{1..5}.cpp shims — a switch that immediately returns the result — so dropping the force hint just lets each <ViewClass> instantiation stay a separate function rather than being copied 12× into one host body. The functor-specialised inner loops that actually matter for perf keep ALWAYS_INLINE.
Other factors
Templates defined in a header already have inline linkage, and sortImpl in the .cpp keeps static, so there is no ODR or linkage change. The only possible effect is codegen layout / binary size, which is exactly the point of the PR and will be validated by the pinned Bun draft PR's size annotations. No outstanding reviewer comments.
1 participant
Follow-up to #403. Restoring
ALWAYS_INLINE=__always_inline__cost every Bun platform 2.5–4 MB of stripped binary (bun-linux-x64 +3.67 MB, bun-windows-x64 +4.04 MB, oven-sh/bun#37352). A per-function diff of.textbetween the two FreeBSD (non-LTO, unstripped)bun-profilebinaries either side of the bump shows the single most concentrated cost is this header:typedArrayViewProtoFuncSorttypedArrayViewProtoFuncToSortedtypedArrayViewProtoFuncFiltertypedArrayViewProtoFuncMaptypedArrayViewProtoFuncFind/FindIndex/FindLast/FindLastIndextypedArrayViewProtoFuncSome/Every/ForEacharrayProtoFuncSort/arrayProtoFuncToSortedtypedArrayViewProtoFuncSortandToSortedare now the two largest functions in Bun's whole binary — bigger thansqlite3VdbeExecorDFG::ByteCodeParser::parseBlock.Every
genericTypedArrayViewProto*<ViewClass>is reached from the 12-way type switch inCALL_GENERIC_TYPEDARRAY_PROTOTYPE_FUNCTION, which tail-calls it and returns, so forcing them inline gains nothing: it just moves the twelve<ViewClass>instantiations (and for sort/toSorted, twelve copies ofarrayStableSort+ insertion sort + the comparator path) into one host function. This demotes the entry points andgenericTypedArrayViewProtoFuncSortImplto plaininline, so each instantiation is its own function again (as it was before #403). The helpers that specialise on a functor and are the actual hot loops —typedArrayViewForEachImpl,typedArrayIndexOfImpl,typedArrayLastIndexOfImpl— stayALWAYS_INLINE.Same change for
ArrayPrototype.cpp'ssortImpl, which was being duplicated into botharrayProtoFuncSortandarrayProtoFuncToSorted.No behaviour change; only inlining hints. I will pin a Bun draft PR to this PR's preview tarball to get the per-platform size annotation.