diff --git a/contributions/66197.md b/contributions/66197.md new file mode 100644 index 00000000..fb89fcfc --- /dev/null +++ b/contributions/66197.md @@ -0,0 +1,273 @@ +--- +pr-url: https://github.com/nodejs/node/pull/66197 +--- + +## 문제 내용 + +ffi로 네이티브 함수를 호출할 때, `u64`, `i64`의 값을 매개변수로 사용한다면 JS 코드에서는 bigint의 타입만 허용한다. 그렇기 때문에, 매 호출 시 마다 사용자는 `Bigint()`, 혹은 `20n`과 같이 추가적인 절차를 거쳐야 한다. + +```js +const ffi = require('node:ffi'); +const { lib, functions } = ffi.dlopen(libraryPath, { + add_i64: { return: 'int64', arguments: ['int64', 'int64'] }, + add_u64: { return: 'uint64', arguments: ['uint64', 'uint64'] }, +}); + +assert.strictEqual(functions.add_i64(10n, 20n), 30n); +assert.strictEqual(functions.add_u64(10n, 20n), 30n); + +assert.strictEqual(functions.add_i64(BigInt(10), BigInt(20)), BigInt(30)) +``` + +이러한 불편함은 ffi를 통해 버퍼를 전달할 때 더 두드러진다. 네이티브 코드에서는 버퍼를 전달하더라도 내부적으로 버퍼의 시작 메모리 주소만 들고 있기 때문에 버퍼의 길이를 전달해야 제대로된 호출이 가능해진다. + +JS에서는 Number 타입의 범위가 2^53-1까지이기 때문에 버퍼의 길이가 이 이상을 초과하는 경우도 고려하여 버퍼를 전달받을 땐 `i64`, `u64`를 통해 전달받아야 한다. 그렇다면 사용자는 버퍼를 전달할 때마다 버퍼의 길이를 `Bigint()`로 감싸서 호출해야 하는 불편함이 생긴다. + +```js +const ffi = require('node:ffi'); +const { functions } = ffi.dlopen(path, { + sum_buffer: { arguments: ['buffer', 'u64'], return: 'u64' }, +}); + +const bytes = new Uint8Array([1, 2, 3]); +functions.sum_buffer(bytes, BigInt(bytes.byteLength)); +``` +이러한 불편함을 개선하기 위해 `i64`, `u64` 타입의 매개변수도 JS에서 Number를 보내더라도 내부적으로 Number와 Bigint를 감지하여 변환할 수 있도록 제안하고자 한다. 더 정확히는 `Bigint` 타입을 요구하여도 Number value를 허용하는 작업이다. + +```js +functions.sum_buffer(bytes, BigInt(bytes.byteLength)); // success +functions.sum_buffer(bytes, bytes.byteLength); // success +``` + +## 해결 과정과 검증 + +허용되지 않던 타입을 다시 허용하는 것이기 때문에 테스트 코드를 먼저 수정하고 추가해주었다. + +- `test-ffi-calls.js` : 공개 FFI API 테스트 +- `test-ffi-fast-integer-validation.js` : Fast API 테스트 +- `test-ffi-shared-buffer.js` : SharedBuffer와 slow-path fallback 테스트 + + +1. 기본적인 타입 테스트 +```js +assert.strictEqual(symbols.add_i64(20, 22), symbols.add_i64(20n, 22n)); +assert.strictEqual(symbols.add_u64(20, 22), symbols.add_u64(20n, 22n)); +assert.strictEqual(symbols.add_i64(-20, 22n), 2n); +assert.strictEqual(symbols.add_u64(20n, 22), 42n); + +assert.throws(() => symbols.add_i64(1.5, 2)) +//Argument 0 must be an int64/); + +assert.throws(() => symbols.add_i64(Number.NaN, 2)) +//Argument 0 must be an int64/); + +assert.throws(() => symbols.add_i64(Number.POSITIVE_INFINITY, 2)) //Argument 0 must be an int64/); + +assert.throws(() => symbols.add_i64(Number.NEGATIVE_INFINITY, 2)) //Argument 0 must be an int64/); + +assert.throws(() => symbols.add_i64(Number.MAX_SAFE_INTEGER + 1, 2)) //Argument 0 must be an int64/); + +assert.throws(() => symbols.add_i64(Number.MIN_SAFE_INTEGER - 1, 2)) //Argument 0 must be an int64/); +``` +2. Buffer와 `.byteLength()` 테스트 + +```js +assert.strictEqual( + symbols.sum_buffer(buffer, buffer.length), + symbols.sum_buffer(buffer, BigInt(buffer.length)), +); +``` + +3. SharedBuffer와 slow-path 테스트 + +```js +test('SB wrapper accepts safe Number values for i64/u64', () => {}) +test('generic conversion accepts safe Number values for i64/u64', () => {}) +``` + +ffi는 내부적으로 Fast API -> SharedBuffer -> Generic path로 fallback되는데, fallback되더라도 해당 경로에서도 안정적으로 `i64`와 `u64` 타입에 number가 들어가도 허용이 되는 지를 테스트하는 코드다. + +테스트 코드를 먼저 작성하고 ffi 모듈 내에서 타입을 검사하는 부분을 수정해주었다. ffi에서는 3가지의 call path가 존재하기 때문에, 이 세 부분을 담당하는 곳을 찾아 각각 type validation을 변경하였다. + +- **Fast API** -> `lib/internal/ffi/fast-api.js` + - V8 Fast API용 네이티브 trampoline을 직접 호출 + - 일반적인 JS→C++ callback 진입과 libffi 변환을 우회 + - 지원 가능한 타입과 최대 8개 인자라는 제약이 있음 +- **SharedBuffer** -> `lib/internal/ffi-shared-buffer.js` + - JS에서 인자를 `ArrayBuffer`에 기록 + - C++의 `InvokeFunctionSB()`가 해당 메모리에서 인자를 읽어 호출 + - generic `ToFFIArgument()` 변환을 매번 실행하지 않음 + - Fast API를 사용할 수 없지만 단순한 스칼라 시그니처인 경우 사용 +- **Generic libffi** -> `src/ffi/types.cc` + - `DynamicLibrary::InvokeFunction()` 실행 + - 각각의 JS 인자를 `ToFFIArgument()`로 검사하고 변환 + - 가장 일반적이지만 세 경로 중 상대적으로 느림 + + +**1. `types.cc`** + 일반적으로 FFI를 통해 네이티브 함수를 호출하면 `ToFFIArgument()`를 통해 들어오는 인자를 검증하고 변환하게 된다. 이 함수 내부에서 각 인자의 타입에 따라 분기처리를 해주고 있다. +```cpp +if (type == &ffi_type_sint64) { + if (!arg->IsBigInt()) THROW_ERR_INVALID_ARG_VALUE(env, "Argument %u must be an int64", index); + return {}; + } + bool lossless; + *static_cast(ret) = arg.As()->Int64Value(&lossless); + if (!lossless) { + THROW_ERR_INVALID_ARG_VALUE(env, "Argument %u must be an int64", index); + return {}; + } +``` + +기존에는 `int64`로 들어왔을 때 `BigInt()`가 아니라면 에러를 발생시키고, 해당 값을 실제 `Int64`로 형변환을 해보았을 때, 그 값이 동일하지 않다면 에러를 발생시켰다. + +```cpp +if (type == &ffi_type_sint64) { + if (arg->IsBigInt()) { + bool lossless; + int64_t value = arg.As()->Int64Value(&lossless); + if (!lossless) { + THROW_ERR_INVALID_ARG_VALUE(env, "Argument %u must be an int64", index); + return {}; + } + + *static_cast(ret) = value; + } else { + int64_t value; + if (!GetStrictSignedInteger( + arg, -kMaxSafeJsInteger, kMaxSafeJsInteger, &value)) { + THROW_ERR_INVALID_ARG_VALUE(env, "Argument %u must be an int64", index); + return {}; + } + + *static_cast(ret) = value; + } +``` + +이제는 해당 함수에서 `BigInt`와 `Number`를 모두 검증해야 한다. + +`int64`로 들어왔을 때 `BigInt`라면 기존과 동일하게 처리했지만 그게 아닌 `Number`의 값이라면 그 `Number`의 범위가 MAX_SAFE_INTEGER 에 해당하는 지, `Number` 타입에 해당하는 지를 검증해주었다. + + +**2. `ffi-shared-buffer.js`** +`Bigint` 형전환과 `Number` 타입의 안전한 범위 검사를 위해 primordials에 `NumberIsSafeInteger()`와 `BigInt()`를 import 해왔다. + +`writeNumericArg()` 함수 내에서 숫자형의 값이 들어오면 `kind` 속성을 통해 타입을 내부적으로 검사하여 에러를 발생시킨다. + +```js +if (kind === 'i64') { + if (typeof arg !== 'bigint' || arg < I64_MIN || arg > I64_MAX) { + throwFFIArgError(`Argument ${index} must be ${info.label}`); + } +sI64(view, offset, arg, true); +return; +} +``` + +기존에는 `i64` 타입으로 들어온다면 `bigint` 가 아니거나 안전한 범위 내에 있는 지 검사한 후 `i64`의 값으로 저장한다. + +하지만 이제 `i64` 로 들어올 때, `Number`의 타입도 허용하기 때문에 다음과 같이 검증해주었다. + +```js +if (kind === 'i64') { + if (typeof arg === 'number') { + if (!NumberIsSafeInteger(arg)) { + throwFFIArgError(`Argument ${index} must be ${info.label}`); + } + arg = BigInt(arg); + } else if (typeof arg !== 'bigint' || arg < I64_MIN || arg > I64_MAX) { + throwFFIArgError(`Argument ${index} must be ${info.label}`); + } + sI64(view, offset, arg, true); + return; +} +``` + +`i64`로 들어올 때, 만약 `number` 타입이라면 해당 값이 `safeInteger` 범위 내에 있는 지 검사하고 에러를 발생시키거나 `BigInt()`로 타입을 변환한다. + - `-(2^53 - 1)` 부터 `2^53 - 1` 까지의 범위 검사 + +만약 `BigInt`로 들어왔으면 기존과 동일하게 검증한다. `u64`로 들어온다면 `number`의 값이 들어올 때, `safeInteger`와 더불어, 0 이상의 값인 지도 검사해주었다. + +**3. `fast-api.js`** + +`convertFastArg()` 함수를 통해 들어온 매개변수 값 중 정수와 포인터 값을 검사한 후 값을 반환한다. 이 때, 정수를 검사하는 `validateFastPointerArg()`을 수정해주었다. + +```js +const validType = info.kind === 'number' ? + typeof value === 'number' && NumberIsInteger(value) : + typeof value === 'bigint'; + +if (!validType || value < info.min || value > info.max) { + throwFFIArgError(`Argument ${index} must be ${info.label}`); +} +``` +`number` 타입이라면 `number` 타입과 안전한 범위인지 여부를 확인하고, 그게 아니라면 `bigint` 타입 여부를 확인한다. 이 후, 해당 매개변수 값의 범위를 확인하여 에러를 발생킨다 + +```js +// The native Fast API expects BigInt for 64-bit integer arguments. +if (info.kind === 'bigint' && typeof value === 'number') { + if (!NumberIsSafeInteger(value) || (info.min === 0n && value < 0)) { + throwFFIArgError(`Argument ${index} must be ${info.label}`); + } + return BigInt(value); +} +``` +해당 함수에 `bigint` 타입이지만 `number`로 들어오는 경우를 분리하여 처리해주었다. 이러한 경우에 들어온 값의 범위를 허용한 뒤, 안전한 범위의 Number가 들어올 경우에 이를 `Bigint()`로 감싸서 처리해주었다. + +## 기여 회고 + +벤치마킹 성능에 대해 리뷰를 요청받았고 이에 대해 성능을 측정하여 답변을 드렸다. + +- Bigint로 감싸서 보냈을 때, 이전 로직에 비해 성능이 향상됨 (로직의 개선이 아닌 V8 Inlinig 최적화로 인한 개선으로 보임) +```console + confidence improvement accuracy (*) (**) (***) +ffi/add-i64.js n=10000000 *** +29.99 % ±4.82% ±6.41% ±8.36% +ffi/add-u64.js n=10000000 *** +35.96 % ±5.13% ±6.83% ±8.89% + + -41.1% 0% +41.1% +ffi/add-i64.js n=10000000 |██████████████░░ +29.99% *** +ffi/add-u64.js n=10000000 |█████████████████░░ +35.96% *** +``` + +- 동일한 함수에 대해 Bigint로 감싸서 보냈을 때 vs Number로 직접 보냈을 때 + +**1. Pre-created BigInt values vs. Number values (the original test)** +```js +const a = input === 'number' ? 20 : 20n; +const b = input === 'number' ? 22 : 22n; + +bench.start(); +for (let i = 0; i < n; ++i) + add(a, b); +bench.end(n); +``` +**2. Explicit conversion of Number constants vs. direct Number arguments** +```js +// input === 'bigint' +add(BigInt(20), BigInt(22)); + +// input === 'number' +add(20, 22); +``` +**3. Explicit conversion of buffer lengths vs. direct Number arguments** +```js +const a = Buffer.alloc(64); +const b = Buffer.alloc(256); + +// input === 'bigint' +add(BigInt(a.length), BigInt(b.length)); + +// input === 'number' +add(a.length, b.length); +``` + +Argument preparation | Benchmark | BigInt (ops/s) | Number (ops/s) | Change +-- | -- | -- | -- | -- +Pre-created values | add-i64 | 29,288,918.5 | 19,523,825.1 | −33.34% +Pre-created values | add-u64 | 28,502,908.3 | 19,819,141.9 | −30.47% +BigInt(20), BigInt(22) | add-i64 | 29,191,962.6 | 24,779,345.4 | −15.12% +BigInt(20), BigInt(22) | add-u64 | 28,580,807.5 | 25,475,704.5 | −10.86% +BigInt(a.length), BigInt(b.length) | add-i64 | 18,111,529.1 | 20,226,464.4 | +11.68% +BigInt(a.length), BigInt(b.length) | add-u64 | 17,549,515.7 | 19,566,290.2 | +11.49% + +미리 Bigint()를 만들고 로직을 측정할 때는 30% (약 15~17ns) 정도 성능이 저하되었지만 Bigint()를 함수 내부에 직접 호출할 경우 15%정도의 성능이 저하되었고 `Buffer.length`를 통해서 Bigint()를 함수 내부에서 호출하면 오히려 11% 정도의 성능 개선이 발생하였다.