Make `JSValue.asCell` more efficient by taylordotfish · Pull Request #23386 · oven-sh/bun

taylordotfish · 2025-10-08T22:44:09Z

Avoid calling into C++ in jsc.JSValue.asCell.

(For internal tracking: fixes ENG-20820)

linear · 2025-10-08T22:44:12Z

ENG-20820 Make `JSValue.asCell` more efficient

robobun · 2025-10-08T22:44:20Z

^{Updated 4:38 PM PT - Oct 15th, 2025}

✅ @taylordotfish, your commit 662f432698c7015c95026053929ced5140529cdd passed in Build #29283! 🎉

🧪 To try this PR locally:

bunx bun-pr 23386

That installs a local version of the PR into your bun-23386 executable, so you can run:

bun-23386 --bun

coderabbitai · 2025-10-08T22:48:07Z

Walkthrough

Adds DecodedJSValue helpers to detect and extract cell pointers, changes JSValue.asCell to use decoded extraction instead of a C++ extern, and removes the C++ JSC__JSValue__asCell binding and its header declaration; JSC__JSValue__asString now uses JSC::asString on the decoded value.

Changes

Cohort / File(s)	Summary of changes
DecodedJSValue cell helpers `src/bun.js/bindings/DecodedJSValue.zig`	Added internal `asU64(self)` bit-cast helper; added `pub fn isCell(self) bool` and `pub fn asCell(self) ?*jsc.JSCell` that use `ffi.NotCellMask`, treat `0` as a cell, and assert via `bun.assertf` when appropriate.
JSValue asCell via decode `src/bun.js/bindings/JSValue.zig`	Removed extern `JSC__JSValue__asCell`; `pub fn asCell(this: JSValue) *JSCell` now asserts `this.isCell()` and returns `this.decode().asCell().?`, using the decoded-path instead of calling the external C++ binding.
C++ binding removals/adjustments `src/bun.js/bindings/bindings.cpp`, `src/bun.js/bindings/headers.h`	Removed exported `JSC__JSValue__asCell(JSC::EncodedJSValue)` and its header declaration; updated `JSC__JSValue__asString(JSC::EncodedJSValue)` implementation to use `JSC::asString(value)` on the decoded value rather than decoding to a cell and returning `.asCell()`.

Possibly related PRs

Add jsc.DecodedJSValue; make jsc.Strong more efficient #23218 — Makes related Zig/C++ binding-layer changes introducing/using DecodedJSValue and replacing C++ externs with decoded-based logic.

Pre-merge checks

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Out of Scope Changes Check	⚠️ Warning	While the pull request focuses on optimizing `JSValue.asCell`, it also modifies the `JSC__JSValue__asString` binding, which is unrelated to the stated objective of avoiding C++ calls in the `asCell` implementation and therefore appears outside the linked issue’s scope.	Please remove the changes to the `asString` binding or move them into a separate pull request, or otherwise update the PR objectives to explicitly include this additional optimization.
Description Check	⚠️ Warning	The pull request description does not follow the repository’s template because it lacks the required “What does this PR do?” and “How did you verify your code works?” sections, leaving out key information on implementation details and testing procedures.	Please update the description to use the provided template by adding a “What does this PR do?” section that outlines the change and a “How did you verify your code works?” section describing your testing strategy.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title Check	✅ Passed	The title clearly and concisely summarizes the primary change by indicating that the `JSValue.asCell` method is being optimized for efficiency, which directly reflects the core update of removing the C++ call overhead in that API.
Linked Issues Check	✅ Passed	The code changes fully implement the objective of ENG-20820 by removing the C++ extern for `asCell` and replacing it with a Zig-level implementation that asserts `isCell` and decodes internally, thereby avoiding any C++ calls in the `JSValue.asCell` path.

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

coderabbitai

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

Disabled knowledge base sources:

Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 767e03e and a8ef91e3fb886e6d9cfb0a2aecfea81e11bfe88a.

