ffi: reuse the callable created per symbol

doc/api/ffi.md

@@ -363,7 +363,8 @@ The returned function has a `.pointer` property containing the native function
 address as a `bigint`.
 
 If the same symbol has already been resolved, requesting it again with a
-different signature throws.
+different signature throws. Requesting it again with the same signature returns
+the same function, as does reading it from [`library.functions`][].
 
 ```cjs
 const { DynamicLibrary, suffix } = require('node:ffi');
@@ -766,5 +767,6 @@ and keep callback and pointer lifetimes explicit on the native side.
 [Permission Model]: permissions.md#permission-model
 [`--allow-ffi`]: cli.md#--allow-ffi
 [`ffi.toBuffer(pointer, length, copy)`]: #ffitobufferpointer-length-copy
+[`library.functions`]: #libraryfunctions
 [`using`]: https://tc39.es/proposal-explicit-resource-management/#sec-using-declarations
 [type names]: #type-names

lib/ffi.js

@@ -7,6 +7,8 @@ const {
   ObjectGetOwnPropertyDescriptor,
   ObjectKeys,
   ObjectPrototypeToString,
+  SafeWeakMap,
+  SafeWeakRef,
   SymbolDispose,
 } = primordials;
 const { Buffer } = require('buffer');
@@ -80,23 +82,36 @@ function makeSignature(argumentTypes, returnType) {
   };
 }
 
+// The native layer hands out one raw function per resolved symbol, so the
+// wrapper composed around it is reused too, otherwise every read of
+// `library.functions` would return callables that are not identical to the
+// previous read's. The entry holds a WeakRef because V8 can keep a raw function
+// alive after user code drops the wrapper, and a strong value would then pin
+// every wrapper for the lifetime of the library.
+const wrappedFunctions = new SafeWeakMap();
+
 function wrapFFIFunction(rawFn, owner) {
-  let argumentTypes;
+  if (rawFn === undefined || rawFn === null) {
+    return rawFn;
+  }
+  const cached = wrappedFunctions.get(rawFn)?.deref();
+  if (cached !== undefined) {
+    return cached;
+  }
   let returnType;
-  if (rawFn !== undefined && rawFn !== null) {
-    const sbArguments = rawFn[kSbArguments];
-    argumentTypes = sbArguments ?? rawFn[kFastArguments];
-    if (sbArguments !== undefined) {
-      returnType = rawFn[kSbReturn];
-    }
+  const sbArguments = rawFn[kSbArguments];
+  const argumentTypes = sbArguments ?? rawFn[kFastArguments];
+  if (sbArguments !== undefined) {
+    returnType = rawFn[kSbReturn];
   }
-  const wrapped = wrapWithSharedBuffer(
+  let wrapped = wrapWithSharedBuffer(
     rawFn,
     argumentTypes === undefined ? undefined : makeSignature(argumentTypes, returnType));
-  if (wrapped !== rawFn) {
-    return wrapped;
+  if (wrapped === rawFn) {
+    wrapped = wrapWithRawPointerConversions(rawFn, argumentTypes, owner);
   }
-  return wrapWithRawPointerConversions(rawFn, argumentTypes, owner);
+  wrappedFunctions.set(rawFn, new SafeWeakRef(wrapped));
+  return wrapped;
 }
 
 const rawGetFunction = DynamicLibrary.prototype.getFunction;

src/node_ffi.cc

@@ -72,6 +72,12 @@ void DynamicLibrary::MemoryInfo(MemoryTracker* tracker) const {
   tracker->TrackFieldWithSize(
       "symbols", symbols_size, "std::unordered_map<std::string, void*>");
 
+  tracker->TrackFieldWithSize(
+      "function_wrappers",
+      function_wrappers_.size() *
+          sizeof(decltype(function_wrappers_)::value_type),
+      "std::unordered_map<std::string, v8::Global<v8::Function>>");
+
   // FFIFunctionInfo instances and their sb_backing ArrayBuffers are
   // owned by V8 function wrappers and reachable only via weak references,
   // so they are deliberately not counted here.
@@ -97,6 +103,7 @@ void DynamicLibrary::Close() {
 
   symbols_.clear();
   functions_.clear();
+  function_wrappers_.clear();
   callbacks_.clear();
 }
 
@@ -242,6 +249,19 @@ MaybeLocal<Function> DynamicLibrary::CreateFunction(
   Isolate* isolate = env->isolate();
   Local<Context> context = env->context();
 
+  // Creating a callable emits a trampoline, allocates an FFIFunctionInfo, and
+  // on the SharedBuffer path allocates an ArrayBuffer, so reuse the one already
+  // handed out for this symbol. `PrepareFunction()` rejects a request that uses
+  // a different signature, so a hit always describes the same signature. An
+  // empty handle means the wrapper was collected; fall through and rebuild.
+  auto cached = function_wrappers_.find(name);
+  if (cached != function_wrappers_.end()) {
+    if (!cached->second.IsEmpty()) {
+      return cached->second.Get(isolate);
+    }
+    function_wrappers_.erase(cached);
+  }
+
   auto info = FFIFunctionInfo::Create(env, fn, this);
 
   DCHECK_EQ(fn->args.size(), fn->arg_type_names.size());
@@ -437,6 +457,14 @@ MaybeLocal<Function> DynamicLibrary::CreateFunction(
     }
   }
 
+  // A strong handle would root the callable, which holds the library object
+  // through FFIFunctionInfo, so neither could ever be collected. Weaken the
+  // stored handle instead, so the cache lasts exactly as long as user code
+  // keeps a reference. SetWeak() runs after the move into the map because
+  // moving a handle relocates the underlying slot.
+  function_wrappers_.emplace(name, Global<Function>(isolate, ret))
+      .first->second.SetWeak();
+
   return ret;
 }
 

src/node_ffi.h

@@ -148,6 +148,12 @@ class DynamicLibrary : public BaseObject {
   std::string path_;
   std::unordered_map<std::string, void*> symbols_;
   std::unordered_map<std::string, std::shared_ptr<FFIFunction>> functions_;
+  // Callables created for `functions_`, so repeated resolution of the same
+  // symbol reuses one wrapper instead of emitting another trampoline. The
+  // handles are weak: an entry disappears once user code drops the wrapper,
+  // which keeps the map from rooting the library through the wrapper's
+  // FFIFunctionInfo.
+  std::unordered_map<std::string, v8::Global<v8::Function>> function_wrappers_;
   std::unordered_map<void*, std::unique_ptr<FFICallback>> callbacks_;
 };
 

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

@@ -176,6 +176,46 @@ test('getFunction caches signatures consistently', () => {
   }
 });
 
+test('resolving the same symbol reuses one function', () => {
+  const lib = new ffi.DynamicLibrary(libraryPath);
+  const definitions = { add_i32: fixtureSymbols.add_i32 };
+
+  try {
+    // Every resolution used to build a new callable, allocating another
+    // trampoline and making `lib.functions.add_i32` a different function on
+    // each read.
+    const fn = lib.getFunction('add_i32', fixtureSymbols.add_i32);
+    assert.strictEqual(lib.getFunction('add_i32', fixtureSymbols.add_i32), fn);
+    assert.strictEqual(lib.functions.add_i32, fn);
+    assert.strictEqual(lib.getFunctions().add_i32, fn);
+    assert.strictEqual(lib.getFunctions(definitions).add_i32, fn);
+    assert.strictEqual(fn(20, 22), 42);
+  } finally {
+    lib.close();
+  }
+});
+
+test('a dropped function wrapper is collectable', async () => {
+  const lib = new ffi.DynamicLibrary(libraryPath);
+
+  try {
+    // Caching the wrapper must not pin it, so that dropping the last user
+    // reference still releases the wrapper and the trampoline it owns.
+    let fn = lib.getFunction('add_i32', fixtureSymbols.add_i32);
+    const ref = new WeakRef(fn);
+    fn = null;
+
+    await gcUntil('a dropped function wrapper is collectable', () => {
+      return ref.deref() === undefined;
+    });
+
+    fn = lib.getFunction('add_i32', fixtureSymbols.add_i32);
+    assert.strictEqual(fn(20, 22), 42);
+  } finally {
+    lib.close();
+  }
+});
+
 test('FFI functions keep their owning library alive', async () => {
   let lib = new ffi.DynamicLibrary(libraryPath);
   const addI32 = lib.getFunction('add_i32', fixtureSymbols.add_i32);