From 53606de6344fca63c40fde37056910b72a3c4f8d Mon Sep 17 00:00:00 2001 From: Divy Srivastava Date: Mon, 27 May 2024 21:01:09 -0700 Subject: [PATCH] BREAKING(ffi/unstable): always return u64 as bigint (#23981) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The mixed `number | bigint` representation was useful optimization for pointers. Now, pointers are represented as V8 externals. As part of the FFI stabilization effort we want to make `bigint` the only representation for `u64` and `i64`. BigInt representation performance is almost on par with mixed representation with the added benefit that its less confusing and users don't need manual checks and conversions for doing operations on the value. ``` cpu: AMD Ryzen 5 7530U with Radeon Graphics runtime: deno 1.43.6+92a8d09 (x86_64-unknown-linux-gnu) file:///home/divy/gh/ffi/main.ts benchmark time (avg) iter/s (min … max) p75 p99 p995 -------------------------------------------------------------------------- ----------------------------- nop 4.01 ns/iter 249,533,690.5 (3.97 ns … 10.8 ns) 3.97 ns 4.36 ns 9.03 ns ret bigint 7.74 ns/iter 129,127,186.8 (7.72 ns … 10.46 ns) 7.72 ns 8.11 ns 8.82 ns ret i32 7.81 ns/iter 128,087,100.5 (7.77 ns … 12.72 ns) 7.78 ns 8.57 ns 9.75 ns ret bigint (add op) 15.02 ns/iter 66,588,253.2 (14.64 ns … 24.99 ns) 14.76 ns 19.13 ns 19.44 ns ret i32 (add op) 12.02 ns/iter 83,209,131.8 (11.95 ns … 18.18 ns) 11.98 ns 13.11 ns 14.5 ns ``` --- cli/tsc/dts/lib.deno.unstable.d.ts | 6 ++-- ext/ffi/00_ffi.js | 17 +---------- ext/ffi/callback.rs | 15 ++-------- ext/ffi/ir.rs | 43 +++------------------------- ext/ffi/lib.rs | 3 -- ext/ffi/static.rs | 32 +++++---------------- tests/ffi/tests/ffi_types.ts | 4 +-- tests/ffi/tests/integration_tests.rs | 10 +++---- tests/ffi/tests/test.js | 2 +- 9 files changed, 25 insertions(+), 107 deletions(-) diff --git a/cli/tsc/dts/lib.deno.unstable.d.ts b/cli/tsc/dts/lib.deno.unstable.d.ts index f4588a23c7c04d..a3e0d2eb06b7d6 100644 --- a/cli/tsc/dts/lib.deno.unstable.d.ts +++ b/cli/tsc/dts/lib.deno.unstable.d.ts @@ -247,7 +247,7 @@ declare namespace Deno { : T extends NativeU32Enum ? U : T extends NativeI32Enum ? U : number - : T extends NativeBigIntType ? number | bigint + : T extends NativeBigIntType ? bigint : T extends NativeBooleanType ? boolean : T extends NativePointerType ? T extends NativeTypedPointer ? U | null : PointerValue @@ -292,7 +292,7 @@ declare namespace Deno { : T extends NativeU32Enum ? U : T extends NativeI32Enum ? U : number - : T extends NativeBigIntType ? number | bigint + : T extends NativeBigIntType ? bigint : T extends NativeBooleanType ? boolean : T extends NativePointerType ? T extends NativeTypedPointer ? U | null : PointerValue @@ -319,7 +319,7 @@ declare namespace Deno { : T extends NativeU32Enum ? U : T extends NativeI32Enum ? U : number - : T extends NativeBigIntType ? number | bigint + : T extends NativeBigIntType ? bigint : T extends NativeBooleanType ? boolean : T extends NativePointerType ? T extends NativeTypedPointer ? U | null : PointerValue diff --git a/ext/ffi/00_ffi.js b/ext/ffi/00_ffi.js index 19d62dfdd847c1..7f39db13e96892 100644 --- a/ext/ffi/00_ffi.js +++ b/ext/ffi/00_ffi.js @@ -46,7 +46,6 @@ const { ObjectDefineProperty, ObjectHasOwn, ObjectPrototypeIsPrototypeOf, - Number, NumberIsSafeInteger, TypedArrayPrototypeGetBuffer, TypedArrayPrototypeGetByteLength, @@ -348,10 +347,6 @@ function isReturnedAsBigInt(type) { type === "usize" || type === "isize"; } -function isI64(type) { - return type === "i64" || type === "isize"; -} - function isStruct(type) { return typeof type === "object" && type !== null && typeof type.struct === "object"; @@ -562,7 +557,6 @@ class DynamicLibrary { const call = this.symbols[symbol]; const parameters = symbols[symbol].parameters; const vi = new Int32Array(2); - const vui = new Uint32Array(TypedArrayPrototypeGetBuffer(vi)); const b = new BigInt64Array(TypedArrayPrototypeGetBuffer(vi)); const params = ArrayPrototypeJoin( @@ -572,22 +566,13 @@ class DynamicLibrary { // Make sure V8 has no excuse to not optimize this function. this.symbols[symbol] = new Function( "vi", - "vui", "b", "call", - "NumberIsSafeInteger", - "Number", `return function (${params}) { call(${params}${parameters.length > 0 ? ", " : ""}vi); - ${ - isI64(resultType) - ? `const n1 = Number(b[0])` - : `const n1 = vui[0] + 2 ** 32 * vui[1]` // Faster path for u64 - }; - if (NumberIsSafeInteger(n1)) return n1; return b[0]; }`, - )(vi, vui, b, call, NumberIsSafeInteger, Number); + )(vi, b, call); } else if (isStructResult && !isNonBlocking) { const call = this.symbols[symbol]; const parameters = symbols[symbol].parameters; diff --git a/ext/ffi/callback.rs b/ext/ffi/callback.rs index 55f38c60218a44..1ddb1fff518011 100644 --- a/ext/ffi/callback.rs +++ b/ext/ffi/callback.rs @@ -4,8 +4,6 @@ use crate::check_unstable; use crate::symbol::NativeType; use crate::FfiPermissions; use crate::ForeignFunction; -use crate::MAX_SAFE_INTEGER; -use crate::MIN_SAFE_INTEGER; use deno_core::error::AnyError; use deno_core::op2; use deno_core::v8; @@ -243,20 +241,11 @@ unsafe fn do_ffi_callback( } NativeType::I64 | NativeType::ISize => { let result = *((*val) as *const i64); - if result > MAX_SAFE_INTEGER as i64 || result < MIN_SAFE_INTEGER as i64 - { - v8::BigInt::new_from_i64(scope, result).into() - } else { - v8::Number::new(scope, result as f64).into() - } + v8::BigInt::new_from_i64(scope, result).into() } NativeType::U64 | NativeType::USize => { let result = *((*val) as *const u64); - if result > MAX_SAFE_INTEGER as u64 { - v8::BigInt::new_from_u64(scope, result).into() - } else { - v8::Number::new(scope, result as f64).into() - } + v8::BigInt::new_from_u64(scope, result).into() } NativeType::Pointer | NativeType::Buffer | NativeType::Function => { let result = *((*val) as *const *mut c_void); diff --git a/ext/ffi/ir.rs b/ext/ffi/ir.rs index 6fd1f53ea3e728..520ead92eae8cc 100644 --- a/ext/ffi/ir.rs +++ b/ext/ffi/ir.rs @@ -1,8 +1,6 @@ // Copyright 2018-2024 the Deno authors. All rights reserved. MIT license. use crate::symbol::NativeType; -use crate::MAX_SAFE_INTEGER; -use crate::MIN_SAFE_INTEGER; use deno_core::error::type_error; use deno_core::error::AnyError; use deno_core::v8; @@ -100,46 +98,13 @@ impl NativeValue { v8::Integer::new_from_unsigned(scope, self.u32_value).into() } NativeType::I32 => v8::Integer::new(scope, self.i32_value).into(), - NativeType::U64 => { - let value = self.u64_value; - let local_value: v8::Local = - if value > MAX_SAFE_INTEGER as u64 { - v8::BigInt::new_from_u64(scope, value).into() - } else { - v8::Number::new(scope, value as f64).into() - }; - local_value - } - NativeType::I64 => { - let value = self.i64_value; - let local_value: v8::Local = - if value > MAX_SAFE_INTEGER as i64 || value < MIN_SAFE_INTEGER as i64 - { - v8::BigInt::new_from_i64(scope, self.i64_value).into() - } else { - v8::Number::new(scope, value as f64).into() - }; - local_value - } + NativeType::U64 => v8::BigInt::new_from_u64(scope, self.u64_value).into(), + NativeType::I64 => v8::BigInt::new_from_i64(scope, self.i64_value).into(), NativeType::USize => { - let value = self.usize_value; - let local_value: v8::Local = - if value > MAX_SAFE_INTEGER as usize { - v8::BigInt::new_from_u64(scope, value as u64).into() - } else { - v8::Number::new(scope, value as f64).into() - }; - local_value + v8::BigInt::new_from_u64(scope, self.usize_value as u64).into() } NativeType::ISize => { - let value = self.isize_value; - let local_value: v8::Local = - if !(MIN_SAFE_INTEGER..=MAX_SAFE_INTEGER).contains(&value) { - v8::BigInt::new_from_i64(scope, self.isize_value as i64).into() - } else { - v8::Number::new(scope, value as f64).into() - }; - local_value + v8::BigInt::new_from_i64(scope, self.isize_value as i64).into() } NativeType::F32 => v8::Number::new(scope, self.f32_value as f64).into(), NativeType::F64 => v8::Number::new(scope, self.f64_value).into(), diff --git a/ext/ffi/lib.rs b/ext/ffi/lib.rs index c678a10b54921c..4e81292408f28d 100644 --- a/ext/ffi/lib.rs +++ b/ext/ffi/lib.rs @@ -39,9 +39,6 @@ const _: () = { assert!(size_of::<*const ()>() == 8); }; -pub(crate) const MAX_SAFE_INTEGER: isize = 9007199254740991; -pub(crate) const MIN_SAFE_INTEGER: isize = -9007199254740991; - pub const UNSTABLE_FEATURE_NAME: &str = "ffi"; fn check_unstable(state: &OpState, api_name: &str) { diff --git a/ext/ffi/static.rs b/ext/ffi/static.rs index 3920d885e7a53f..f08605754b2c27 100644 --- a/ext/ffi/static.rs +++ b/ext/ffi/static.rs @@ -2,8 +2,6 @@ use crate::dlfcn::DynamicLibraryResource; use crate::symbol::NativeType; -use crate::MAX_SAFE_INTEGER; -use crate::MIN_SAFE_INTEGER; use deno_core::error::type_error; use deno_core::error::AnyError; use deno_core::op2; @@ -90,45 +88,29 @@ pub fn op_ffi_get_static<'scope>( NativeType::U64 => { // SAFETY: ptr is user provided let result = unsafe { ptr::read_unaligned(data_ptr as *const u64) }; - let integer: v8::Local = if result > MAX_SAFE_INTEGER as u64 { - v8::BigInt::new_from_u64(scope, result).into() - } else { - v8::Number::new(scope, result as f64).into() - }; + let integer: v8::Local = + v8::BigInt::new_from_u64(scope, result).into(); integer } NativeType::I64 => { // SAFETY: ptr is user provided let result = unsafe { ptr::read_unaligned(data_ptr as *const i64) }; - let integer: v8::Local = if result > MAX_SAFE_INTEGER as i64 - || result < MIN_SAFE_INTEGER as i64 - { - v8::BigInt::new_from_i64(scope, result).into() - } else { - v8::Number::new(scope, result as f64).into() - }; + let integer: v8::Local = + v8::BigInt::new_from_i64(scope, result).into(); integer } NativeType::USize => { // SAFETY: ptr is user provided let result = unsafe { ptr::read_unaligned(data_ptr as *const usize) }; - let integer: v8::Local = if result > MAX_SAFE_INTEGER as usize - { - v8::BigInt::new_from_u64(scope, result as u64).into() - } else { - v8::Number::new(scope, result as f64).into() - }; + let integer: v8::Local = + v8::BigInt::new_from_u64(scope, result as u64).into(); integer } NativeType::ISize => { // SAFETY: ptr is user provided let result = unsafe { ptr::read_unaligned(data_ptr as *const isize) }; let integer: v8::Local = - if !(MIN_SAFE_INTEGER..=MAX_SAFE_INTEGER).contains(&result) { - v8::BigInt::new_from_i64(scope, result as i64).into() - } else { - v8::Number::new(scope, result as f64).into() - }; + v8::BigInt::new_from_i64(scope, result as i64).into(); integer } NativeType::F32 => { diff --git a/tests/ffi/tests/ffi_types.ts b/tests/ffi/tests/ffi_types.ts index 596662873157fe..93ac98d926d5c4 100644 --- a/tests/ffi/tests/ffi_types.ts +++ b/tests/ffi/tests/ffi_types.ts @@ -160,7 +160,7 @@ let r_1: number | bigint = result; const result2 = remote.symbols.method17(); // @ts-expect-error: Invalid argument result2.then((_0: string) => {}); -result2.then((_1: number | bigint) => {}); +result2.then((_1: bigint) => {}); const result3 = remote.symbols.method18(); // @ts-expect-error: Invalid argument @@ -430,7 +430,7 @@ type __Tests__ = [ symbols: { foo: ( ...args: (number | Deno.PointerValue | null)[] - ) => number | bigint; + ) => bigint; }; close(): void; }, diff --git a/tests/ffi/tests/integration_tests.rs b/tests/ffi/tests/integration_tests.rs index d0ac6e1049995e..2235405a8a4d22 100644 --- a/tests/ffi/tests/integration_tests.rs +++ b/tests/ffi/tests/integration_tests.rs @@ -76,10 +76,10 @@ fn basic() { 5\n\ 5\n\ 579\n\ - 8589934590\n\ - -8589934590\n\ - 8589934590\n\ - -8589934590\n\ + 8589934590n\n\ + -8589934590n\n\ + 8589934590n\n\ + -8589934590n\n\ 9007199254740992n\n\ 9007199254740992n\n\ -9007199254740992n\n\ @@ -110,7 +110,7 @@ fn basic() { Before\n\ After\n\ logCallback\n\ - 1 -1 2 -2 3 -3 4 -4 0.5 -0.5 1 2 3 4 5 6 7 8\n\ + 1 -1 2 -2 3 -3 4n -4n 0.5 -0.5 1 2 3 4 5 6 7 8\n\ u8: 8\n\ buf: [1, 2, 3, 4, 5, 6, 7, 8]\n\ logCallback\n\ diff --git a/tests/ffi/tests/test.js b/tests/ffi/tests/test.js index 6b8e509c0f8291..fccf6b35e8f27f 100644 --- a/tests/ffi/tests/test.js +++ b/tests/ffi/tests/test.js @@ -637,7 +637,7 @@ await dylib.symbols.call_fn_ptr_return_u8_thread_safe(returnU8Callback.pointer); // Test statics assertEquals(dylib.symbols.static_u32, 42); -assertEquals(dylib.symbols.static_i64, -1242464576485); +assertEquals(dylib.symbols.static_i64, -1242464576485n); assert( typeof dylib.symbols.static_ptr === "object" );