📒 Files selected for processing (4)

src/bun.js/bindings/DecodedJSValue.zig (2 hunks)
src/bun.js/bindings/JSValue.zig (1 hunks)
src/bun.js/bindings/bindings.cpp (0 hunks)
src/bun.js/bindings/headers.h (0 hunks)

💤 Files with no reviewable changes (2)

src/bun.js/bindings/headers.h
src/bun.js/bindings/bindings.cpp

🧰 Additional context used

📓 Path-based instructions (3)

**/*.zig

📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)

**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue

Files:

src/bun.js/bindings/JSValue.zig
src/bun.js/bindings/DecodedJSValue.zig

src/bun.js/**/*.zig

📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)

src/bun.js/**/*.zig: In Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS and re-export toJS/fromJS/fromJSDirect
Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Use parameter name globalObject (not ctx) and accept (*JSC.JSGlobalObject, *JSC.CallFrame) in binding methods/constructors
Implement getters as get(this, globalObject) returning JSC.JSValue and matching the .classes.ts interface
Provide deinit() for resource cleanup and finalize() that calls deinit(); use bun.destroy(this) or appropriate destroy pattern
Access JS call data via CallFrame (argument(i), argumentCount(), thisValue()) and throw errors with globalObject.throw(...)
For properties marked cache: true, use the generated Zig accessors (NameSetCached/GetCached) to work with GC-owned values
In finalize() for objects holding JS references, release them using .deref() before destroy

Files:

src/bun.js/bindings/JSValue.zig
src/bun.js/bindings/DecodedJSValue.zig

src/**/*.zig

📄 CodeRabbit inference engine (CLAUDE.md)

In Zig code, manage memory carefully and use defer for cleanup of allocations/resources

src/**/*.zig: Use the # prefix to declare private fields in Zig structs (e.g., struct { #foo: u32 })
Prefer decl literals when initializing values in Zig (e.g., const decl: Decl = .{ .binding = 0, .value = 0 })
Place @import directives at the bottom of Zig files
Use @import("bun") instead of @import("root").bun

When adding debug logs in Zig, create a scoped logger and log via Bun APIs: const log = bun.Output.scoped(.${SCOPE}, .hidden); then log("...", .{})

Files:

src/bun.js/bindings/JSValue.zig
src/bun.js/bindings/DecodedJSValue.zig

🧠 Learnings (9)

📚 Learning: 2025-08-30T00:13:36.815Z

Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : In Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS<ClassName> and re-export toJS/fromJS/fromJSDirect

Applied to files:

src/bun.js/bindings/JSValue.zig
src/bun.js/bindings/DecodedJSValue.zig

📚 Learning: 2025-08-30T00:11:00.890Z

Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-08-30T00:11:00.890Z
Learning: Applies to **/*.zig : Wrap the Bun__<Type>__toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue

Applied to files:

src/bun.js/bindings/JSValue.zig
src/bun.js/bindings/DecodedJSValue.zig

📚 Learning: 2025-08-30T00:11:57.076Z

Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-08-30T00:11:57.076Z
Learning: Applies to src/{**/js_*.zig,bun.js/api/**/*.zig} : Use bun.JSError!JSValue for proper error propagation in JS-exposed Zig functions

Applied to files:

src/bun.js/bindings/JSValue.zig
src/bun.js/bindings/DecodedJSValue.zig

📚 Learning: 2025-08-30T00:13:36.815Z

Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions

Applied to files:

src/bun.js/bindings/DecodedJSValue.zig

📚 Learning: 2025-08-30T00:13:36.815Z

Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Create Bun JSC class bindings by pairing a TypeScript class interface with a Zig implementation and relying on the generator to produce glue code

Applied to files:

src/bun.js/bindings/DecodedJSValue.zig

📚 Learning: 2025-10-07T03:19:53.031Z

