Skip to content

Commit 16a5d6e

Browse files
committed
ffi: reuse the callable created per symbol
CreateFunction() ran on every getFunction() call, every getFunctions() call, and every read of the functions accessor, each time emitting a trampoline, allocating an FFIFunctionInfo, and on the SharedBuffer path an ArrayBuffer. lib.functions.foo was therefore a different function on each read, and calling through the accessor in a loop leaked a page per iteration until GC: 20000 calls grew RSS by 58 MiB. Cache the created callable per symbol in function_wrappers_, and memoize the JS wrapper composed around it. Both entries are weak, so dropping the last user reference still releases the wrapper and its trampoline. The JS side stores a WeakRef because V8 can keep a raw function alive after the wrapper is gone, and a strong value would pin every wrapper for the lifetime of the library. Signed-off-by: Trivikram Kamat <16024985+trivikr@users.noreply.github.com> Assisted-by: claude:opus-5
1 parent f43086d commit 16a5d6e

5 files changed

Lines changed: 103 additions & 12 deletions

File tree

doc/api/ffi.md

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -363,7 +363,8 @@ The returned function has a `.pointer` property containing the native function
363363
address as a `bigint`.
364364

365365
If the same symbol has already been resolved, requesting it again with a
366-
different signature throws.
366+
different signature throws. Requesting it again with the same signature returns
367+
the same function, as does reading it from [`library.functions`][].
367368

