diff --git a/CHANGELOG.md b/CHANGELOG.md index 3635e9a50d..607101d6e1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,7 +15,9 @@ [All changes in [[UnreleasedVersion]]](https://github.com/mozilla/uniffi-rs/compare/v0.23.0...HEAD). ### ⚠️ Breaking Changes ⚠️ -- ABI: Implemented a new callback-interface ABI that significantly improves performance on Python and Kotlin. UniFFI users will automatically get the benefits of this without any code changes. This is a breaking change because the new generated code is not compatible with code generated with UniFFI 0.23.x. +- ABI: Implemented a new callback-interface ABI that significantly improves performance on Python and Kotlin. + - UniFFI users will automatically get the benefits of this without any code changes. + - External bindings authors will need to update their bindings code. See PR #1494 for details. ### What's changed diff --git a/fixtures/benchmarks/README.md b/fixtures/benchmarks/README.md index 46d234436c..93fb811742 100644 --- a/fixtures/benchmarks/README.md +++ b/fixtures/benchmarks/README.md @@ -1,5 +1,10 @@ This fixture runs a set of benchmark tests, using criterion to test the performance. +- `cargo bench` to run all benchmarks. +- `cargo bench -- -p` to run all python benchmarks (or -s for swift, -k for kotlin) +- `cargo bench -- [glob]` to run a subset of the benchmarks +- `cargo bench -- --help` for more details on the CLI + Benchmarking UniFFI is tricky and involves a bit of ping-pong between Rust and the foreign language: diff --git a/fixtures/benchmarks/benches/bindings/run_benchmarks.kts b/fixtures/benchmarks/benches/bindings/run_benchmarks.kts index d11bfffbf1..9d0b7ef408 100644 --- a/fixtures/benchmarks/benches/bindings/run_benchmarks.kts +++ b/fixtures/benchmarks/benches/bindings/run_benchmarks.kts @@ -6,14 +6,14 @@ import uniffi.benchmarks.* import kotlin.system.measureNanoTime class TestCallbackObj : TestCallbackInterface { - override fun testMethod(a: Int, b: Int, data: TestData): String { + override fun method(a: Int, b: Int, data: TestData): String { return data.bar; } - override fun testVoidReturn(a: Int, b: Int, data: TestData) { + override fun methodWithVoidReturn(a: Int, b: Int, data: TestData) { } - override fun testNoArgsVoidReturn() { + override fun methodWithNoArgsAndVoidReturn() { } override fun runTest(testCase: TestCase, count: ULong): ULong { diff --git a/fixtures/benchmarks/benches/bindings/run_benchmarks.py b/fixtures/benchmarks/benches/bindings/run_benchmarks.py index ce2d22ff4d..10063512cd 100644 --- a/fixtures/benchmarks/benches/bindings/run_benchmarks.py +++ b/fixtures/benchmarks/benches/bindings/run_benchmarks.py @@ -6,13 +6,13 @@ import time class TestCallbackObj: - def test_method(self, a, b, data): + def method(self, a, b, data): return data.bar - def test_void_return(self, a, b, data): + def method_with_void_return(self, a, b, data): pass - def test_no_args_void_return(self): + def method_with_no_args_and_void_return(self): pass def run_test(self, test_case, count): diff --git a/fixtures/benchmarks/benches/bindings/run_benchmarks.swift b/fixtures/benchmarks/benches/bindings/run_benchmarks.swift index 6404e918df..023e24ee50 100644 --- a/fixtures/benchmarks/benches/bindings/run_benchmarks.swift +++ b/fixtures/benchmarks/benches/bindings/run_benchmarks.swift @@ -13,14 +13,14 @@ #endif class TestCallbackObj: TestCallbackInterface { - func testMethod(a: Int32, b: Int32, data: TestData) -> String { + func method(a: Int32, b: Int32, data: TestData) -> String { return data.bar } - func testVoidReturn(a: Int32, b: Int32, data: TestData) { + func methodWithVoidReturn(a: Int32, b: Int32, data: TestData) { } - func testNoArgsVoidReturn() { + func methodWithNoArgsAndVoidReturn() { } func runTest(testCase: TestCase, count: UInt64) -> UInt64 { diff --git a/fixtures/benchmarks/src/benchmarks.udl b/fixtures/benchmarks/src/benchmarks.udl index 53ea45cc81..c6646d33ee 100644 --- a/fixtures/benchmarks/src/benchmarks.udl +++ b/fixtures/benchmarks/src/benchmarks.udl @@ -31,9 +31,9 @@ callback interface TestCallbackInterface { // including: popping arguments from the stack, unpacking RustBuffers, // pushing return values back to the stack, etc. - string test_method(i32 a, i32 b, TestData data); // Should return data.bar - void test_void_return(i32 a, i32 b, TestData data); - void test_no_args_void_return(); + string method(i32 a, i32 b, TestData data); // Should return data.bar + void method_with_void_return(i32 a, i32 b, TestData data); + void method_with_no_args_and_void_return(); // Run a performance test N times and return the elapsed time in nanoseconds u64 run_test(TestCase test_case, u64 count); diff --git a/fixtures/benchmarks/src/lib.rs b/fixtures/benchmarks/src/lib.rs index 624f0549a0..34ff5d858b 100644 --- a/fixtures/benchmarks/src/lib.rs +++ b/fixtures/benchmarks/src/lib.rs @@ -20,9 +20,9 @@ pub enum TestCase { } pub trait TestCallbackInterface { - fn test_method(&self, a: i32, b: i32, data: TestData) -> String; - fn test_void_return(&self, a: i32, b: i32, data: TestData); - fn test_no_args_void_return(&self); + fn method(&self, a: i32, b: i32, data: TestData) -> String; + fn method_with_void_return(&self, a: i32, b: i32, data: TestData); + fn method_with_no_args_and_void_return(&self); fn run_test(&self, test_case: TestCase, count: u64) -> u64; } @@ -61,7 +61,7 @@ pub fn run_benchmarks(language: String, cb: Box) { .noise_threshold(0.05) .bench_function(format!("{language}-callbacks-basic"), |b| { b.iter(|| { - cb.test_method( + cb.method( 10, 100, TestData { @@ -73,7 +73,7 @@ pub fn run_benchmarks(language: String, cb: Box) { }) .bench_function(format!("{language}-callbacks-void-return"), |b| { b.iter(|| { - cb.test_void_return( + cb.method_with_void_return( 10, 100, TestData { @@ -84,7 +84,7 @@ pub fn run_benchmarks(language: String, cb: Box) { }) }) .bench_function(format!("{language}-callbacks-no-args-void-return"), |b| { - b.iter(|| cb.test_no_args_void_return()) + b.iter(|| cb.method_with_no_args_and_void_return()) }); c.final_summary(); diff --git a/uniffi_bindgen/src/bindings/kotlin/templates/CallbackInterfaceRuntime.kt b/uniffi_bindgen/src/bindings/kotlin/templates/CallbackInterfaceRuntime.kt index 302702b8bd..0f58da6068 100644 --- a/uniffi_bindgen/src/bindings/kotlin/templates/CallbackInterfaceRuntime.kt +++ b/uniffi_bindgen/src/bindings/kotlin/templates/CallbackInterfaceRuntime.kt @@ -41,6 +41,10 @@ interface ForeignCallback : com.sun.jna.Callback { // Magic number for the Rust proxy to call using the same mechanism as every other method, // to free the callback once it's dropped by Rust. internal const val IDX_CALLBACK_FREE = 0 +// Callback return codes +internal const val UNIFFI_CALLBACK_SUCCESS = 0 +internal const val UNIFFI_CALLBACK_ERROR = 1 +internal const val UNIFFI_CALLBACK_UNEXPECTED_ERROR = 2 public abstract class FfiConverterCallbackInterface( protected val foreignCallback: ForeignCallback diff --git a/uniffi_bindgen/src/bindings/kotlin/templates/CallbackInterfaceTemplate.kt b/uniffi_bindgen/src/bindings/kotlin/templates/CallbackInterfaceTemplate.kt index d1fb106c12..7ea7fe25d8 100644 --- a/uniffi_bindgen/src/bindings/kotlin/templates/CallbackInterfaceTemplate.kt +++ b/uniffi_bindgen/src/bindings/kotlin/templates/CallbackInterfaceTemplate.kt @@ -27,15 +27,15 @@ internal class {{ foreign_callback }} : ForeignCallback { return when (method) { IDX_CALLBACK_FREE -> { {{ ffi_converter_name }}.drop(handle) - // No return value. - // See docs of ForeignCallback in `uniffi/src/ffi/foreigncallbacks.rs` - 0 + // Successful return + // See docs of ForeignCallback in `uniffi_core/src/ffi/foreigncallbacks.rs` + UNIFFI_CALLBACK_SUCCESS } {% for meth in cbi.methods() -%} {% let method_name = format!("invoke_{}", meth.name())|fn_name -%} {{ loop.index }} -> { // Call the method, write to outBuf and return a status code - // See docs of ForeignCallback in `uniffi/src/ffi/foreigncallbacks.rs` for info + // See docs of ForeignCallback in `uniffi_core/src/ffi/foreigncallbacks.rs` for info try { this.{{ method_name }}(cb, argsData, argsLen, outBuf) } catch (e: Throwable) { @@ -46,20 +46,20 @@ internal class {{ foreign_callback }} : ForeignCallback { } catch (e: Throwable) { // If that fails, then it's time to give up and just return } - -1 + UNIFFI_CALLBACK_UNEXPECTED_ERROR } } {% endfor %} else -> { // An unexpected error happened. - // See docs of ForeignCallback in `uniffi/src/ffi/foreigncallbacks.rs` + // See docs of ForeignCallback in `uniffi_core/src/ffi/foreigncallbacks.rs` try { // Try to serialize the error into a string outBuf.setValue({{ Type::String.borrow()|ffi_converter_name }}.lower("Invalid Callback index")) } catch (e: Throwable) { // If that fails, then it's time to give up and just return } - -1 + UNIFFI_CALLBACK_UNEXPECTED_ERROR } } } @@ -84,7 +84,7 @@ internal class {{ foreign_callback }} : ForeignCallback { {%- endfor %} ) outBuf.setValue({{ return_type|ffi_converter_name }}.lowerIntoRustBuffer(returnValue)) - return 1 + return UNIFFI_CALLBACK_SUCCESS } {%- when None %} fun makeCall() : Int { @@ -94,7 +94,7 @@ internal class {{ foreign_callback }} : ForeignCallback { {%- if !loop.last %}, {% endif %} {%- endfor %} ) - return 1 + return UNIFFI_CALLBACK_SUCCESS } {%- endmatch %} @@ -107,7 +107,7 @@ internal class {{ foreign_callback }} : ForeignCallback { } catch (e: {{ error_type|type_name }}) { // Expected error, serialize it into outBuf outBuf.setValue({{ error_type|ffi_converter_name }}.lowerIntoRustBuffer(e)) - -2 + UNIFFI_CALLBACK_ERROR } {%- endmatch %} diff --git a/uniffi_bindgen/src/bindings/python/templates/CallbackInterfaceRuntime.py b/uniffi_bindgen/src/bindings/python/templates/CallbackInterfaceRuntime.py index e8a0ec56d9..4d23701689 100644 --- a/uniffi_bindgen/src/bindings/python/templates/CallbackInterfaceRuntime.py +++ b/uniffi_bindgen/src/bindings/python/templates/CallbackInterfaceRuntime.py @@ -40,6 +40,10 @@ def remove(self, handle): # Magic number for the Rust proxy to call using the same mechanism as every other method, # to free the callback once it's dropped by Rust. IDX_CALLBACK_FREE = 0 +# Return codes for callback calls +UNIFFI_CALLBACK_SUCCESS = 0 +UNIFFI_CALLBACK_ERROR = 1 +UNIFFI_CALLBACK_UNEXPECTED_ERROR = 2 class FfiConverterCallbackInterface: _handle_map = ConcurrentHandleMap() diff --git a/uniffi_bindgen/src/bindings/python/templates/CallbackInterfaceTemplate.py b/uniffi_bindgen/src/bindings/python/templates/CallbackInterfaceTemplate.py index 5702736de4..b847f04971 100644 --- a/uniffi_bindgen/src/bindings/python/templates/CallbackInterfaceTemplate.py +++ b/uniffi_bindgen/src/bindings/python/templates/CallbackInterfaceTemplate.py @@ -40,7 +40,7 @@ def makeCallAndHandleReturn(): {%- when None %} makeCall() {%- endmatch %} - return 1 + return UNIFFI_CALLBACK_SUCCESS {%- match meth.throws_type() %} {%- when None %} @@ -53,7 +53,7 @@ def makeCallAndHandleReturn(): with RustBuffer.allocWithBuilder() as builder: {{ err|write_fn }}(e, builder) buf_ptr[0] = builder.finalize() - return -2 + return UNIFFI_CALLBACK_ERROR {%- endmatch %} {% endfor %} @@ -64,15 +64,15 @@ def makeCallAndHandleReturn(): if method == IDX_CALLBACK_FREE: {{ ffi_converter_name }}.drop(handle) - # No return value. - # See docs of ForeignCallback in `uniffi/src/ffi/foreigncallbacks.rs` - return 0 + # Successfull return + # See docs of ForeignCallback in `uniffi_core/src/ffi/foreigncallbacks.rs` + return UNIFFI_CALLBACK_SUCCESS {% for meth in cbi.methods() -%} {% let method_name = format!("invoke_{}", meth.name())|fn_name -%} if method == {{ loop.index }}: # Call the method and handle any errors - # See docs of ForeignCallback in `uniffi/src/ffi/foreigncallbacks.rs` for details + # See docs of ForeignCallback in `uniffi_core/src/ffi/foreigncallbacks.rs` for details try: return {{ method_name }}(cb, RustBufferStream(args_data, args_len), buf_ptr) except BaseException as e: @@ -83,7 +83,7 @@ def makeCallAndHandleReturn(): except: # If that fails, just give up pass - return -1 + return UNIFFI_CALLBACK_UNEXPECTED_ERROR {% endfor %} # This should never happen, because an out of bounds method index won't @@ -91,8 +91,8 @@ def makeCallAndHandleReturn(): # https://github.com/mozilla/uniffi-rs/issues/351 # An unexpected error happened. - # See docs of ForeignCallback in `uniffi/src/ffi/foreigncallbacks.rs` - return -1 + # See docs of ForeignCallback in `uniffi_core/src/ffi/foreigncallbacks.rs` + return UNIFFI_CALLBACK_UNEXPECTED_ERROR # We need to keep this function reference alive: # if they get GC'd while in use then UniFFI internals could attempt to call a function diff --git a/uniffi_bindgen/src/bindings/swift/templates/CallbackInterfaceRuntime.swift b/uniffi_bindgen/src/bindings/swift/templates/CallbackInterfaceRuntime.swift index 9ab9bdbe48..9ae62d1667 100644 --- a/uniffi_bindgen/src/bindings/swift/templates/CallbackInterfaceRuntime.swift +++ b/uniffi_bindgen/src/bindings/swift/templates/CallbackInterfaceRuntime.swift @@ -58,3 +58,7 @@ fileprivate class UniFFICallbackHandleMap { // Magic number for the Rust proxy to call using the same mechanism as every other method, // to free the callback once it's dropped by Rust. private let IDX_CALLBACK_FREE: Int32 = 0 +// Callback return codes +private let UNIFFI_CALLBACK_SUCCESS: Int32 = 0 +private let UNIFFI_CALLBACK_ERROR: Int32 = 1 +private let UNIFFI_CALLBACK_UNEXPECTED_ERROR: Int32 = 2 diff --git a/uniffi_bindgen/src/bindings/swift/templates/CallbackInterfaceTemplate.swift b/uniffi_bindgen/src/bindings/swift/templates/CallbackInterfaceTemplate.swift index 4e4267b7f4..53e347c189 100644 --- a/uniffi_bindgen/src/bindings/swift/templates/CallbackInterfaceTemplate.swift +++ b/uniffi_bindgen/src/bindings/swift/templates/CallbackInterfaceTemplate.swift @@ -37,7 +37,7 @@ fileprivate let {{ foreign_callback }} : ForeignCallback = var writer = [UInt8]() {{ return_type|write_fn }}(result, into: &writer) out_buf.pointee = RustBuffer(bytes: writer) - return 1 + return UNIFFI_CALLBACK_SUCCESS } {%- when None %} func makeCall() throws -> Int32 { @@ -47,7 +47,7 @@ fileprivate let {{ foreign_callback }} : ForeignCallback = {%- if !loop.last %}, {% endif %} {% endfor -%} ) - return 1 + return UNIFFI_CALLBACK_SUCCESS } {%- endmatch %} @@ -59,7 +59,7 @@ fileprivate let {{ foreign_callback }} : ForeignCallback = return try makeCall() } catch let error as {{ error_type|type_name }} { out_buf.pointee = {{ error_type|lower_fn }}(error) - return -2 + return UNIFFI_CALLBACK_ERROR } {%- endmatch %} } @@ -69,9 +69,9 @@ fileprivate let {{ foreign_callback }} : ForeignCallback = switch method { case IDX_CALLBACK_FREE: {{ ffi_converter_name }}.drop(handle: handle) - // No return value. - // See docs of ForeignCallback in `uniffi/src/ffi/foreigncallbacks.rs` - return 0 + // Sucessful return + // See docs of ForeignCallback in `uniffi_core/src/ffi/foreigncallbacks.rs` + return UNIFFI_CALLBACK_SUCCESS {% for meth in cbi.methods() -%} {% let method_name = format!("invoke_{}", meth.name())|fn_name -%} case {{ loop.index }}: @@ -80,13 +80,13 @@ fileprivate let {{ foreign_callback }} : ForeignCallback = cb = try {{ ffi_converter_name }}.lift(handle) } catch { out_buf.pointee = {{ Type::String.borrow()|lower_fn }}("{{ cbi.name() }}: Invalid handle") - return -1 + return UNIFFI_CALLBACK_UNEXPECTED_ERROR } do { return try {{ method_name }}(cb, argsData, argsLen, out_buf) } catch let error { out_buf.pointee = {{ Type::String.borrow()|lower_fn }}(String(describing: error)) - return -1 + return UNIFFI_CALLBACK_UNEXPECTED_ERROR } {% endfor %} // This should never happen, because an out of bounds method index won't @@ -94,8 +94,8 @@ fileprivate let {{ foreign_callback }} : ForeignCallback = // https://github.com/mozilla/uniffi-rs/issues/351 default: // An unexpected error happened. - // See docs of ForeignCallback in `uniffi/src/ffi/foreigncallbacks.rs` - return -1 + // See docs of ForeignCallback in `uniffi_core/src/ffi/foreigncallbacks.rs` + return UNIFFI_CALLBACK_UNEXPECTED_ERROR } } diff --git a/uniffi_core/src/ffi/foreigncallbacks.rs b/uniffi_core/src/ffi/foreigncallbacks.rs index 40a51ecafc..9fd50508a7 100644 --- a/uniffi_core/src/ffi/foreigncallbacks.rs +++ b/uniffi_core/src/ffi/foreigncallbacks.rs @@ -126,8 +126,8 @@ use std::sync::atomic::{AtomicUsize, Ordering}; /// * The `handle` is the key into a handle map on the other side of the FFI used to look up the foreign language object /// that implements the callback interface/trait. /// * The `method` selector specifies the method that will be called on the object, by looking it up in a list of methods from -/// the IDL. The index is 1 indexed. Note that the list of methods is generated by at uniffi from the IDL and used in all -/// bindings: so we can rely on the method list being stable within the same run of uniffi. +/// the IDL. The list is 1 indexed. Note that the list of methods is generated by UniFFI from the IDL and used in all +/// bindings, so we can rely on the method list being stable within the same run of UniFFI. /// * `args_data` and `args_len` represents a serialized buffer of arguments to the function. The scaffolding code /// writes the callback arguments to this buffer, in order, using `FfiConverter.write()`. The bindings code reads the /// arguments from the buffer and passes them to the user's callback. @@ -151,6 +151,9 @@ pub type ForeignCallback = unsafe extern "C" fn( /// The method index used by the Drop trait to communicate to the foreign language side that Rust has finished with it, /// and it can be deleted from the handle map. pub const IDX_CALLBACK_FREE: u32 = 0; +pub const CALLBACK_SUCCESS: i32 = 0; +pub const CALLBACK_ERROR: i32 = 1; +pub const CALLBACK_UNEXPECTED_ERROR: i32 = 2; // Overly-paranoid sanity checking to ensure that these types are // convertible between each-other. `transmute` actually should check this for @@ -206,6 +209,8 @@ impl ForeignCallbackInternals { ) -> c_int { let ptr_value = self.callback_ptr.load(Ordering::SeqCst); unsafe { + // SAFETY: `callback_ptr` was set in `set_callback` from a ForeignCallback pointer, so + // it's safe to transmute it back here. let callback = std::mem::transmute::>(ptr_value) .expect("Callback interface handler not set"); callback( @@ -218,6 +223,8 @@ impl ForeignCallbackInternals { } } + // TODO: Refactor the callback code so that we don't need to have 3 different functions here + /// Invoke a callback interface method that can't throw on the foreign side and returns /// non-Result value on the Rust side pub fn invoke_callback(&self, handle: u64, method: u32, args: RustBuffer) -> R @@ -227,23 +234,15 @@ impl ForeignCallbackInternals { let mut ret_rbuf = RustBuffer::new(); let callback_result = self.call_callback(handle, method, args, &mut ret_rbuf); match callback_result { - 1 => { - // 1 indicates success with the return value written to the RustBuffer for - // non-void calls. + CALLBACK_SUCCESS => { let vec = ret_rbuf.destroy_into_vec(); let mut ret_buf = vec.as_slice(); R::try_read(&mut ret_buf).unwrap() } - -2 => { - panic!("Callback return -2, but no Error was expected") - } - // 0 is a deprecated method to indicates success for void returns - 0 => { - log::error!("UniFFI: Callback interface returned 0. Please update the bindings code to return 1 for all successful calls"); - panic!("Callback returned 0 when we were expecting a return value"); + CALLBACK_ERROR => { + panic!("Callback return 1, but no Error was expected") } - // -1 indicates an unexpected error - -1 => { + CALLBACK_UNEXPECTED_ERROR => { let reason = if !ret_rbuf.is_empty() { match >::try_lift(ret_rbuf) { Ok(s) => s, @@ -263,8 +262,8 @@ impl ForeignCallbackInternals { } } - /// Invoke a callback interface method that can throw on the foreign side / returnns a Result<> - /// on the Rust side + /// Invoke a callback interface method that can throw on the foreign side / returns a + /// `Result<>` on the Rust side pub fn invoke_callback_with_error( &self, handle: u64, @@ -279,26 +278,17 @@ impl ForeignCallbackInternals { let mut ret_rbuf = RustBuffer::new(); let callback_result = self.call_callback(handle, method, args, &mut ret_rbuf); match callback_result { - 1 => { - // 1 indicates success with the return value written to the RustBuffer for - // non-void calls. + CALLBACK_SUCCESS => { let vec = ret_rbuf.destroy_into_vec(); let mut ret_buf = vec.as_slice(); Ok(R::try_read(&mut ret_buf).unwrap()) } - -2 => { - // -2 indicates an error written to the RustBuffer + CALLBACK_ERROR => { let vec = ret_rbuf.destroy_into_vec(); let mut ret_buf = vec.as_slice(); Err(E::try_read(&mut ret_buf).unwrap()) } - // 0 is a deprecated method to indicates success for void returns - 0 => { - log::error!("UniFFI: Callback interface returned 0. Please update the bindings code to return 1 for all successful calls"); - panic!("Callback returned 0 when we were expecting a return value"); - } - // -1 indicates an unexpected error - -1 => { + CALLBACK_UNEXPECTED_ERROR => { let reason = if !ret_rbuf.is_empty() { match >::try_lift(ret_rbuf) { Ok(s) => s, @@ -323,17 +313,9 @@ impl ForeignCallbackInternals { let mut ret_rbuf = RustBuffer::new(); let callback_result = self.call_callback(handle, method, args, &mut ret_rbuf); match callback_result { - // 1 indicates success - 1 => {} - -2 => { - panic!("Callback return -2, but no Error was expected") - } - // 0 is a deprecated method to indicates success for void returns - 0 => { - log::error!("UniFFI: Callback interface returned 0. Please update the bindings code to return 1 for all successful calls"); - } - // -1 indicates an unexpected error - -1 => { + CALLBACK_SUCCESS => {} + CALLBACK_ERROR => panic!("Callback return -2, but no Error was expected"), + CALLBACK_UNEXPECTED_ERROR => { let reason = if !ret_rbuf.is_empty() { match >::try_lift(ret_rbuf) { Ok(s) => s,