GeezerVison is showing github.com — ffi: fix optimized buffer conversions · nodejs/node@48f4cfb
File treeExpand file treeCollapse file tree Open diff view settings Expand file treeCollapse file tree Open diff view settings Collapse file Copy file name to clipboardExpand all lines: lib/ffi.js -2Lines changed: 0 additions & 2 deletions | Original file line number | Diff line number | Diff line change |
|---|
@@ -68,7 +68,6 @@ const { | 68 | 68 | } = require('internal/ffi-shared-buffer'); | 69 | 69 |
| 70 | 70 | const { | 71 | | - initializeFastBufferMetadata, | 72 | 71 | wrapWithRawPointerConversions, | 73 | 72 | } = require('internal/ffi/fast-api'); | 74 | 73 |
| @@ -90,7 +89,6 @@ function wrapFFIFunction(rawFn, owner) { | 90 | 89 | returnType = rawFn[kSbReturn]; | 91 | 90 | } | 92 | 91 | } | 93 | | - initializeFastBufferMetadata(rawFn, argumentTypes); | 94 | 92 | const wrapped = wrapWithSharedBuffer( | 95 | 93 | rawFn, | 96 | 94 | argumentTypes === undefined ? undefined : makeSignature(argumentTypes, returnType)); | |
Collapse file Copy file name to clipboardExpand all lines: lib/internal/ffi/fast-api.js +16-49Lines changed: 16 additions & 49 deletions | Original file line number | Diff line number | Diff line change |
|---|
| 6 | 6 | ObjectDefineProperty, | 7 | 7 | ReflectApply, | 8 | 8 | StringPrototypeIncludes, | 9 | | - Symbol, | 10 | 9 | TypeError, | 11 | 10 | } = primordials; | 12 | 11 |
| @@ -24,11 +23,8 @@ const { | 24 | 23 | getRawPointer, | 25 | 24 | kFastArguments, | 26 | 25 | kFastBufferInvoke, | 27 | | - kSbSharedBuffer, | 28 | 26 | } = internalBinding('ffi'); | 29 | 27 |
| 30 | | -const kFastBuffer = Symbol('kFastBuffer'); | 31 | | - | 32 | 28 | const U64_MAX = 0xFFFFFFFFFFFFFFFFn; | 33 | 29 | const I64_MAX = 0x7FFFFFFFFFFFFFFFn; | 34 | 30 | const I64_MIN = -0x8000000000000000n; | @@ -79,16 +75,13 @@ function validateFastIntegerArg(type, value, index) { | 79 | 75 | } | 80 | 76 | } | 81 | 77 |
| 82 | | -function needsRawPointerConversion(type, rawFn) { | 83 | | - if (rawFn !== undefined && rawFn[kFastBuffer] === true && | 84 | | - (type === 'buffer' || type === 'arraybuffer')) { | 85 | | - return false; | 86 | | - } | | 78 | +function needsRawPointerConversion(type) { | 87 | 79 | return type === 'buffer' || type === 'arraybuffer'; | 88 | 80 | } | 89 | 81 |
| 90 | 82 | function needsPointerLikeConversion(type) { | 91 | | - return type === 'pointer' || type === 'ptr' || type === 'function'; | | 83 | + return type === 'pointer' || type === 'ptr' || type === 'function' || | | 84 | + type === 'buffer' || type === 'arraybuffer'; | 92 | 85 | } | 93 | 86 |
| 94 | 87 | function needsStringPointerConversion(type) { | @@ -100,12 +93,8 @@ function needsNullPointerConversion(type) { | 100 | 93 | needsRawPointerConversion(type); | 101 | 94 | } | 102 | 95 |
| 103 | | -function needsPointerConversion(type, rawFn) { | 104 | | - if (rawFn !== undefined && rawFn[kFastBuffer] === true && | 105 | | - (type === 'buffer' || type === 'arraybuffer')) { | 106 | | - return false; | 107 | | - } | 108 | | - return needsRawPointerConversion(type, rawFn) || | | 96 | +function needsPointerConversion(type) { | | 97 | + return needsRawPointerConversion(type) || | 109 | 98 | needsNullPointerConversion(type) || needsStringPointerConversion(type); | 110 | 99 | } | 111 | 100 |
| @@ -174,11 +163,11 @@ function convertPointerArg(type, value, stringState, index) { | 174 | 163 | return value; | 175 | 164 | } | 176 | 165 |
| 177 | | -function getFastArgumentIndexes(argumentsTypes, rawFn) { | | 166 | +function getFastArgumentIndexes(argumentsTypes) { | 178 | 167 | let indexes = null; | 179 | 168 | for (let i = 0; i < argumentsTypes.length; i++) { | 180 | 169 | if (fastIntegerTypeInfo[argumentsTypes[i]] === undefined && | 181 | | - !needsPointerConversion(argumentsTypes[i], rawFn)) { | | 170 | + !needsPointerConversion(argumentsTypes[i])) { | 182 | 171 | continue; | 183 | 172 | } | 184 | 173 | if (indexes === null) { | @@ -189,31 +178,12 @@ function getFastArgumentIndexes(argumentsTypes, rawFn) { | 189 | 178 | return indexes; | 190 | 179 | } | 191 | 180 |
| 192 | | -function convertFastArg(type, value, rawFn, stringState, index) { | | 181 | +function convertFastArg(type, value, stringState, index) { | 193 | 182 | validateFastIntegerArg(type, value, index); | 194 | | - return needsPointerConversion(type, rawFn) ? | | 183 | + return needsPointerConversion(type) ? | 195 | 184 | convertPointerArg(type, value, stringState, index) : value; | 196 | 185 | } | 197 | 186 |
| 198 | | -function initializeFastBufferMetadata(rawFn, argumentTypes) { | 199 | | - if (rawFn === undefined || rawFn === null || argumentTypes === undefined) { | 200 | | - return; | 201 | | - } | 202 | | - if (rawFn[kSbSharedBuffer] !== undefined) { | 203 | | - return; | 204 | | - } | 205 | | - | 206 | | - if (rawFn[kFastArguments] !== undefined) { | 207 | | - for (let i = 0; i < argumentTypes.length; i++) { | 208 | | - const type = argumentTypes[i]; | 209 | | - if (type === 'buffer' || type === 'arraybuffer') { | 210 | | - rawFn[kFastBuffer] = true; | 211 | | - break; | 212 | | - } | 213 | | - } | 214 | | - } | 215 | | -} | 216 | | - | 217 | 187 | function inheritMetadata(wrapper, rawFn, nargs) { | 218 | 188 | ObjectDefineProperty(wrapper, 'name', { | 219 | 189 | __proto__: null, value: rawFn.name, configurable: true, | @@ -239,7 +209,7 @@ function wrapWithRawPointerConversions(rawFn, argumentTypes, _owner) { | 239 | 209 | return rawFn; | 240 | 210 | } | 241 | 211 |
| 242 | | - const indexes = getFastArgumentIndexes(argumentTypes, rawFn); | | 212 | + const indexes = getFastArgumentIndexes(argumentTypes); | 243 | 213 | if (indexes === null) { | 244 | 214 | return rawFn; | 245 | 215 | } | @@ -295,9 +265,8 @@ function wrapWithRawPointerConversions(rawFn, argumentTypes, _owner) { | 295 | 265 | (c1 && hasStringPointerArg(t1, a1)); | 296 | 266 | if (stringCall) enterStringConversion(stringState); | 297 | 267 | try { | 298 | | - return rawFn(c0 ? | 299 | | - convertFastArg(t0, a0, rawFn, stringState, 0) : a0, | 300 | | - c1 ? convertFastArg(t1, a1, rawFn, stringState, 1) : a1); | | 268 | + return rawFn(c0 ? convertFastArg(t0, a0, stringState, 0) : a0, | | 269 | + c1 ? convertFastArg(t1, a1, stringState, 1) : a1); | 301 | 270 | } finally { | 302 | 271 | if (stringCall) exitStringConversion(stringState); | 303 | 272 | } | @@ -318,10 +287,9 @@ function wrapWithRawPointerConversions(rawFn, argumentTypes, _owner) { | 318 | 287 | (c2 && hasStringPointerArg(t2, a2)); | 319 | 288 | if (stringCall) enterStringConversion(stringState); | 320 | 289 | try { | 321 | | - return rawFn(c0 ? | 322 | | - convertFastArg(t0, a0, rawFn, stringState, 0) : a0, | 323 | | - c1 ? convertFastArg(t1, a1, rawFn, stringState, 1) : a1, | 324 | | - c2 ? convertFastArg(t2, a2, rawFn, stringState, 2) : a2); | | 290 | + return rawFn(c0 ? convertFastArg(t0, a0, stringState, 0) : a0, | | 291 | + c1 ? convertFastArg(t1, a1, stringState, 1) : a1, | | 292 | + c2 ? convertFastArg(t2, a2, stringState, 2) : a2); | 325 | 293 | } finally { | 326 | 294 | if (stringCall) exitStringConversion(stringState); | 327 | 295 | } | @@ -344,7 +312,7 @@ function wrapWithRawPointerConversions(rawFn, argumentTypes, _owner) { | 344 | 312 | for (let i = 0; i < indexes.length; i++) { | 345 | 313 | const index = indexes[i]; | 346 | 314 | args[index] = convertFastArg( | 347 | | - argumentTypes[index], args[index], rawFn, stringState, index); | | 315 | + argumentTypes[index], args[index], stringState, index); | 348 | 316 | } | 349 | 317 | return ReflectApply(rawFn, undefined, args); | 350 | 318 | } finally { | @@ -360,6 +328,5 @@ module.exports = { | 360 | 328 | convertPointerArg, | 361 | 329 | hasPointerMemoryArg, | 362 | 330 | hasStringPointerArg, | 363 | | - initializeFastBufferMetadata, | 364 | 331 | wrapWithRawPointerConversions, | 365 | 332 | }; |
Collapse file Copy file name to clipboardExpand all lines: src/ffi/fast.cc +21-1Lines changed: 21 additions & 1 deletion | Original file line number | Diff line number | Diff line change |
|---|
@@ -178,12 +178,32 @@ bool IsPointerTypeName(const std::string& name) { | 178 | 178 | return name == "pointer" || name == "ptr" || name == "function"; | 179 | 179 | } | 180 | 180 |
| | 181 | +bool IsBufferTypeName(const std::string& name) { | | 182 | + return name == "buffer" || name == "arraybuffer"; | | 183 | +} | | 184 | + | 181 | 185 | bool SignatureNeedsFastBufferInvoke(const FFIFunction& fn) { | 182 | 186 | // The secondary buffer invoke is only generated for the hot monomorphic case | 183 | 187 | // where a single pointer-like argument can be satisfied by a Buffer or | 184 | 188 | // ArrayBuffer without allocating or caching a BigInt pointer in JS. | 185 | 189 | return fn.arg_type_names.size() == 1 && | 186 | | - IsPointerTypeName(fn.arg_type_names[0]); | | 190 | + (IsPointerTypeName(fn.arg_type_names[0]) || | | 191 | + IsBufferTypeName(fn.arg_type_names[0])); | | 192 | +} | | 193 | + | | 194 | +std::shared_ptr<FFIFunction> CloneWithRawPointerArgNames( | | 195 | + const std::shared_ptr<FFIFunction>& fn) { | | 196 | + // The primary Fast API entrypoint receives pointer-compatible values as | | 197 | + // BigInts after the JS wrapper has converted strings, nullish values, and | | 198 | + // memory-backed objects. A secondary entrypoint handles the monomorphic | | 199 | + // memory-backed case without extracting the pointer in JS. | | 200 | + auto clone = std::make_shared<FFIFunction>(*fn); | | 201 | + for (std::string& name : clone->arg_type_names) { | | 202 | + if (IsBufferTypeName(name)) { | | 203 | + name = "pointer"; | | 204 | + } | | 205 | + } | | 206 | + return clone; | 187 | 207 | } | 188 | 208 |
| 189 | 209 | std::shared_ptr<FFIFunction> CloneWithFastBufferArgNames( | |
Collapse file Copy file name to clipboardExpand all lines: src/ffi/fast.h +2Lines changed: 2 additions & 0 deletions | Original file line number | Diff line number | Diff line change |
|---|
@@ -61,6 +61,8 @@ bool SignatureNeedsRawPointerConversions(const FFIFunction& fn); | 61 | 61 | bool SignatureNeedsFastIntegerValidation(const FFIFunction& fn); | 62 | 62 | bool IsPointerTypeName(const std::string& name); | 63 | 63 | bool SignatureNeedsFastBufferInvoke(const FFIFunction& fn); | | 64 | +std::shared_ptr<FFIFunction> CloneWithRawPointerArgNames( | | 65 | + const std::shared_ptr<FFIFunction>& fn); | 64 | 66 | std::shared_ptr<FFIFunction> CloneWithFastBufferArgNames( | 65 | 67 | const std::shared_ptr<FFIFunction>& fn); | 66 | 68 | std::unique_ptr<FastFFIMetadata> CreateFastFFIMetadata(const FFIFunction& fn); | |
Collapse file Copy file name to clipboardExpand all lines: src/node_ffi.cc +2-1Lines changed: 2 additions & 1 deletion | Original file line number | Diff line number | Diff line change |
|---|
@@ -249,7 +249,8 @@ MaybeLocal<Function> DynamicLibrary::CreateFunction( | 249 | 249 | // Try the generated Fast API path first. If metadata creation rejects the | 250 | 250 | // signature, fall back to SharedBuffer for supported scalar shapes, then to | 251 | 251 | // the generic libffi invoker. | 252 | | - info->fast_metadata = CreateFastFFIMetadata(*fn); | | 252 | + std::shared_ptr<FFIFunction> fast_fn = CloneWithRawPointerArgNames(fn); | | 253 | + info->fast_metadata = CreateFastFFIMetadata(*fast_fn); | 253 | 254 | bool use_fast_api = info->fast_metadata != nullptr; | 254 | 255 | bool use_sb = !use_fast_api && IsSBEligibleSignature(*fn); | 255 | 256 | bool has_ptr_args = use_sb && SignatureHasPointerArgs(*fn); | |
Collapse file Copy file name to clipboardExpand all lines: test/ffi/test-ffi-fast-buffer.js +38Lines changed: 38 additions & 0 deletions | Original file line number | Diff line number | Diff line change |
|---|
@@ -95,3 +95,41 @@ test('fast FFI string buffers survive reentrant callbacks', { | 95 | 95 | lib.close(); | 96 | 96 | } | 97 | 97 | }); | | 98 | + | | 99 | +test('optimized buffer signatures preserve pointer-like conversions', () => { | | 100 | + const lib = new ffi.DynamicLibrary(libraryPath); | | 101 | + const asBuffer = lib.getFunction('pointer_to_usize', { | | 102 | + arguments: ['buffer'], | | 103 | + return: 'u64', | | 104 | + }); | | 105 | + const asArrayBuffer = lib.getFunction('pointer_to_usize', { | | 106 | + arguments: ['arraybuffer'], | | 107 | + return: 'u64', | | 108 | + }); | | 109 | + | | 110 | + function callBuffer(value) { | | 111 | + return asBuffer(value); | | 112 | + } | | 113 | + | | 114 | + function callArrayBuffer(value) { | | 115 | + return asArrayBuffer(value); | | 116 | + } | | 117 | + | | 118 | + try { | | 119 | + for (let i = 0; i < 100_000; i++) { | | 120 | + assert.strictEqual(callBuffer(0n), 0n); | | 121 | + assert.strictEqual(callArrayBuffer(0n), 0n); | | 122 | + } | | 123 | + | | 124 | + for (const call of [callBuffer, callArrayBuffer]) { | | 125 | + assert.strictEqual(call(null), 0n); | | 126 | + assert.strictEqual(call(undefined), 0n); | | 127 | + assert.notStrictEqual(call('ffi'), 0n); | | 128 | + | | 129 | + const bytes = Buffer.alloc(1); | | 130 | + assert.strictEqual(call(bytes), ffi.getRawPointer(bytes)); | | 131 | + } | | 132 | + } finally { | | 133 | + lib.close(); | | 134 | + } | | 135 | +}); |
|