368369
```cjs
369370
const { DynamicLibrary, suffix } = require('node:ffi');
@@ -766,5 +767,6 @@ and keep callback and pointer lifetimes explicit on the native side.
766767
[Permission Model]: permissions.md#permission-model
767768
[`--allow-ffi`]: cli.md#--allow-ffi
768769
[`ffi.toBuffer(pointer, length, copy)`]: #ffitobufferpointer-length-copy
770+
[`library.functions`]: #libraryfunctions
769771
[`using`]: https://tc39.es/proposal-explicit-resource-management/#sec-using-declarations
770772
[type names]: #type-names

lib/ffi.js

Lines changed: 26 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,8 @@ const {
77
ObjectGetOwnPropertyDescriptor,
88
ObjectKeys,
99
ObjectPrototypeToString,
10+
SafeWeakMap,
11+
SafeWeakRef,
1012
SymbolDispose,
1113
} = primordials;
1214
const { Buffer } = require('buffer');
@@ -80,23 +82,36 @@ function makeSignature(argumentTypes, returnType) {
8082
};
8183
}
8284

85+
// The native layer hands out one raw function per resolved symbol, so the
86+
// wrapper composed around it is reused too, otherwise every read of
87+
// `library.functions` would return callables that are not identical to the
88+
// previous read's. The entry holds a WeakRef because V8 can keep a raw function
89+
// alive after user code drops the wrapper, and a strong value would then pin
90+
// every wrapper for the lifetime of the library.
91+
const wrappedFunctions = new SafeWeakMap();
92+
8393
function wrapFFIFunction(rawFn, owner) {
84-
let argumentTypes;
94+
if (rawFn === undefined || rawFn === null) {
95+
return rawFn;
96+
}
97+
const cached = wrappedFunctions.get(rawFn)?.deref();
98+
if (cached !== undefined) {
99+
return cached;
100+
}
85101
let returnType;
86-
if (rawFn !== undefined && rawFn !== null) {
87-
const sbArguments = rawFn[kSbArguments];
88-
argumentTypes = sbArguments ?? rawFn[kFastArguments];
89-
if (sbArguments !== undefined) {
90-
returnType = rawFn[kSbReturn];
91-
}
102+
const sbArguments = rawFn[kSbArguments];
103+
const argumentTypes = sbArguments ?? rawFn[kFastArguments];
104+
if (sbArguments !== undefined) {
105+
returnType = rawFn[kSbReturn];
92106
}
93-
const wrapped = wrapWithSharedBuffer(
107+
let wrapped = wrapWithSharedBuffer(
94108
rawFn,
95109
argumentTypes === undefined ? undefined : makeSignature(argumentTypes, returnType));
96-
if (wrapped !== rawFn) {
97-
return wrapped;
110+
if (wrapped === rawFn) {
111+
wrapped = wrapWithRawPointerConversions(rawFn, argumentTypes, owner);
98112
}
99-
return wrapWithRawPointerConversions(rawFn, argumentTypes, owner);
113+
wrappedFunctions.set(rawFn, new SafeWeakRef(wrapped));
114+
return wrapped;
100115
}
101116

102117
const rawGetFunction = DynamicLibrary.prototype.getFunction;

src/node_ffi.cc

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,12 @@ void DynamicLibrary::MemoryInfo(MemoryTracker* tracker) const {
7272
tracker->TrackFieldWithSize(
7373
"symbols", symbols_size, "std::unordered_map<std::string, void*>");
7474

75+
tracker->TrackFieldWithSize(
76+
"function_wrappers",
77+
function_wrappers_.size() *
78+
sizeof(decltype(function_wrappers_)::value_type),
79+
"std::unordered_map<std::string, v8::Global<v8::Function>>");
80+
7581
// FFIFunctionInfo instances and their sb_backing ArrayBuffers are
7682
// owned by V8 function wrappers and reachable only via weak references,
7783
// so they are deliberately not counted here.
@@ -97,6 +103,7 @@ void DynamicLibrary::Close() {
97103

98104
symbols_.clear();
99105
functions_.clear();
106+
function_wrappers_.clear();
100107
callbacks_.clear();
101108
}
102109

@@ -242,6 +249,19 @@ MaybeLocal<Function> DynamicLibrary::CreateFunction(
242249
Isolate* isolate = env->isolate();
243250
Local<Context> context = env->context();
244251

252+
// Creating a callable emits a trampoline, allocates an FFIFunctionInfo, and
253+
// on the SharedBuffer path allocates an ArrayBuffer, so reuse the one already
254+
// handed out for this symbol. `PrepareFunction()` rejects a request that uses
255+
// a different signature, so a hit always describes the same signature. An
256+
// empty handle means the wrapper was collected; fall through and rebuild.
257+
auto cached = function_wrappers_.find(name);
258+
if (cached != function_wrappers_.end()) {
259+
if (!cached->second.IsEmpty()) {
260+
return cached->second.Get(isolate);
261+
}
262+
function_wrappers_.erase(cached);
263+
}
264+
245265
auto info = FFIFunctionInfo::Create(env, fn, this);
246266

247267
DCHECK_EQ(fn->args.size(), fn->arg_type_names.size());
@@ -437,6 +457,14 @@ MaybeLocal<Function> DynamicLibrary::CreateFunction(
437457
}
438458
}
439459

460+
// A strong handle would root the callable, which holds the library object
461+
// through FFIFunctionInfo, so neither could ever be collected. Weaken the
462+
// stored handle instead, so the cache lasts exactly as long as user code
463+
// keeps a reference. SetWeak() runs after the move into the map because
464+
// moving a handle relocates the underlying slot.
465+
function_wrappers_.emplace(name, Global<Function>(isolate, ret))
466+
.first->second.SetWeak();
467+
440468
return ret;
441469
}
442470

src/node_ffi.h

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -148,6 +148,12 @@ class DynamicLibrary : public BaseObject {
148148
std::string path_;
149149
std::unordered_map<std::string, void*> symbols_;
150150
std::unordered_map<std::string, std::shared_ptr<FFIFunction>> functions_;
151+
// Callables created for `functions_`, so repeated resolution of the same
152+
// symbol reuses one wrapper instead of emitting another trampoline. The
153+
// handles are weak: an entry disappears once user code drops the wrapper,
154+
// which keeps the map from rooting the library through the wrapper's
155+
// FFIFunctionInfo.
156+
std::unordered_map<std::string, v8::Global<v8::Function>> function_wrappers_;
151157
std::unordered_map<void*, std::unique_ptr<FFICallback>> callbacks_;
152158
};
153159

test/ffi/test-ffi-dynamic-library.js

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -176,6 +176,46 @@ test('getFunction caches signatures consistently', () => {
176176
}
177177
});
178178

179+
test('resolving the same symbol reuses one function', () => {
180+
const lib = new ffi.DynamicLibrary(libraryPath);
181+
const definitions = { add_i32: fixtureSymbols.add_i32 };
182+
183+
try {
184+
// Every resolution used to build a new callable, allocating another
185+
// trampoline and making `lib.functions.add_i32` a different function on
186+
// each read.
187+
const fn = lib.getFunction('add_i32', fixtureSymbols.add_i32);
188+
assert.strictEqual(lib.getFunction('add_i32', fixtureSymbols.add_i32), fn);
189+
assert.strictEqual(lib.functions.add_i32, fn);
190+
assert.strictEqual(lib.getFunctions().add_i32, fn);
191+
assert.strictEqual(lib.getFunctions(definitions).add_i32, fn);
192+
assert.strictEqual(fn(20, 22), 42);
193+
} finally {
194+
lib.close();
195+
}
196+
});
197+
198+
test('a dropped function wrapper is collectable', async () => {
199+
const lib = new ffi.DynamicLibrary(libraryPath);
200+
201+
try {
202+
// Caching the wrapper must not pin it, so that dropping the last user
203+
// reference still releases the wrapper and the trampoline it owns.
204+
let fn = lib.getFunction('add_i32', fixtureSymbols.add_i32);
205+
const ref = new WeakRef(fn);
206+
fn = null;
207+
208+
await gcUntil('a dropped function wrapper is collectable', () => {
209+
return ref.deref() === undefined;
210+
});
211+
212+
fn = lib.getFunction('add_i32', fixtureSymbols.add_i32);
213+
assert.strictEqual(fn(20, 22), 42);
214+
} finally {
215+
lib.close();
216+
}
217+
});
218+
179219
test('FFI functions keep their owning library alive', async () => {
180220
let lib = new ffi.DynamicLibrary(libraryPath);
181221
const addI32 = lib.getFunction('add_i32', fixtureSymbols.add_i32);

0 commit comments

Comments
 (0)