replace jsDoubleNumber with jsNumber to use NumberTag if possible#22476
Conversation
|
Updated 4:50 PM PT - Sep 7th, 2025
❌ @dylan-conway, your commit 08bc393 has 3 failures in
🧪 To try this PR locally: bunx bun-pr 22476That installs a local version of the PR into your bun-22476 --bun |
WalkthroughUnifies numeric JSValue encoding by replacing jsDoubleNumber with jsNumber across bindings and modules; JSValue.zig now encodes integer-valued floats as int32 when safe (adds canBeStrictInt32 and removes two helpers). Removes some exported JSValue wrappers and simplifies a valkey ArrayBuffer return. Changes
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (4)**/*.{cpp,h}📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
Files:
**/*.cpp📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
Files:
src/bun.js/bindings/**/*.cpp📄 CodeRabbit inference engine (CLAUDE.md)
Files:
**/*.{cpp,hpp,h,cc,cxx}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧠 Learnings (1)📓 Common learnings⏰ 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)
🔇 Additional comments (2)
✨ Finishing Touches
🧪 Generate unit tests
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/bun.js/bindings/NodeFSStatBinding.cpp (2)
774-781: Replace remaining jsDoubleNumber in BigInt→ms conversions.For consistency and to benefit NumberTag where possible, use jsNumber here as well.
- atimeMs = jsDoubleNumber(atimeNs.toBigInt64(globalObject) / kNsPerMsBigInt); + atimeMs = jsNumber(atimeNs.toBigInt64(globalObject) / kNsPerMsBigInt); RETURN_IF_EXCEPTION(scope, {}); - mtimeMs = jsDoubleNumber(mtimeNs.toBigInt64(globalObject) / kNsPerMsBigInt); + mtimeMs = jsNumber(mtimeNs.toBigInt64(globalObject) / kNsPerMsBigInt); RETURN_IF_EXCEPTION(scope, {}); - ctimeMs = jsDoubleNumber(ctimeNs.toBigInt64(globalObject) / kNsPerMsBigInt); + ctimeMs = jsNumber(ctimeNs.toBigInt64(globalObject) / kNsPerMsBigInt); RETURN_IF_EXCEPTION(scope, {}); - birthtimeMs = jsDoubleNumber(birthtimeNs.toBigInt64(globalObject) / kNsPerMsBigInt); + birthtimeMs = jsNumber(birthtimeNs.toBigInt64(globalObject) / kNsPerMsBigInt); RETURN_IF_EXCEPTION(scope, {});
859-866: Same here: swap jsDoubleNumber → jsNumber.Mirror the change in construct path.
- atimeMs = jsDoubleNumber(atimeNs.toBigInt64(globalObject) / kNsPerMsBigInt); + atimeMs = jsNumber(atimeNs.toBigInt64(globalObject) / kNsPerMsBigInt); RETURN_IF_EXCEPTION(scope, {}); - mtimeMs = jsDoubleNumber(mtimeNs.toBigInt64(globalObject) / kNsPerMsBigInt); + mtimeMs = jsNumber(mtimeNs.toBigInt64(globalObject) / kNsPerMsBigInt); RETURN_IF_EXCEPTION(scope, {}); - ctimeMs = jsDoubleNumber(ctimeNs.toBigInt64(globalObject) / kNsPerMsBigInt); + ctimeMs = jsNumber(ctimeNs.toBigInt64(globalObject) / kNsPerMsBigInt); RETURN_IF_EXCEPTION(scope, {}); - birthtimeMs = jsDoubleNumber(birthtimeNs.toBigInt64(globalObject) / kNsPerMsBigInt); + birthtimeMs = jsNumber(birthtimeNs.toBigInt64(globalObject) / kNsPerMsBigInt); RETURN_IF_EXCEPTION(scope, {});
🧹 Nitpick comments (4)
src/bun.js/bindings/v8/V8Data.h (1)
57-59: Clarify comment to match behavior (HeapNumber is always encoded as double).HeapNumber may also hold int32-range values; the reason we force jsDoubleNumber here is to preserve “double” identity for non-Smi values. Suggest rewording to avoid implying it “doesn’t fit in int32_t.”
- // a number that doesn't fit in int32_t, always EncodeAsDouble + // HeapNumber: force EncodeAsDouble to preserve double identity for non-Smi valuessrc/bun.js/modules/NodeBufferModule.h (1)
213-215: Likely copy/paste: isAscii fallback points to isUtf8.The last argument to JSFunction::create for "isAscii" references jsBufferConstructorFunction_isUtf8; it should reference isAscii.
- put(JSC::Identifier::fromString(vm, "isAscii"_s), JSC::JSFunction::create(vm, globalObject, 1, "isAscii"_s, jsBufferConstructorFunction_isAscii, ImplementationVisibility::Public, NoIntrinsic, jsBufferConstructorFunction_isUtf8)); + put(JSC::Identifier::fromString(vm, "isAscii"_s), JSC::JSFunction::create(vm, globalObject, 1, "isAscii"_s, jsBufferConstructorFunction_isAscii, ImplementationVisibility::Public, NoIntrinsic, jsBufferConstructorFunction_isAscii));src/bun.js/bindings/JSValue.zig (1)
798-808: Make canBeStrictInt32 robust to out-of-range casts (optional).@intFromFloat on out-of-range values with safety off is UB; add explicit range checks to avoid relying on undefined behavior.
pub fn canBeStrictInt32(value: f64) bool { - if (std.math.isInf(value) or std.math.isNan(value)) { + if (std.math.isInf(value) or std.math.isNan(value)) { return false; - } + } + if (value < @as(f64, @floatFromInt(std.math.minInt(i32))) or + value > @as(f64, @floatFromInt(std.math.maxInt(i32)))) { + return false; + } const int: i32 = int: { @setRuntimeSafety(false); break :int @intFromFloat(value); }; return !(@as(f64, @floatFromInt(int)) != value or (int == 0 and std.math.signbit(value))); // true for -0.0 }src/bun.js/bindings/BunProcess.cpp (1)
1886-1886: Minor: drop redundant JSC:: qualifier for consistencyYou already have “using namespace JSC;” — prefer unqualified jsNumber consistently within these blocks.
Apply:
- heap->putDirect(vm, JSC::Identifier::fromString(vm, "totalMemory"_s), JSC::jsNumber(WTF::ramSize()), 0); + heap->putDirect(vm, JSC::Identifier::fromString(vm, "totalMemory"_s), jsNumber(WTF::ramSize()), 0); @@ - heap->putDirect(vm, JSC::Identifier::fromString(vm, "externalMemory"_s), JSC::jsNumber(vm.heap.externalMemorySize()), 0); + heap->putDirect(vm, JSC::Identifier::fromString(vm, "externalMemory"_s), jsNumber(vm.heap.externalMemorySize()), 0); @@ - result->putDirectOffset(vm, 0, JSC::jsNumber(current_rss)); + result->putDirectOffset(vm, 0, jsNumber(current_rss)); - result->putDirectOffset(vm, 1, JSC::jsNumber(vm.heap.blockBytesAllocated())); + result->putDirectOffset(vm, 1, jsNumber(vm.heap.blockBytesAllocated())); @@ - result->putDirectOffset(vm, 2, JSC::jsNumber(vm.heap.sizeAfterLastEdenCollection())); + result->putDirectOffset(vm, 2, jsNumber(vm.heap.sizeAfterLastEdenCollection())); @@ - result->putDirectOffset(vm, 3, JSC::jsNumber(vm.heap.extraMemorySize() + vm.heap.externalMemorySize())); + result->putDirectOffset(vm, 3, jsNumber(vm.heap.extraMemorySize() + vm.heap.externalMemorySize())); @@ - result->putDirectOffset(vm, 4, JSC::jsNumber(vm.heap.arrayBufferSize())); + result->putDirectOffset(vm, 4, jsNumber(vm.heap.arrayBufferSize()));Also applies to: 1895-1895, 3204-3205, 3210-3210, 3212-3212, 3225-3225
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (12)
src/bun.js/bindings/BunProcess.cpp(9 hunks)src/bun.js/bindings/JSValue.zig(2 hunks)src/bun.js/bindings/NodeFSStatBinding.cpp(1 hunks)src/bun.js/bindings/SQLClient.cpp(1 hunks)src/bun.js/bindings/bindings.cpp(1 hunks)src/bun.js/bindings/headers.h(0 hunks)src/bun.js/bindings/sqlite/JSSQLStatement.cpp(2 hunks)src/bun.js/bindings/v8/V8Data.h(1 hunks)src/bun.js/bindings/webcore/JSPerformance.cpp(2 hunks)src/bun.js/modules/BunJSCModule.h(2 hunks)src/bun.js/modules/NodeBufferModule.h(1 hunks)src/valkey/valkey_protocol.zig(1 hunks)
💤 Files with no reviewable changes (1)
- src/bun.js/bindings/headers.h
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{cpp,h}
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.{cpp,h}: When exposing a JS class with public Constructor and Prototype, define three C++ types: class Foo : public JSC::DestructibleObject (if it has C++ fields), class FooPrototype : public JSC::JSNonFinalObject, and class FooConstructor : public JSC::InternalFunction
If the class has C++ data members, inherit from JSC::DestructibleObject and provide proper destruction; if it has no C++ fields (only JS properties), avoid a class and use JSC::constructEmptyObject(vm, structure) with putDirectOffset
Prefer placing the subspaceFor implementation in the .cpp file rather than the header when possible
Files:
src/bun.js/bindings/SQLClient.cppsrc/bun.js/bindings/v8/V8Data.hsrc/bun.js/modules/NodeBufferModule.hsrc/bun.js/modules/BunJSCModule.hsrc/bun.js/bindings/NodeFSStatBinding.cppsrc/bun.js/bindings/sqlite/JSSQLStatement.cppsrc/bun.js/bindings/webcore/JSPerformance.cppsrc/bun.js/bindings/BunProcess.cppsrc/bun.js/bindings/bindings.cpp
**/*.cpp
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.cpp: Include "root.h" at the top of C++ binding files to satisfy lints
Define prototype properties using a const HashTableValue array and declare accessors/functions with JSC_DECLARE_* macros
Prototype classes should subclass JSC::JSNonFinalObject, provide create/createStructure, DECLARE_INFO, finishCreation that reifies static properties, and set mayBePrototype on the Structure
Custom getters should use JSC_DEFINE_CUSTOM_GETTER, jsDynamicCast to validate this, and throwThisTypeError on mismatch
Custom setters should use JSC_DEFINE_CUSTOM_SETTER, validate this via jsDynamicCast, and store via WriteBarrier/set semantics
Prototype functions should use JSC_DEFINE_HOST_FUNCTION, validate this with jsDynamicCast, and return encoded JSValue
Constructors should subclass JSC::InternalFunction, return internalFunctionSpace in subspaceFor, set the prototype property as non-configurable/non-writable, and provide create/createStructure
Provide a setup function that builds the Prototype, Constructor, and Structure, and assigns them to the LazyClassStructure initializer
Use the cached Structure via globalObject->m_.get(globalObject) when constructing instances
Expose constructors to Zig via an extern "C" function that returns the constructor from the LazyClassStructure
Provide an extern "C" Bun____toJS function that creates an instance using the cached Structure and returns an EncodedJSValue
Files:
src/bun.js/bindings/SQLClient.cppsrc/bun.js/bindings/NodeFSStatBinding.cppsrc/bun.js/bindings/sqlite/JSSQLStatement.cppsrc/bun.js/bindings/webcore/JSPerformance.cppsrc/bun.js/bindings/BunProcess.cppsrc/bun.js/bindings/bindings.cpp
src/bun.js/bindings/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
src/bun.js/bindings/**/*.cpp: When implementing a JS class with a public constructor in C++ bindings, create Foo, FooPrototype, and FooConstructor classes
Define JS-visible properties using HashTableValue arrays
Add iso subspaces for classes with C++ fields
Cache class structures in ZigGlobalObject for bindings
Files:
src/bun.js/bindings/SQLClient.cppsrc/bun.js/bindings/NodeFSStatBinding.cppsrc/bun.js/bindings/sqlite/JSSQLStatement.cppsrc/bun.js/bindings/webcore/JSPerformance.cppsrc/bun.js/bindings/BunProcess.cppsrc/bun.js/bindings/bindings.cpp
**/*.{cpp,hpp,h,cc,cxx}
📄 CodeRabbit inference engine (CLAUDE.md)
Run clang-format to format C/C++ files (bun run clang-format)
Files:
src/bun.js/bindings/SQLClient.cppsrc/bun.js/bindings/v8/V8Data.hsrc/bun.js/modules/NodeBufferModule.hsrc/bun.js/modules/BunJSCModule.hsrc/bun.js/bindings/NodeFSStatBinding.cppsrc/bun.js/bindings/sqlite/JSSQLStatement.cppsrc/bun.js/bindings/webcore/JSPerformance.cppsrc/bun.js/bindings/BunProcess.cppsrc/bun.js/bindings/bindings.cpp
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
Implement debug logs in Zig using
const log = bun.Output.scoped(.${SCOPE}, false);and invokinglog("...", .{})In Zig code, manage memory carefully: use appropriate allocators and defer for cleanup
Files:
src/valkey/valkey_protocol.zigsrc/bun.js/bindings/JSValue.zig
**/*.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.JSValueRun zig-format to format Zig files (bun run zig-format)
Files:
src/valkey/valkey_protocol.zigsrc/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
🧠 Learnings (19)
📓 Common learnings
Learnt from: markovejnovic
PR: oven-sh/bun#21728
File: src/valkey/js_valkey_functions.zig:852-867
Timestamp: 2025-09-05T19:49:26.188Z
Learning: In Bun’s Zig code, `.js_undefined` is a valid and preferred JSValue literal for “undefined” (e.g., resolving JSPromise). Do not refactor usages to `jsc.JSValue.jsUndefined()`, especially in src/valkey/js_valkey_functions.zig unsubscribe().
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
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
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
📚 Learning: 2025-09-03T17:09:28.113Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-03T17:09:28.113Z
Learning: Applies to src/bun.js/bindings/**/*.cpp : Define JS-visible properties using HashTableValue arrays
Applied to files:
src/bun.js/bindings/v8/V8Data.hsrc/bun.js/modules/BunJSCModule.hsrc/bun.js/bindings/NodeFSStatBinding.cppsrc/bun.js/bindings/BunProcess.cppsrc/bun.js/bindings/bindings.cpp
📚 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 **/*.cpp : Prototype functions should use JSC_DEFINE_HOST_FUNCTION, validate this with jsDynamicCast, and return encoded JSValue
Applied to files:
src/bun.js/bindings/v8/V8Data.hsrc/bun.js/bindings/BunProcess.cppsrc/bun.js/bindings/bindings.cpp
📚 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 : Implement getters as get<PropertyName>(this, globalObject) returning JSC.JSValue and matching the .classes.ts interface
Applied to files:
src/bun.js/modules/NodeBufferModule.hsrc/bun.js/bindings/bindings.cpp
📚 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/modules/BunJSCModule.hsrc/bun.js/bindings/bindings.cppsrc/bun.js/bindings/JSValue.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/modules/BunJSCModule.hsrc/bun.js/bindings/bindings.cppsrc/bun.js/bindings/JSValue.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 : 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/modules/BunJSCModule.hsrc/bun.js/bindings/bindings.cppsrc/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 **/*.cpp : Provide an extern "C" Bun__<Type>__toJS function that creates an instance using the cached Structure and returns an EncodedJSValue
Applied to files:
src/bun.js/modules/BunJSCModule.h
📚 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/modules/BunJSCModule.hsrc/bun.js/bindings/NodeFSStatBinding.cppsrc/bun.js/bindings/bindings.cpp
📚 Learning: 2025-09-05T18:45:29.200Z
Learnt from: markovejnovic
PR: oven-sh/bun#21728
File: src/valkey/js_valkey.zig:0-0
Timestamp: 2025-09-05T18:45:29.200Z
Learning: In JSValkeyClient.cloneWithoutConnecting() in src/valkey/js_valkey.zig, the address/username/password fields must be repointed to the duplicated connection_strings buffer to avoid use-after-free when the original client is destroyed. The original client properly frees connection_strings in ValkeyClient.deinit().
Applied to files:
src/valkey/valkey_protocol.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/valkey/valkey_protocol.zigsrc/bun.js/bindings/bindings.cppsrc/bun.js/bindings/JSValue.zig
📚 Learning: 2025-09-03T17:09:28.113Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-03T17:09:28.113Z
Learning: Applies to src/bun.js/bindings/**/*.cpp : Add iso subspaces for classes with C++ fields
Applied to files:
src/bun.js/bindings/NodeFSStatBinding.cpp
📚 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 **/*.{cpp,h} : If the class has C++ data members, inherit from JSC::DestructibleObject and provide proper destruction; if it has no C++ fields (only JS properties), avoid a class and use JSC::constructEmptyObject(vm, structure) with putDirectOffset
Applied to files:
src/bun.js/bindings/NodeFSStatBinding.cppsrc/bun.js/bindings/BunProcess.cpp
📚 Learning: 2025-08-31T09:08:12.104Z
Learnt from: Jarred-Sumner
PR: oven-sh/bun#22279
File: test/js/web/structured-clone-fastpath.test.ts:12-12
Timestamp: 2025-08-31T09:08:12.104Z
Learning: In Bun, process.memoryUsage.rss() is called as a direct function, not as process.memoryUsage().rss like in Node.js. This is the correct API shape for Bun.
Applied to files:
src/bun.js/bindings/BunProcess.cpp
📚 Learning: 2025-09-05T19:49:26.188Z
Learnt from: markovejnovic
PR: oven-sh/bun#21728
File: src/valkey/js_valkey_functions.zig:852-867
Timestamp: 2025-09-05T19:49:26.188Z
Learning: In Bun’s Zig code, `.js_undefined` is a valid and preferred JSValue literal for “undefined” (e.g., resolving JSPromise). Do not refactor usages to `jsc.JSValue.jsUndefined()`, especially in src/valkey/js_valkey_functions.zig unsubscribe().
Applied to files:
src/bun.js/bindings/bindings.cppsrc/bun.js/bindings/JSValue.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/bindings/generated_classes_list.zig : Update src/bun.js/bindings/generated_classes_list.zig to include new classes
Applied to files:
src/bun.js/bindings/bindings.cppsrc/bun.js/bindings/JSValue.zig
📚 Learning: 2025-09-05T19:49:26.188Z
Learnt from: markovejnovic
PR: oven-sh/bun#21728
File: src/valkey/js_valkey_functions.zig:852-867
Timestamp: 2025-09-05T19:49:26.188Z
Learning: In Bun's Zig codebase, `.js_undefined` is a valid way to represent JavaScript's undefined value when working with JSPromise.resolve() and similar JavaScript interop functions. This is the correct pattern to use rather than `jsc.JSValue.jsUndefined()`.
Applied to files:
src/bun.js/bindings/bindings.cpp
📚 Learning: 2025-09-04T02:04:43.094Z
Learnt from: Jarred-Sumner
PR: oven-sh/bun#22278
File: src/ast/E.zig:980-1003
Timestamp: 2025-09-04T02:04:43.094Z
Learning: In Bun's Zig codebase, `as(i32, u8_or_u16_value)` is sufficient for casting u8/u16 to i32 in comparison operations. `intCast` is not required in this context, and the current casting approach compiles successfully.
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 : Handle reference counting correctly with ref()/deref() in JS-facing Zig code
Applied to files:
src/bun.js/bindings/JSValue.zig
🧬 Code graph analysis (1)
src/bun.js/bindings/webcore/JSPerformance.cpp (1)
src/bun.js/bindings/BunProcess.cpp (1)
Bun__readOriginTimerStart(691-691)
⏰ 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 (20)
src/bun.js/modules/NodeBufferModule.h (1)
143-144: LGTM: switch to jsNumber.Matches project-wide encoding policy; preserves -0 and int32 fast path via jsNumber.
src/bun.js/modules/BunJSCModule.h (3)
346-347: LGTM: zoneSizes encoded via jsNumber.Consistent with unified number encoding; preserves int32 fast path.
885-889: LGTM: estimateDirectMemoryUsage encoded via jsNumber.Aligns with policy; keeps integers as int32 when possible.
895-898: Consider jsNumber for consistency (or add a comment why double is required).percentAvailableMemoryInUse currently uses jsDoubleNumber; unless callers rely on “isDouble” identity, jsNumber would be consistent with the rest of this PR.
- return JSValue::encode(jsDoubleNumber(bmalloc::api::percentAvailableMemoryInUse())); + return JSValue::encode(jsNumber(bmalloc::api::percentAvailableMemoryInUse()));If double identity is required, keep as-is and add a short comment.
src/valkey/valkey_protocol.zig (1)
252-253: LGTM: return ArrayBuffer JSValue directly.Removes an extra toJS hop; API still returns a JS ArrayBuffer. Keep the >4.7 GB TODO.
src/bun.js/bindings/webcore/JSPerformance.cpp (2)
122-125: LGTM: explicitly return double for now().Matches DOMJIT SpecDoubleReal; keeping jsDoubleNumber here is correct.
287-289: Should timeOrigin also force double?Spec type is DOMHighResTimeStamp (double). Using jsNumber may produce int32-tagged values when integral. If any code/tests depend on double identity, consider jsDoubleNumber or add a comment confirming int32 tagging is acceptable here.
- jsNumber(Bun__readOriginTimerStart(reinterpret_cast<Zig::GlobalObject*>(this->globalObject())->bunVM())), + jsDoubleNumber(Bun__readOriginTimerStart(reinterpret_cast<Zig::GlobalObject*>(this->globalObject())->bunVM())),src/bun.js/bindings/JSValue.zig (1)
587-592: LGTM: int32-fast-path for f32/f64 with -0.0 preservation.canBeStrictInt32 prevents collapsing -0.0 and only int-tags exact int32 values; this is the desired behavior.
src/bun.js/bindings/SQLClient.cpp (1)
155-156: Good switch to jsNumber.This aligns with the PR objective and enables int32 tagging for integer-valued doubles where safe.
src/bun.js/bindings/NodeFSStatBinding.cpp (1)
607-621: Consistent numeric encoding.Replacing all stat fields with jsNumber is correct and matches the new JSValue policy.
src/bun.js/bindings/sqlite/JSSQLStatement.cpp (2)
160-164: Int32 fast-path preserved, double fallback when needed.The branch cleanly prefers int32 tagging when within range; otherwise emits a double. Looks good.
564-565: Float path aligned with jsNumber.Matches the project-wide migration and allows future int32 optimizations when values are integer-valued.
src/bun.js/bindings/bindings.cpp (1)
4300-4304: Approve changes: jsTDZValue export retained
Verification searches forJSC__JSValue__jsNull|UndefinedandjsDoubleNumber(returned no hits—no residual references remain.src/bun.js/bindings/BunProcess.cpp (7)
1647-1647: Exit code normalization via jsNumber looks correctValue is still validated/integral before modulo 256; no behavior change.
1726-1726: RLIMIT soft/hard now encoded with jsNumber — OKKeeps "unlimited" as string; numeric paths benefit from int32 fast-path when applicable.
Also applies to: 1728-1728
1886-1886: Report heap totals switched to jsNumber — OKConsistent with the PR’s objective; large values will encode as double.
Also applies to: 1895-1895
2757-2757: availableMemory: jsNumber adoption — OKMatches other numeric bindings.
2963-2963: constrainedMemory: jsNumber adoption — OKNo semantic change.
3087-3088: cpuUsage fields via jsNumber — OKprevValue validation uses toNumber(), so int32/double mix won’t break callers.
3204-3205: memoryUsage metrics via jsNumber — OKConsistent with the PR; large size_t values will encode as double when needed.
Also applies to: 3210-3210, 3212-3212, 3225-3225
3 participants
What does this PR do?
Replaces usages of
jsDoubleNumberwithjsNumberin places where the value is likely to be either a double or strict int32.jsNumberwill decide to useNumberTagorEncodeAsDouble.If the number is used in a lot of arithmetic this could boost performance (related #18585).
How did you verify your code works?
CI