Learnt from: CR
PR: oven-sh/bun#0
File: src/CLAUDE.md:0-0
Timestamp: 2025-10-07T03:19:53.031Z
Learning: Applies to src/**/*.zig : Use import("bun") instead of import("root").bun

Applied to files:

src/bun.js/bindings/DecodedJSValue.zig

📚 Learning: 2025-10-04T09:51:30.294Z

Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-04T09:51:30.294Z
Learning: Applies to src/js/bun/** : Organize Bun-specific built-in modules under src/js/bun/ (e.g., bun:ffi, bun:sqlite)

Applied to files:

src/bun.js/bindings/DecodedJSValue.zig

📚 Learning: 2025-08-30T00:13:36.815Z

Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : Use parameter name globalObject (not ctx) and accept (*JSC.JSGlobalObject, *JSC.CallFrame) in binding methods/constructors

Applied to files:

src/bun.js/bindings/DecodedJSValue.zig

📚 Learning: 2025-08-30T00:11:57.076Z

Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-08-30T00:11:57.076Z
Learning: Applies to src/bun.js/api/BunObject.zig : Create a getter function in BunObject.zig (define const Getter with toJSGetter and export it in exportAll)

Applied to files:

src/bun.js/bindings/DecodedJSValue.zig

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

GitHub Check: Format

🔇 Additional comments (2)

src/bun.js/bindings/JSValue.zig (1)

1080-1086: LGTM! Efficient cell extraction via DecodedJSValue.

The refactor successfully avoids the C++ extern call by leveraging the new DecodedJSValue.asCell() API. The assertion and unwrap are correct since isCell() excludes .zero.

src/bun.js/bindings/DecodedJSValue.zig (1)

22-24: Helper and import additions look correct.

The asU64() bitcast helper, asCell() implementation, and FFI import are all sound. Once isCell() is fixed, these will work correctly.

Also applies to: 31-35, 49-49

(For internal tracking: fixes ENG-20820)

paperclover · 2025-10-09T19:35:01Z

        bun.unsafeAssert(this.isCell());
-        return JSC__JSValue__asCell(this);
+        // We know `DecodedJSValue.asCell` cannot return null, since `isCell` already checked for
+        // `.zero`.


this isn't true

Which part isn't true? Unlike JSC::JSValue::isCell in C++, jsc.JSValue.isCell in Zig does check for .zero (see the method above this one). Are you saying DecodedJSValue.asCell could still return null even if the JSValue isn't .zero? I don't see how that's possible.

…as-cell

coderabbitai

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

Disabled knowledge base sources:

Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 5f21244 and 662f432.

📒 Files selected for processing (2)

src/bun.js/bindings/JSValue.zig (1 hunks)
src/bun.js/bindings/bindings.cpp (0 hunks)

💤 Files with no reviewable changes (1)

src/bun.js/bindings/bindings.cpp

🧰 Additional context used

📓 Path-based instructions (3)

**/*.zig

📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)

**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue

Files:

src/bun.js/bindings/JSValue.zig

src/bun.js/**/*.zig

📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)

src/bun.js/**/*.zig: In Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS and re-export toJS/fromJS/fromJSDirect
Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Use parameter name globalObject (not ctx) and accept (*JSC.JSGlobalObject, *JSC.CallFrame) in binding methods/constructors
Implement getters as get(this, globalObject) returning JSC.JSValue and matching the .classes.ts interface
Provide deinit() for resource cleanup and finalize() that calls deinit(); use bun.destroy(this) or appropriate destroy pattern
Access JS call data via CallFrame (argument(i), argumentCount(), thisValue()) and throw errors with globalObject.throw(...)
For properties marked cache: true, use the generated Zig accessors (NameSetCached/GetCached) to work with GC-owned values
In finalize() for objects holding JS references, release them using .deref() before destroy

Files:

src/bun.js/bindings/JSValue.zig

src/**/*.zig

📄 CodeRabbit inference engine (CLAUDE.md)

In Zig code, manage memory carefully and use defer for cleanup of allocations/resources

When adding debug logs in Zig, create a scoped logger and log via Bun APIs: const log = bun.Output.scoped(.${SCOPE}, .hidden); then log("...", .{})

src/**/*.zig: Use private fields in Zig with the # prefix (e.g., struct { #foo: u32 };)
Prefer decl literals in Zig (e.g., const decl: Decl = .{ .binding = 0, .value = 0 };)
Prefer placing @import statements at the bottom of the Zig file (formatter may reorder automatically)
Prefer @import("bun") rather than @import("root").bun or @import("../bun.zig")

Files:

src/bun.js/bindings/JSValue.zig

🧠 Learnings (3)

📚 Learning: 2025-08-30T00:13:36.815Z

Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : In Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS<ClassName> and re-export toJS/fromJS/fromJSDirect

Applied to files:

src/bun.js/bindings/JSValue.zig

📚 Learning: 2025-08-30T00:11:57.076Z

Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-08-30T00:11:57.076Z
Learning: Applies to src/{**/js_*.zig,bun.js/api/**/*.zig} : Use bun.JSError!JSValue for proper error propagation in JS-exposed Zig functions

Applied to files:

src/bun.js/bindings/JSValue.zig

📚 Learning: 2025-08-30T00:11:00.890Z

Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-08-30T00:11:00.890Z
Learning: Applies to **/*.zig : Wrap the Bun__<Type>__toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue

Applied to files:

src/bun.js/bindings/JSValue.zig

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

GitHub Check: Format

coderabbitai Bot reviewed Oct 8, 2025

View reviewed changes

Comment thread src/bun.js/bindings/DecodedJSValue.zig

Make JSValue.asCell more efficient

32f29ff

(For internal tracking: fixes ENG-20820)

taylordotfish force-pushed the taylor.fish/js-value-as-cell branch from 919665f to 32f29ff Compare October 8, 2025 22:53

[autofix.ci] apply automated fixes

5f21244

paperclover reviewed Oct 9, 2025

View reviewed changes

Merge remote-tracking branch 'origin/main' into taylor.fish/js-value-…

662f432

…as-cell

coderabbitai Bot reviewed Oct 15, 2025

View reviewed changes

Comment thread src/bun.js/bindings/JSValue.zig

Jarred-Sumner merged commit 81c453c into main Oct 15, 2025
39 of 50 checks passed

Jarred-Sumner deleted the taylor.fish/js-value-as-cell branch October 15, 2025 21:34

coderabbitai Bot mentioned this pull request Nov 24, 2025

Fix conversions from JSValue to FFI pointer #25045

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Make `JSValue.asCell` more efficient#23386

Make `JSValue.asCell` more efficient#23386
Jarred-Sumner merged 3 commits into
mainfrom
taylor.fish/js-value-as-cell

taylordotfish commented Oct 8, 2025

Uh oh!

linear Bot commented Oct 8, 2025

Uh oh!

robobun commented Oct 8, 2025 •

edited

Loading

Uh oh!

coderabbitai Bot commented Oct 8, 2025 •

edited

Loading

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

paperclover Oct 9, 2025

Uh oh!

taylordotfish Oct 9, 2025 •

edited

Loading

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

taylordotfish commented Oct 8, 2025

Uh oh!

linear Bot commented Oct 8, 2025

Uh oh!

robobun commented Oct 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai Bot commented Oct 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Possibly related PRs

Pre-merge checks

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

paperclover Oct 9, 2025

Choose a reason for hiding this comment

Uh oh!

taylordotfish Oct 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

robobun commented Oct 8, 2025 •

edited

Loading

coderabbitai Bot commented Oct 8, 2025 •

edited

Loading

taylordotfish Oct 9, 2025 •

edited

Loading