From 50a5902ac56fab0d2bc716efda12552a5d2c8861 Mon Sep 17 00:00:00 2001 From: Ben Dean-Kawamura Date: Wed, 1 Mar 2023 15:18:05 -0500 Subject: [PATCH 01/10] Adding benchmark tests - Added the `benchmarks` fixture. This runs a set of benchmark tests and uses `criterion` to analyze the results. Added some benchmarks for calling functions and callback interfaces. - Added `run_script()` function, which is a more generic version of `run_test()`. - uniffi_testing: Don't add `--test` when running `cargo build` to find the dylibs. It's not needed for the fixture/example tests and it breaks the benchmark tests. --- Cargo.toml | 1 + fixtures/benchmarks/Cargo.toml | 27 ++++++ fixtures/benchmarks/README.md | 17 ++++ fixtures/benchmarks/benches/benchmarks.rs | 48 ++++++++++ .../benches/bindings/run_benchmarks.kts | 41 ++++++++ .../benches/bindings/run_benchmarks.py | 35 +++++++ .../benches/bindings/run_benchmarks.swift | 52 +++++++++++ fixtures/benchmarks/build.rs | 7 ++ fixtures/benchmarks/src/benchmarks.udl | 40 ++++++++ fixtures/benchmarks/src/cli.rs | 88 ++++++++++++++++++ fixtures/benchmarks/src/lib.rs | 93 +++++++++++++++++++ uniffi_bindgen/src/bindings/kotlin/mod.rs | 2 +- uniffi_bindgen/src/bindings/kotlin/test.rs | 69 ++++++++++---- uniffi_bindgen/src/bindings/mod.rs | 7 ++ uniffi_bindgen/src/bindings/python/mod.rs | 2 +- uniffi_bindgen/src/bindings/python/test.rs | 42 ++++++--- uniffi_bindgen/src/bindings/ruby/mod.rs | 2 +- uniffi_bindgen/src/bindings/ruby/test.rs | 44 +++++---- uniffi_bindgen/src/bindings/swift/mod.rs | 2 +- uniffi_bindgen/src/bindings/swift/test.rs | 57 +++++++++--- uniffi_testing/src/lib.rs | 1 - 21 files changed, 606 insertions(+), 71 deletions(-) create mode 100644 fixtures/benchmarks/Cargo.toml create mode 100644 fixtures/benchmarks/README.md create mode 100644 fixtures/benchmarks/benches/benchmarks.rs create mode 100644 fixtures/benchmarks/benches/bindings/run_benchmarks.kts create mode 100644 fixtures/benchmarks/benches/bindings/run_benchmarks.py create mode 100644 fixtures/benchmarks/benches/bindings/run_benchmarks.swift create mode 100644 fixtures/benchmarks/build.rs create mode 100644 fixtures/benchmarks/src/benchmarks.udl create mode 100644 fixtures/benchmarks/src/cli.rs create mode 100644 fixtures/benchmarks/src/lib.rs diff --git a/Cargo.toml b/Cargo.toml index 853406bd71..ac20cc0fa9 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -18,6 +18,7 @@ members = [ "examples/custom-types", "examples/app/uniffi-bindgen-cli", + "fixtures/benchmarks", "fixtures/coverall", "fixtures/callbacks", diff --git a/fixtures/benchmarks/Cargo.toml b/fixtures/benchmarks/Cargo.toml new file mode 100644 index 0000000000..ad2329ae81 --- /dev/null +++ b/fixtures/benchmarks/Cargo.toml @@ -0,0 +1,27 @@ +[package] +name = "uniffi-fixture-benchmarks" +edition = "2021" +version = "0.22.0" +authors = ["Firefox Sync Team "] +license = "MPL-2.0" +publish = false + +[lib] +crate-type = ["lib", "cdylib"] +name = "uniffi_benchmarks" +bench = false + +[dependencies] +uniffi = {path = "../../uniffi"} +clap = { version = "3.1", features = ["cargo", "std", "derive"] } +criterion = "0.4.0" + +[build-dependencies] +uniffi = {path = "../../uniffi", features = ["build"] } + +[dev-dependencies] +uniffi_bindgen = {path = "../../uniffi_bindgen"} + +[[bench]] +name = "benchmarks" +harness = false diff --git a/fixtures/benchmarks/README.md b/fixtures/benchmarks/README.md new file mode 100644 index 0000000000..482d650318 --- /dev/null +++ b/fixtures/benchmarks/README.md @@ -0,0 +1,17 @@ +This fixture runs a set of benchmark tests, using criterion to test the performance. + +Benchmarking UniFFI is tricky and involves a bit of ping-pong between Rust and +the foreign language: + + - `benchmarks.rs` is the top-level Rust executuble where the process starts. + It parses the CLI arguments and determines which languages we want to run + the benchmarks for. + - `benchmarks.rs` executes a script for each foreign language that we want to benchmark. + - Those scripts call the `run_benchmarks()` function from `lib.rs` + - `run_benchmarks()` parses the CLI arguments again, this time to how to setup + the `Criterion` object. + - Testing callback interfaces is relatively straightforward, we simply invoke + the callback method. + - Testing regular functions requires some extra care, since these are called + by the foreign bindings. To test those, `benchmarks.rs` invokes a callback + interface method that call the rust function and reports the time taken. diff --git a/fixtures/benchmarks/benches/benchmarks.rs b/fixtures/benchmarks/benches/benchmarks.rs new file mode 100644 index 0000000000..eba996291c --- /dev/null +++ b/fixtures/benchmarks/benches/benchmarks.rs @@ -0,0 +1,48 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +use clap::Parser; +use std::env; +use uniffi_benchmarks::Args; +use uniffi_bindgen::bindings::{kotlin, python, swift, RunScriptMode}; + +fn main() { + let args = Args::parse(); + let script_args: Vec = std::iter::once(String::from("--")) + .chain(env::args()) + .collect(); + + if args.should_run_python() { + python::run_script( + std::env!("CARGO_TARGET_TMPDIR"), + "uniffi-fixture-benchmarks", + "benches/bindings/run_benchmarks.py", + script_args.clone(), + RunScriptMode::PerformanceTest, + ) + .unwrap() + } + + if args.should_run_kotlin() { + kotlin::run_script( + std::env!("CARGO_TARGET_TMPDIR"), + "uniffi-fixture-benchmarks", + "benches/bindings/run_benchmarks.kts", + script_args.clone(), + RunScriptMode::PerformanceTest, + ) + .unwrap() + } + + if args.should_run_swift() { + swift::run_script( + std::env!("CARGO_TARGET_TMPDIR"), + "uniffi-fixture-benchmarks", + "benches/bindings/run_benchmarks.swift", + script_args.clone(), + RunScriptMode::PerformanceTest, + ) + .unwrap() + } +} diff --git a/fixtures/benchmarks/benches/bindings/run_benchmarks.kts b/fixtures/benchmarks/benches/bindings/run_benchmarks.kts new file mode 100644 index 0000000000..d11bfffbf1 --- /dev/null +++ b/fixtures/benchmarks/benches/bindings/run_benchmarks.kts @@ -0,0 +1,41 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +import uniffi.benchmarks.* +import kotlin.system.measureNanoTime + +class TestCallbackObj : TestCallbackInterface { + override fun testMethod(a: Int, b: Int, data: TestData): String { + return data.bar; + } + + override fun testVoidReturn(a: Int, b: Int, data: TestData) { + } + + override fun testNoArgsVoidReturn() { + } + + override fun runTest(testCase: TestCase, count: ULong): ULong { + val data = TestData("StringOne", "StringTwo") + return when (testCase) { + TestCase.FUNCTION -> measureNanoTime { + for (i in 0UL..count) { + testFunction(10, 20, data) + } + } + TestCase.VOID_RETURN -> measureNanoTime { + for (i in 0UL..count) { + testVoidReturn(10, 20, data) + } + } + TestCase.NO_ARGS_VOID_RETURN -> measureNanoTime { + for (i in 0UL..count) { + testNoArgsVoidReturn() + } + } + }.toULong() + } +} + +runBenchmarks("kotlin", TestCallbackObj()) diff --git a/fixtures/benchmarks/benches/bindings/run_benchmarks.py b/fixtures/benchmarks/benches/bindings/run_benchmarks.py new file mode 100644 index 0000000000..ce2d22ff4d --- /dev/null +++ b/fixtures/benchmarks/benches/bindings/run_benchmarks.py @@ -0,0 +1,35 @@ +# This Source Code Form is subject to the terms of the Mozilla Public +# License, v. 2.0. If a copy of the MPL was not distributed with this +# file, You can obtain one at http://mozilla.org/MPL/2.0/. + +from benchmarks import * +import time + +class TestCallbackObj: + def test_method(self, a, b, data): + return data.bar + + def test_void_return(self, a, b, data): + pass + + def test_no_args_void_return(self): + pass + + def run_test(self, test_case, count): + data = TestData("StringOne", "StringTwo") + if test_case == TestCase.FUNCTION: + start = time.perf_counter_ns() + for i in range(count): + test_function(10, 20, data) + elif test_case == TestCase.VOID_RETURN: + start = time.perf_counter_ns() + for i in range(count): + test_void_return(10, 20, data) + elif test_case == TestCase.NO_ARGS_VOID_RETURN: + start = time.perf_counter_ns() + for i in range(count): + test_no_args_void_return() + end = time.perf_counter_ns() + return end - start + +run_benchmarks("python", TestCallbackObj()) diff --git a/fixtures/benchmarks/benches/bindings/run_benchmarks.swift b/fixtures/benchmarks/benches/bindings/run_benchmarks.swift new file mode 100644 index 0000000000..6404e918df --- /dev/null +++ b/fixtures/benchmarks/benches/bindings/run_benchmarks.swift @@ -0,0 +1,52 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +#if canImport(benchmarks) + import benchmarks +#endif + +#if os(Linux) + import Glibc +#else + import Darwin.C +#endif + +class TestCallbackObj: TestCallbackInterface { + func testMethod(a: Int32, b: Int32, data: TestData) -> String { + return data.bar + } + + func testVoidReturn(a: Int32, b: Int32, data: TestData) { + } + + func testNoArgsVoidReturn() { + } + + func runTest(testCase: TestCase, count: UInt64) -> UInt64 { + let data = TestData(foo: "StringOne", bar: "StringTwo") + let start: clock_t + switch testCase { + case TestCase.function: + start = clock() + for _ in 0...count { + testFunction(a: 10, b: 20, data: data) + } + case TestCase.voidReturn: + start = clock() + for _ in 0...count { + testVoidReturn(a: 10, b: 20, data: data) + } + + case TestCase.noArgsVoidReturn: + start = clock() + for _ in 0...count { + testNoArgsVoidReturn() + } + } + let end = clock() + return UInt64((end - start) * 1000000000 / CLOCKS_PER_SEC) + } +} + +runBenchmarks(languageName: "swift", cb: TestCallbackObj()) diff --git a/fixtures/benchmarks/build.rs b/fixtures/benchmarks/build.rs new file mode 100644 index 0000000000..6b5056de75 --- /dev/null +++ b/fixtures/benchmarks/build.rs @@ -0,0 +1,7 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +fn main() { + uniffi::generate_scaffolding("./src/benchmarks.udl").unwrap(); +} diff --git a/fixtures/benchmarks/src/benchmarks.udl b/fixtures/benchmarks/src/benchmarks.udl new file mode 100644 index 0000000000..9c48b8192a --- /dev/null +++ b/fixtures/benchmarks/src/benchmarks.udl @@ -0,0 +1,40 @@ +namespace benchmarks { + // Run all benchmarks and print the results to stdout + void run_benchmarks(string language_name, TestCallbackInterface cb); + + // Test functions + // + // These are intented to test the overhead of Rust function calls including: + // popping arguments from the stack, unpacking RustBuffers, pushing return + // values back to the stack, etc. + + string test_function(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(); +}; + +dictionary TestData { + string foo; + string bar; +}; + +enum TestCase { + "Function", + "VoidReturn", + "NoArgsVoidReturn", +}; + +callback interface TestCallbackInterface { + // Test callback methods. + // + // These are intented to test the overhead of callback interface calls + // 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(); + + // 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/cli.rs b/fixtures/benchmarks/src/cli.rs new file mode 100644 index 0000000000..b8447d6d85 --- /dev/null +++ b/fixtures/benchmarks/src/cli.rs @@ -0,0 +1,88 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +use clap::Parser; +use criterion::Criterion; + +#[derive(Parser, Debug)] +pub struct Args { + // Args to select which test scripts run. These are handled in `benchmarks.rs`. + /// Run Python tests + #[clap(short, long = "py", display_order = 0)] + pub python: bool, + /// Run Kotlin tests + #[clap(short, long = "kt", display_order = 0)] + pub kotlin: bool, + /// Run Swift tests + #[clap(short, long, display_order = 0)] + pub swift: bool, + + /// Dump compiler output to the console. Good for debugging new benchmarks. + #[clap(long, display_order = 1, action)] + pub compiler_messages: bool, + + // Args for running the metrics, these are handled in `lib.rs` + /// Skip benchmarks whose names do not contain FILTER + #[clap()] + pub filter: Option, + + // It would be great to also support the baseline arguments, but there doesn't seem to be any + // way to manually set those. + + // Ignore the `--bench` arg, which Cargo passes to us + #[clap(long, hide = true)] + bench: bool, +} + +impl Args { + /// Should we run the Python tests? + pub fn should_run_python(&self) -> bool { + self.python || self.no_languages_selected() + } + + /// Should we run the Kotlin tests? + pub fn should_run_kotlin(&self) -> bool { + self.kotlin || self.no_languages_selected() + } + + /// Should we run the Swift tests? + pub fn should_run_swift(&self) -> bool { + self.swift || self.no_languages_selected() + } + + pub fn no_languages_selected(&self) -> bool { + !(self.python || self.kotlin || self.swift) + } + + /// Parse arguments for run_benchmarks() + /// + /// This is slightly tricky, because run_benchmarks() is called from the foreign bindings side. + /// This means that `sys::env::args()` will contain all the arguments needed to run the foreign + /// bindings script, and we don't want to parse all of that. + pub fn parse_for_run_benchmarks() -> Self { + Self::parse_from( + std::env::args() + .into_iter() + // This method finds the first "--" arg, which `benchmarks.rs` inserts to mark the start of + // our arguments. + .skip_while(|a| a != "--") + // Skip over any "--" args. This is mainly for the "--" arg that we use to separate + // our args. However, it's also needed to workaround some kotlinc behavior. We need + // to pass it the "--" arg to get it to start trying to parse our arguments, but + // kotlinc still passes "--" back to us. + .skip_while(|a| a == "--") + .collect::>(), + ) + } + + /// Build a Criterion instance from the arguments + pub fn build_criterion(&self) -> Criterion { + let mut c = Criterion::default(); + c = match &self.filter { + Some(f) => c.with_filter(f), + None => c, + }; + c + } +} diff --git a/fixtures/benchmarks/src/lib.rs b/fixtures/benchmarks/src/lib.rs new file mode 100644 index 0000000000..624f0549a0 --- /dev/null +++ b/fixtures/benchmarks/src/lib.rs @@ -0,0 +1,93 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +use std::env; +use std::time::Duration; + +mod cli; +pub use cli::Args; + +pub struct TestData { + foo: String, + bar: String, +} + +pub enum TestCase { + Function, + VoidReturn, + NoArgsVoidReturn, +} + +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 run_test(&self, test_case: TestCase, count: u64) -> u64; +} + +pub fn test_function(_a: i32, _b: i32, data: TestData) -> String { + data.bar +} +pub fn test_void_return(_a: i32, _b: i32, _data: TestData) {} +pub fn test_no_args_void_return() {} + +pub fn run_benchmarks(language: String, cb: Box) { + let args = Args::parse_for_run_benchmarks(); + let mut c = args.build_criterion(); + + c.benchmark_group("calls") + // FFI Function call benchmarks + // + // Note: these are more a proof-of-concept than real benchmarks. Before using these to + // drive changes, make sure to take some time and double check that they're testing the + // correct things. + .bench_function(format!("{language}-functions-basic"), |b| { + b.iter_custom(|count| Duration::from_nanos(cb.run_test(TestCase::Function, count))) + }) + .bench_function(format!("{language}-functions-void-return"), |b| { + b.iter_custom(|count| Duration::from_nanos(cb.run_test(TestCase::VoidReturn, count))) + }) + .bench_function(format!("{language}-functions-no-args-void-return"), |b| { + b.iter_custom(|count| { + Duration::from_nanos(cb.run_test(TestCase::NoArgsVoidReturn, count)) + }) + }); + + c.benchmark_group("callbacks") + // These benchmarks are extra noisy, take extra time to measure them and set a higher noise + // threshold + .measurement_time(Duration::from_secs(10)) + .noise_threshold(0.05) + .bench_function(format!("{language}-callbacks-basic"), |b| { + b.iter(|| { + cb.test_method( + 10, + 100, + TestData { + foo: String::from("SomeStringData"), + bar: String::from("SomeMoreStringData"), + }, + ) + }) + }) + .bench_function(format!("{language}-callbacks-void-return"), |b| { + b.iter(|| { + cb.test_void_return( + 10, + 100, + TestData { + foo: String::from("SomeStringData"), + bar: String::from("SomeMoreStringData"), + }, + ) + }) + }) + .bench_function(format!("{language}-callbacks-no-args-void-return"), |b| { + b.iter(|| cb.test_no_args_void_return()) + }); + + c.final_summary(); +} + +include!(concat!(env!("OUT_DIR"), "/benchmarks.uniffi.rs")); diff --git a/uniffi_bindgen/src/bindings/kotlin/mod.rs b/uniffi_bindgen/src/bindings/kotlin/mod.rs index 7f22b330e3..fd60dfd09e 100644 --- a/uniffi_bindgen/src/bindings/kotlin/mod.rs +++ b/uniffi_bindgen/src/bindings/kotlin/mod.rs @@ -12,7 +12,7 @@ pub use gen_kotlin::{generate_bindings, Config}; mod test; use super::super::interface::ComponentInterface; -pub use test::run_test; +pub use test::{run_script, run_test}; pub fn write_bindings( config: &Config, diff --git a/uniffi_bindgen/src/bindings/kotlin/test.rs b/uniffi_bindgen/src/bindings/kotlin/test.rs index f62019a3b3..a60a421583 100644 --- a/uniffi_bindgen/src/bindings/kotlin/test.rs +++ b/uniffi_bindgen/src/bindings/kotlin/test.rs @@ -2,6 +2,7 @@ License, v. 2.0. If a copy of the MPL was not distributed with this * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ +use crate::bindings::RunScriptMode; use anyhow::{bail, Context, Result}; use camino::{Utf8Path, Utf8PathBuf}; use std::env; @@ -10,19 +11,34 @@ use uniffi_testing::UniFFITestHelper; /// Run Kotlin tests for a UniFFI test fixture pub fn run_test(tmp_dir: &str, fixture_name: &str, script_file: &str) -> Result<()> { + run_script( + tmp_dir, + fixture_name, + script_file, + vec![], + RunScriptMode::Test, + ) +} + +/// Run a Kotlin script +/// +/// This function will set things up so that the script can import the UniFFI bindings for a crate +pub fn run_script( + tmp_dir: &str, + crate_name: &str, + script_file: &str, + args: Vec, + mode: RunScriptMode, +) -> Result<()> { let script_path = Utf8Path::new(".").join(script_file); - let test_helper = UniFFITestHelper::new(fixture_name).context("UniFFITestHelper::new")?; - let out_dir = test_helper - .create_out_dir(tmp_dir, &script_path) - .context("create_out_dir")?; - test_helper - .copy_cdylibs_to_out_dir(&out_dir) - .context("copy_fixture_library_to_out_dir")?; - generate_sources(&test_helper.cdylib_path()?, &out_dir, &test_helper) - .context("generate_sources")?; - let jar_file = build_jar(fixture_name, &out_dir).context("build_jar")?; + let test_helper = UniFFITestHelper::new(crate_name)?; + let out_dir = test_helper.create_out_dir(tmp_dir, &script_path)?; + test_helper.copy_cdylibs_to_out_dir(&out_dir)?; + generate_sources(&test_helper.cdylib_path()?, &out_dir, &test_helper)?; + let jar_file = build_jar(crate_name, &out_dir, mode)?; - let status = Command::new("kotlinc") + let mut command = kotlinc_command(mode); + command .arg("-classpath") .arg(calc_classpath(vec![&out_dir, &jar_file])) // Enable runtime assertions, for easy testing etc. @@ -31,6 +47,13 @@ pub fn run_test(tmp_dir: &str, fixture_name: &str, script_file: &str) -> Result< .arg("-Werror") .arg("-script") .arg(script_path) + .args(if args.is_empty() { + vec![] + } else { + std::iter::once(String::from("--")).chain(args).collect() + }); + + let status = command .spawn() .context("Failed to spawn `kotlinc` to run Kotlin script")? .wait() @@ -47,10 +70,6 @@ fn generate_sources( test_helper: &UniFFITestHelper, ) -> Result<()> { for source in test_helper.get_compile_sources()? { - println!( - "Generating bindings: {:?} {:?} {:?}", - source.udl_path, source.config_path, out_dir - ); crate::generate_bindings( &source.udl_path, source.config_path.as_deref(), @@ -65,9 +84,9 @@ fn generate_sources( /// Generate kotlin bindings for the given namespace, then use the kotlin /// command-line tools to compile them into a .jar file. -fn build_jar(fixture_name: &str, out_dir: &Utf8Path) -> Result { +fn build_jar(crate_name: &str, out_dir: &Utf8Path, mode: RunScriptMode) -> Result { let mut jar_file = Utf8PathBuf::from(out_dir); - jar_file.push(format!("{fixture_name}.jar")); + jar_file.push(format!("{crate_name}.jar")); let sources = glob::glob(out_dir.join("**/*.kt").as_str())? .flatten() .map(|p| String::from(p.to_string_lossy())) @@ -75,16 +94,18 @@ fn build_jar(fixture_name: &str, out_dir: &Utf8Path) -> Result { if sources.is_empty() { bail!("No kotlin sources found in {out_dir}") } - println!("building jar from: {sources:?}"); - let status = Command::new("kotlinc") + let mut command = kotlinc_command(mode); + command // Our generated bindings should not produce any warnings; fail tests if they do. .arg("-Werror") .arg("-d") .arg(&jar_file) .arg("-classpath") .arg(calc_classpath(vec![])) - .args(sources) + .args(sources); + + let status = command .spawn() .context("Failed to spawn `kotlinc` to compile the bindings")? .wait() @@ -95,6 +116,14 @@ fn build_jar(fixture_name: &str, out_dir: &Utf8Path) -> Result { Ok(jar_file) } +fn kotlinc_command(mode: RunScriptMode) -> Command { + let mut command = Command::new("kotlinc"); + if let RunScriptMode::PerformanceTest = mode { + command.arg("-nowarn"); + } + command +} + fn calc_classpath(extra_paths: Vec<&Utf8Path>) -> String { extra_paths .into_iter() diff --git a/uniffi_bindgen/src/bindings/mod.rs b/uniffi_bindgen/src/bindings/mod.rs index 81210612f5..6987b4205f 100644 --- a/uniffi_bindgen/src/bindings/mod.rs +++ b/uniffi_bindgen/src/bindings/mod.rs @@ -33,6 +33,13 @@ pub enum TargetLanguage { Ruby, } +/// Mode for the `run_script` function defined for each language +#[derive(Copy, Clone, Debug)] +pub enum RunScriptMode { + Test, + PerformanceTest, +} + impl TryFrom<&str> for TargetLanguage { type Error = anyhow::Error; fn try_from(value: &str) -> Result { diff --git a/uniffi_bindgen/src/bindings/python/mod.rs b/uniffi_bindgen/src/bindings/python/mod.rs index a71c2397b3..9c5d0cb185 100644 --- a/uniffi_bindgen/src/bindings/python/mod.rs +++ b/uniffi_bindgen/src/bindings/python/mod.rs @@ -13,7 +13,7 @@ mod test; use super::super::interface::ComponentInterface; pub use gen_python::{generate_python_bindings, Config}; -pub use test::run_test; +pub use test::{run_script, run_test}; // Generate python bindings for the given ComponentInterface, in the given output directory. pub fn write_bindings( diff --git a/uniffi_bindgen/src/bindings/python/test.rs b/uniffi_bindgen/src/bindings/python/test.rs index d050fc0389..3321552ff8 100644 --- a/uniffi_bindgen/src/bindings/python/test.rs +++ b/uniffi_bindgen/src/bindings/python/test.rs @@ -2,37 +2,53 @@ License, v. 2.0. If a copy of the MPL was not distributed with this * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ +use crate::bindings::RunScriptMode; use anyhow::{Context, Result}; use camino::Utf8Path; use std::env; use std::ffi::OsString; -use std::process::{Command, Stdio}; +use std::process::Command; use uniffi_testing::UniFFITestHelper; /// Run Python tests for a UniFFI test fixture pub fn run_test(tmp_dir: &str, fixture_name: &str, script_file: &str) -> Result<()> { + run_script( + tmp_dir, + fixture_name, + script_file, + vec![], + RunScriptMode::Test, + ) +} + +/// Run a Python script +/// +/// This function will set things up so that the script can import the UniFFI bindings for a crate +pub fn run_script( + tmp_dir: &str, + crate_name: &str, + script_file: &str, + args: Vec, + _mode: RunScriptMode, +) -> Result<()> { let script_path = Utf8Path::new(".").join(script_file).canonicalize_utf8()?; - let test_helper = UniFFITestHelper::new(fixture_name).context("UniFFITestHelper::new")?; - let out_dir = test_helper - .create_out_dir(tmp_dir, &script_path) - .context("create_out_dir")?; - test_helper - .copy_cdylibs_to_out_dir(&out_dir) - .context("copy_cdylibs_to_out_dir")?; - generate_sources(&test_helper.cdylib_path()?, &out_dir, &test_helper) - .context("generate_sources")?; + let test_helper = UniFFITestHelper::new(crate_name)?; + let out_dir = test_helper.create_out_dir(tmp_dir, &script_path)?; + test_helper.copy_cdylibs_to_out_dir(&out_dir)?; + generate_sources(&test_helper.cdylib_path()?, &out_dir, &test_helper)?; let pythonpath = env::var_os("PYTHONPATH").unwrap_or_else(|| OsString::from("")); let pythonpath = env::join_paths( env::split_paths(&pythonpath).chain(vec![out_dir.to_path_buf().into_std_path_buf()]), )?; - let status = Command::new("python3") + let mut command = Command::new("python3"); + command .current_dir(out_dir) .env("PYTHONPATH", pythonpath) .arg(script_path) - .stderr(Stdio::inherit()) - .stdout(Stdio::inherit()) + .args(args); + let status = command .spawn() .context("Failed to spawn `python3` when running script")? .wait() diff --git a/uniffi_bindgen/src/bindings/ruby/mod.rs b/uniffi_bindgen/src/bindings/ruby/mod.rs index 2a92eb6f5d..498e4c46a5 100644 --- a/uniffi_bindgen/src/bindings/ruby/mod.rs +++ b/uniffi_bindgen/src/bindings/ruby/mod.rs @@ -11,7 +11,7 @@ use fs_err::File; pub mod gen_ruby; mod test; pub use gen_ruby::{Config, RubyWrapper}; -pub use test::run_test; +pub use test::{run_test, test_script_command}; use super::super::interface::ComponentInterface; diff --git a/uniffi_bindgen/src/bindings/ruby/test.rs b/uniffi_bindgen/src/bindings/ruby/test.rs index 5c445bfae5..2c3f443500 100644 --- a/uniffi_bindgen/src/bindings/ruby/test.rs +++ b/uniffi_bindgen/src/bindings/ruby/test.rs @@ -11,36 +11,42 @@ use uniffi_testing::UniFFITestHelper; /// Run Ruby tests for a UniFFI test fixture pub fn run_test(tmp_dir: &str, fixture_name: &str, script_file: &str) -> Result<()> { + let status = test_script_command(tmp_dir, fixture_name, script_file)? + .spawn() + .context("Failed to spawn `ruby` when running script")? + .wait() + .context("Failed to wait for `ruby` when running script")?; + if !status.success() { + bail!("running `ruby` failed"); + } + Ok(()) +} + +/// Create a `Command` instance that runs a test script +pub fn test_script_command( + tmp_dir: &str, + fixture_name: &str, + script_file: &str, +) -> Result { let script_path = Utf8Path::new(".").join(script_file).canonicalize_utf8()?; - let test_helper = UniFFITestHelper::new(fixture_name).context("UniFFITestHelper::new")?; - let out_dir = test_helper - .create_out_dir(tmp_dir, &script_path) - .context("create_out_dir")?; - test_helper - .copy_cdylibs_to_out_dir(&out_dir) - .context("copy_cdylibs_to_out_dir")?; - generate_sources(&test_helper.cdylib_path()?, &out_dir, &test_helper) - .context("generate_sources")?; + let test_helper = UniFFITestHelper::new(fixture_name)?; + let out_dir = test_helper.create_out_dir(tmp_dir, &script_path)?; + test_helper.copy_cdylibs_to_out_dir(&out_dir)?; + generate_sources(&test_helper.cdylib_path()?, &out_dir, &test_helper)?; let rubypath = env::var_os("RUBYLIB").unwrap_or_else(|| OsString::from("")); let rubypath = env::join_paths( env::split_paths(&rubypath).chain(vec![out_dir.to_path_buf().into_std_path_buf()]), )?; - let status = Command::new("ruby") + let mut command = Command::new("ruby"); + command .current_dir(out_dir) .env("RUBYLIB", rubypath) .arg(script_path) .stderr(Stdio::inherit()) - .stdout(Stdio::inherit()) - .spawn() - .context("Failed to spawn `ruby` when running script")? - .wait() - .context("Failed to wait for `ruby` when running script")?; - if !status.success() { - bail!("running `ruby` failed"); - } - Ok(()) + .stdout(Stdio::inherit()); + Ok(command) } fn generate_sources( diff --git a/uniffi_bindgen/src/bindings/swift/mod.rs b/uniffi_bindgen/src/bindings/swift/mod.rs index 61312631a0..90703814aa 100644 --- a/uniffi_bindgen/src/bindings/swift/mod.rs +++ b/uniffi_bindgen/src/bindings/swift/mod.rs @@ -40,7 +40,7 @@ pub use gen_swift::{generate_bindings, Config}; mod test; use super::super::interface::ComponentInterface; -pub use test::run_test; +pub use test::{run_script, run_test}; /// The Swift bindings generated from a [`ComponentInterface`]. /// diff --git a/uniffi_bindgen/src/bindings/swift/test.rs b/uniffi_bindgen/src/bindings/swift/test.rs index 1ed171a944..93de1aaff9 100644 --- a/uniffi_bindgen/src/bindings/swift/test.rs +++ b/uniffi_bindgen/src/bindings/swift/test.rs @@ -2,6 +2,7 @@ License, v. 2.0. If a copy of the MPL was not distributed with this * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ +use crate::bindings::RunScriptMode; use anyhow::{bail, Context, Result}; use camino::{Utf8Path, Utf8PathBuf}; use heck::ToSnakeCase; @@ -9,22 +10,36 @@ use std::env::consts::{DLL_PREFIX, DLL_SUFFIX}; use std::ffi::OsStr; use std::fs::{read_to_string, File}; use std::io::Write; -use std::process::Command; +use std::process::{Command, Stdio}; use uniffi_testing::{CompileSource, UniFFITestHelper}; /// Run Swift tests for a UniFFI test fixture pub fn run_test(tmp_dir: &str, fixture_name: &str, script_file: &str) -> Result<()> { + run_script( + tmp_dir, + fixture_name, + script_file, + vec![], + RunScriptMode::Test, + ) +} + +/// Run a Swift script +/// +/// This function will set things up so that the script can import the UniFFI bindings for a crate +pub fn run_script( + tmp_dir: &str, + crate_name: &str, + script_file: &str, + args: Vec, + mode: RunScriptMode, +) -> Result<()> { let script_path = Utf8Path::new(".").join(script_file).canonicalize_utf8()?; - let test_helper = UniFFITestHelper::new(fixture_name).context("UniFFITestHelper::new")?; - let out_dir = test_helper - .create_out_dir(tmp_dir, &script_path) - .context("create_out_dir()")?; - test_helper - .copy_cdylibs_to_out_dir(&out_dir) - .context("copy_fixture_library_to_out_dir()")?; + let test_helper = UniFFITestHelper::new(crate_name)?; + let out_dir = test_helper.create_out_dir(tmp_dir, &script_path)?; + test_helper.copy_cdylibs_to_out_dir(&out_dir)?; let generated_sources = - GeneratedSources::new(&test_helper.cdylib_path()?, &out_dir, &test_helper) - .context("generate_sources()")?; + GeneratedSources::new(&test_helper.cdylib_path()?, &out_dir, &test_helper)?; // Compile the generated sources together to create a single swift module compile_swift_module( @@ -32,11 +47,11 @@ pub fn run_test(tmp_dir: &str, fixture_name: &str, script_file: &str) -> Result< &calc_module_name(&generated_sources.main_source_filename), &generated_sources.generated_swift_files, &generated_sources.module_map, + mode, )?; // Run the test script against compiled bindings - - let mut command = Command::new("swift"); + let mut command = create_command("swift", mode); command .current_dir(&out_dir) .arg("-I") @@ -49,7 +64,8 @@ pub fn run_test(tmp_dir: &str, fixture_name: &str, script_file: &str) -> Result< "-fmodule-map-file={}", generated_sources.module_map )) - .arg(&script_path); + .arg(&script_path) + .args(args); let status = command .spawn() .context("Failed to spawn `swiftc` when running test script")? @@ -70,9 +86,10 @@ fn compile_swift_module>( module_name: &str, sources: impl IntoIterator, module_map: &Utf8Path, + mode: RunScriptMode, ) -> Result<()> { let output_filename = format!("{DLL_PREFIX}testmod_{module_name}{DLL_SUFFIX}"); - let mut command = Command::new("swiftc"); + let mut command = create_command("swiftc", mode); command .current_dir(out_dir) .arg("-emit-module") @@ -195,6 +212,18 @@ impl GeneratedSources { } } +fn create_command(program: &str, mode: RunScriptMode) -> Command { + let mut command = Command::new(program); + if let RunScriptMode::PerformanceTest = mode { + // This prevents most compiler messages, but not remarks + command.arg("-suppress-warnings"); + // This gets the remorks. Note: swift will eventually get a `-supress-remarks` argument, + // maybe we can eventually move to that + command.stderr(Stdio::null()); + } + command +} + // Wraps glob to use Utf8Paths and flattens errors fn glob(globspec: &Utf8Path) -> Result> { glob::glob(globspec.as_str())? diff --git a/uniffi_testing/src/lib.rs b/uniffi_testing/src/lib.rs index cc08895214..1b3943a79d 100644 --- a/uniffi_testing/src/lib.rs +++ b/uniffi_testing/src/lib.rs @@ -258,7 +258,6 @@ fn get_cargo_build_messages() -> Vec { let mut child = Command::new(env!("CARGO")) .arg("build") .arg("--message-format=json") - .arg("--tests") .stdout(Stdio::piped()) .spawn() .expect("Error running cargo build"); From 98c11f997dc2ff3603f2326b3de1c49b92d7ab88 Mon Sep 17 00:00:00 2001 From: Ben Dean-Kawamura Date: Thu, 2 Mar 2023 17:10:41 -0500 Subject: [PATCH 02/10] Refactoring callback interface scaffolding code The main goal here is to make the logic live in regular, non-templated, code. This makes it easier to understand what's going on and hopefully easier to change. One issue with this approach is that we need 3 functions depending on the method return/throws types. I think it's worth it to have clearer code and I also hope to merge these into 1 function. We could use the same move as in PR#1469: define a new `FfiConverter` method like `callback_interface_method_return()` that handles the specifics then have a default implementation, with a specialized implementation for `Result<>` and `()`. --- .../templates/CallbackInterfaceTemplate.rs | 111 +++----------- uniffi_core/src/ffi/foreigncallbacks.rs | 141 +++++++++++++++++- 2 files changed, 155 insertions(+), 97 deletions(-) diff --git a/uniffi_bindgen/src/scaffolding/templates/CallbackInterfaceTemplate.rs b/uniffi_bindgen/src/scaffolding/templates/CallbackInterfaceTemplate.rs index defdf1235a..3429222ebc 100644 --- a/uniffi_bindgen/src/scaffolding/templates/CallbackInterfaceTemplate.rs +++ b/uniffi_bindgen/src/scaffolding/templates/CallbackInterfaceTemplate.rs @@ -34,9 +34,9 @@ struct {{ trait_impl }} { impl Drop for {{ trait_impl }} { fn drop(&mut self) { - let callback = {{ foreign_callback_internals }}.get_callback().unwrap(); - let mut rbuf = uniffi::RustBuffer::new(); - unsafe { callback(self.handle, uniffi::IDX_CALLBACK_FREE, Default::default(), &mut rbuf) }; + {{ foreign_callback_internals }}.invoke_callback_void_return( + self.handle, uniffi::IDX_CALLBACK_FREE, Default::default() + ) } } @@ -68,97 +68,20 @@ impl r#{{ trait_name }} for {{ trait_impl }} { let args_rbuf = uniffi::RustBuffer::from_vec(args_buf); {#- Calling into foreign code. #} - let callback = {{ foreign_callback_internals }}.get_callback().unwrap(); - - unsafe { - // SAFETY: - // * We're passing in a pointer to an empty buffer. - // * Nothing allocated, so nothing to drop. - // * We expect the callback to write into that a valid allocated instance of a - // RustBuffer. - let mut ret_rbuf = uniffi::RustBuffer::new(); - let ret = callback(self.handle, {{ loop.index }}, args_rbuf, &mut ret_rbuf); - #[allow(clippy::let_and_return, clippy::let_unit_value)] - match ret { - 1 => { - // 1 indicates success with the return value written to the RustBuffer for - // non-void calls. - let result = { - {% match meth.return_type() -%} - {%- when Some(return_type) -%} - let vec = ret_rbuf.destroy_into_vec(); - let mut ret_buf = vec.as_slice(); - {{ return_type|ffi_converter }}::try_read(&mut ret_buf).unwrap() - {%- else %} - uniffi::RustBuffer::destroy(ret_rbuf); - {%- endmatch %} - }; - {%- if meth.throws() %} - Ok(result) - {%- else %} - result - {%- endif %} - } - -2 => { - // -2 indicates an error written to the RustBuffer - {% match meth.throws_type() -%} - {% when Some(error_type) -%} - let vec = ret_rbuf.destroy_into_vec(); - let mut ret_buf = vec.as_slice(); - Err({{ error_type|ffi_converter }}::try_read(&mut ret_buf).unwrap()) - {%- else -%} - panic!("Callback return -2, but throws_type() is None"); - {%- endmatch %} - } - // 0 is a deprecated method to indicates success for void returns - 0 => { - uniffi::deps::log::error!("UniFFI: Callback interface returned 0. Please update the bindings code to return 1 for all successful calls"); - {% match (meth.return_type(), meth.throws()) %} - {% when (Some(_), _) %} - panic!("Callback returned 0 when we were expecting a return value"); - {% when (None, false) %} - {% when (None, true) %} - Ok(()) - {%- endmatch %} - } - // -1 indicates an unexpected error - {% match meth.throws_type() %} - {%- when Some(error_type) -%} - -1 => { - let reason = if !ret_rbuf.is_empty() { - match {{ Type::String.borrow()|ffi_converter }}::try_lift(ret_rbuf) { - Ok(s) => s, - Err(e) => { - uniffi::deps::log::error!("{{ trait_name }} Error reading ret_buf: {e}"); - String::from("[Error reading reason]") - } - } - } else { - uniffi::RustBuffer::destroy(ret_rbuf); - String::from("[Unknown Reason]") - }; - let e: {{ error_type|type_rs }} = uniffi::UnexpectedUniFFICallbackError::from_reason(reason).into(); - Err(e) - } - {%- else %} - -1 => { - if !ret_rbuf.is_empty() { - let reason = match {{ Type::String.borrow()|ffi_converter }}::try_lift(ret_rbuf) { - Ok(s) => s, - Err(_) => { - String::from("[Error reading reason]") - } - }; - panic!("callback failed. Reason: {reason}"); - } else { - panic!("Callback failed") - } - }, - {%- endmatch %} - // Other values should never be returned - _ => panic!("Callback failed with unexpected return code"), - } - } + {%- match (meth.return_type(), meth.throws_type()) %} + {%- when (Some(return_type), None) %} + {{ foreign_callback_internals }}.invoke_callback::<{{ return_type|type_rs }}, crate::UniFfiTag>(self.handle, {{ loop.index }}, args_rbuf) + + {%- when (Some(return_type), Some(error_type)) %} + {{ foreign_callback_internals }}.invoke_callback_with_error::<{{ return_type|type_rs }}, {{ error_type|type_rs }}, crate::UniFfiTag>(self.handle, {{ loop.index }}, args_rbuf) + + {%- when (None, Some(error_type)) %} + {{ foreign_callback_internals }}.invoke_callback_with_error::<(), {{ error_type|type_rs }}, crate::UniFfiTag>(self.handle, {{ loop.index }}, args_rbuf) + + {%- when (None, None) %} + {{ foreign_callback_internals }}.invoke_callback_void_return(self.handle, {{ loop.index }}, args_rbuf) + + {%- endmatch %} } {%- endfor %} } diff --git a/uniffi_core/src/ffi/foreigncallbacks.rs b/uniffi_core/src/ffi/foreigncallbacks.rs index 9d513353a6..54d2125c07 100644 --- a/uniffi_core/src/ffi/foreigncallbacks.rs +++ b/uniffi_core/src/ffi/foreigncallbacks.rs @@ -113,7 +113,7 @@ //! type and then returns to client code. //! -use super::RustBuffer; +use crate::{FfiConverter, RustBuffer}; use std::fmt; use std::os::raw::c_int; use std::sync::atomic::{AtomicUsize, Ordering}; @@ -195,9 +195,144 @@ impl ForeignCallbackInternals { }; } - pub fn get_callback(&self) -> Option { + pub fn call_callback(&self, handle: u64, method: u32, args: RustBuffer, ret_rbuf: &mut RustBuffer) -> c_int { let ptr_value = self.callback_ptr.load(Ordering::SeqCst); - unsafe { std::mem::transmute::>(ptr_value) } + unsafe { + let callback = std::mem::transmute::>(ptr_value) + .expect("Callback interface handler not set"); + callback(handle, method, args, ret_rbuf) + } + } + + /// 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 + where R: FfiConverter + { + 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. + 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"); + } + // -1 indicates an unexpected error + -1 => { + let reason = if !ret_rbuf.is_empty() { + match >::try_lift(ret_rbuf) { + Ok(s) => s, + Err(e) => { + log::error!("{{ trait_name }} Error reading ret_buf: {e}"); + String::from("[Error reading reason]") + } + } + } else { + RustBuffer::destroy(ret_rbuf); + String::from("[Unknown Reason]") + }; + panic!("callback failed. Reason: {reason}"); + } + // Other values should never be returned + _ => panic!("Callback failed with unexpected return code"), + } + } + + /// Invoke a callback interface method that can throw on the foreign side / returnns a Result<> + /// on the Rust side + pub fn invoke_callback_with_error(&self, handle: u64, method: u32, args: RustBuffer) -> Result + where R: FfiConverter, + E: FfiConverter, + E: From + { + 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. + 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 + 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 => { + let reason = if !ret_rbuf.is_empty() { + match >::try_lift(ret_rbuf) { + Ok(s) => s, + Err(e) => { + log::error!("{{ trait_name }} Error reading ret_buf: {e}"); + String::from("[Error reading reason]") + } + } + } else { + RustBuffer::destroy(ret_rbuf); + String::from("[Unknown Reason]") + }; + Err(UnexpectedUniFFICallbackError::from_reason(reason).into()) + } + // Other values should never be returned + _ => panic!("Callback failed with unexpected return code"), + } + } + + /// Invoke a callback interface method that returns void and doesn't throw + pub fn invoke_callback_void_return(&self, handle: u64, method: u32, args: RustBuffer) { + 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 + () + } + -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 => { + let reason = if !ret_rbuf.is_empty() { + match >::try_lift(ret_rbuf) { + Ok(s) => s, + Err(e) => { + log::error!("{{ trait_name }} Error reading ret_buf: {e}"); + String::from("[Error reading reason]") + } + } + } else { + RustBuffer::destroy(ret_rbuf); + String::from("[Unknown Reason]") + }; + panic!("callback failed. Reason: {reason}"); + } + // Other values should never be returned + _ => panic!("Callback failed with unexpected return code"), + } } } From 8ec09aaa2c7eee0703d51f300d0913592bebb85c Mon Sep 17 00:00:00 2001 From: Ben Dean-Kawamura Date: Fri, 3 Mar 2023 12:33:01 -0500 Subject: [PATCH 03/10] Started work on the new callback interface ABI MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The goal here is to lower the overhead of callback interface calls, specifically targeting things like logging calls which can be called many times in a short timespan. Since this is an ABI change, it's not backwards compatible and therefore requires a breaking version bump before it can be the default. The initial ABI change was fairly minor: the argument RustBuffer is now passed by reference instead of by value. This means the foreign code doesn't need to use an FFI call to destroy it. For Kotlin, and maybe other languages, it also seems to be slightly faster to use a reference because that's less bits that need to be pushed to/popped from the stack. This brought some speedups to python and kotlin, and a minor speedup to Swift that only passes the noise thresholdin the no-args-void-return case: callbacks/python-callbacks-basic time: [37.277 µs 37.299 µs 37.320 µs] change: [-10.692% -10.532% -10.379%] (p = 0.00 < 0.05) Performance has improved. callbacks/kotlin-callbacks-basic time: [15.533 µs 15.801 µs 16.069 µs] change: [-35.654% -31.481% -27.245%] (p = 0.00 < 0.05) Performance has improved. callbacks/swift-callbacks-no-args-void-return time: [454.96 ns 455.64 ns 456.71 ns] change: [-35.872% -35.792% -35.710%] (p = 0.00 < 0.05) Performance has improved. --- .../templates/CallbackInterfaceRuntime.kt | 2 +- .../templates/CallbackInterfaceTemplate.kt | 54 +++++++++---------- .../templates/CallbackInterfaceTemplate.py | 4 +- .../src/bindings/python/templates/Helpers.py | 2 +- .../python/templates/RustBufferTemplate.py | 13 ++++- .../swift/templates/BridgingHeaderTemplate.h | 2 +- .../templates/CallbackInterfaceTemplate.swift | 7 ++- uniffi_core/src/ffi/foreigncallbacks.rs | 31 +++++++---- 8 files changed, 67 insertions(+), 48 deletions(-) diff --git a/uniffi_bindgen/src/bindings/kotlin/templates/CallbackInterfaceRuntime.kt b/uniffi_bindgen/src/bindings/kotlin/templates/CallbackInterfaceRuntime.kt index 3f5d72a608..e7a7b4a361 100644 --- a/uniffi_bindgen/src/bindings/kotlin/templates/CallbackInterfaceRuntime.kt +++ b/uniffi_bindgen/src/bindings/kotlin/templates/CallbackInterfaceRuntime.kt @@ -35,7 +35,7 @@ internal class ConcurrentHandleMap( } interface ForeignCallback : com.sun.jna.Callback { - public fun invoke(handle: Handle, method: Int, args: RustBuffer.ByValue, outBuf: RustBufferByReference): Int + public fun invoke(handle: Handle, method: Int, args: RustBuffer.ByReference, outBuf: RustBufferByReference): Int } // Magic number for the Rust proxy to call using the same mechanism as every other method, diff --git a/uniffi_bindgen/src/bindings/kotlin/templates/CallbackInterfaceTemplate.kt b/uniffi_bindgen/src/bindings/kotlin/templates/CallbackInterfaceTemplate.kt index c7c4d2b973..031b04a548 100644 --- a/uniffi_bindgen/src/bindings/kotlin/templates/CallbackInterfaceTemplate.kt +++ b/uniffi_bindgen/src/bindings/kotlin/templates/CallbackInterfaceTemplate.kt @@ -22,7 +22,7 @@ public interface {{ type_name }} { // The ForeignCallback that is passed to Rust. internal class {{ foreign_callback }} : ForeignCallback { @Suppress("TooGenericExceptionCaught") - override fun invoke(handle: Handle, method: Int, args: RustBuffer.ByValue, outBuf: RustBufferByReference): Int { + override fun invoke(handle: Handle, method: Int, args: RustBuffer.ByReference, outBuf: RustBufferByReference): Int { val cb = {{ ffi_converter_name }}.lift(handle) return when (method) { IDX_CALLBACK_FREE -> { @@ -84,37 +84,33 @@ internal class {{ foreign_callback }} : ForeignCallback { {% for meth in cbi.methods() -%} {% let method_name = format!("invoke_{}", meth.name())|fn_name %} - private fun {{ method_name }}(kotlinCallbackInterface: {{ type_name }}, args: RustBuffer.ByValue): RustBuffer.ByValue = - try { + private fun {{ method_name }}(kotlinCallbackInterface: {{ type_name }}, @Suppress("UNUSED_PARAMETER")args: RustBuffer.ByReference): RustBuffer.ByValue { {#- Unpacking args from the RustBuffer #} - {%- if meth.arguments().len() != 0 -%} - {#- Calling the concrete callback object #} - val buf = args.asByteBuffer() ?: throw InternalException("No ByteBuffer in RustBuffer; this is a Uniffi bug") - kotlinCallbackInterface.{{ meth.name()|fn_name }}( - {% for arg in meth.arguments() -%} - {{ arg|read_fn }}(buf) - {%- if !loop.last %}, {% endif %} - {% endfor -%} - ) - {% else %} - kotlinCallbackInterface.{{ meth.name()|fn_name }}() - {% endif -%} + {%- if meth.arguments().len() != 0 -%} + {#- Calling the concrete callback object #} + val buf = args.asByteBuffer() ?: throw InternalException("No ByteBuffer in RustBuffer; this is a Uniffi bug") + return kotlinCallbackInterface.{{ meth.name()|fn_name }}( + {% for arg in meth.arguments() -%} + {{ arg|read_fn }}(buf) + {%- if !loop.last %}, {% endif %} + {% endfor -%} + ) + {% else %} + return kotlinCallbackInterface.{{ meth.name()|fn_name }}() + {% endif -%} {#- Packing up the return value into a RustBuffer #} - {%- match meth.return_type() -%} - {%- when Some with (return_type) -%} - .let { - {{ return_type|ffi_converter_name }}.lowerIntoRustBuffer(it) - } - {%- else -%} - .let { RustBuffer.ByValue() } - {% endmatch -%} - // TODO catch errors and report them back to Rust. - // https://github.com/mozilla/uniffi-rs/issues/351 - } finally { - RustBuffer.free(args) - } - + {%- match meth.return_type() -%} + {%- when Some with (return_type) -%} + .let { + {{ return_type|ffi_converter_name }}.lowerIntoRustBuffer(it) + } + {%- else -%} + .let { RustBuffer.ByValue() } + {%- endmatch %} + // TODO catch errors and report them back to Rust. + // https://github.com/mozilla/uniffi-rs/issues/351 + } {% endfor %} } diff --git a/uniffi_bindgen/src/bindings/python/templates/CallbackInterfaceTemplate.py b/uniffi_bindgen/src/bindings/python/templates/CallbackInterfaceTemplate.py index cf4411b8a0..c140481711 100644 --- a/uniffi_bindgen/src/bindings/python/templates/CallbackInterfaceTemplate.py +++ b/uniffi_bindgen/src/bindings/python/templates/CallbackInterfaceTemplate.py @@ -19,7 +19,7 @@ def {{ method_name }}(python_callback, args): {#- Unpacking args from the RustBuffer #} {%- if meth.arguments().len() != 0 -%} {#- Calling the concrete callback object #} - with args.consumeWithStream() as buf: + with args.contents.readWithStream() as buf: rval = python_callback.{{ meth.name()|fn_name }}( {% for arg in meth.arguments() -%} {{ arg|read_fn }}(buf) @@ -98,7 +98,7 @@ def {{ method_name }}(python_callback, args): # that is in freed memory. # That would be...uh...bad. Yeah, that's the word. Bad. {{ foreign_callback }} = FOREIGN_CALLBACK_T(py_{{ foreign_callback }}) +rust_call(lambda err: _UniFFILib.{{ cbi.ffi_init_callback().name() }}({{ foreign_callback }}, err)) # The FfiConverter which transforms the Callbacks in to Handles to pass to Rust. -rust_call(lambda err: _UniFFILib.{{ cbi.ffi_init_callback().name() }}({{ foreign_callback }}, err)) {{ ffi_converter_name }} = FfiConverterCallbackInterface({{ foreign_callback }}) diff --git a/uniffi_bindgen/src/bindings/python/templates/Helpers.py b/uniffi_bindgen/src/bindings/python/templates/Helpers.py index fbce501981..8afd370e63 100644 --- a/uniffi_bindgen/src/bindings/python/templates/Helpers.py +++ b/uniffi_bindgen/src/bindings/python/templates/Helpers.py @@ -64,4 +64,4 @@ def rust_call_with_error(error_ffi_converter, fn, *args): # A function pointer for a callback as defined by UniFFI. # Rust definition `fn(handle: u64, method: u32, args: RustBuffer, buf_ptr: *mut RustBuffer) -> int` -FOREIGN_CALLBACK_T = ctypes.CFUNCTYPE(ctypes.c_int, ctypes.c_ulonglong, ctypes.c_ulong, RustBuffer, ctypes.POINTER(RustBuffer)) +FOREIGN_CALLBACK_T = ctypes.CFUNCTYPE(ctypes.c_int, ctypes.c_ulonglong, ctypes.c_ulong, ctypes.POINTER(RustBuffer), ctypes.POINTER(RustBuffer)) diff --git a/uniffi_bindgen/src/bindings/python/templates/RustBufferTemplate.py b/uniffi_bindgen/src/bindings/python/templates/RustBufferTemplate.py index 95a3b1f706..b55eaf5df5 100644 --- a/uniffi_bindgen/src/bindings/python/templates/RustBufferTemplate.py +++ b/uniffi_bindgen/src/bindings/python/templates/RustBufferTemplate.py @@ -49,10 +49,21 @@ def consumeWithStream(self): s = RustBufferStream(self) yield s if s.remaining() != 0: - raise RuntimeError("junk data left in buffer after consuming") + raise RuntimeError("junk data left in buffer at end of consumeWithStream") finally: self.free() + @contextlib.contextmanager + def readWithStream(self): + """Context-manager to read a buffer using a RustBufferStream. + + This is like consumeWithStream, but doesn't free the buffer afterwards. + It should only be used with borrowed `RustBuffer` data. + """ + s = RustBufferStream(self) + yield s + if s.remaining() != 0: + raise RuntimeError("junk data left in buffer at end of readWithStream") class ForeignBytes(ctypes.Structure): _fields_ = [ diff --git a/uniffi_bindgen/src/bindings/swift/templates/BridgingHeaderTemplate.h b/uniffi_bindgen/src/bindings/swift/templates/BridgingHeaderTemplate.h index 7cfbfb0573..01bd28635c 100644 --- a/uniffi_bindgen/src/bindings/swift/templates/BridgingHeaderTemplate.h +++ b/uniffi_bindgen/src/bindings/swift/templates/BridgingHeaderTemplate.h @@ -28,7 +28,7 @@ typedef struct RustBuffer uint8_t *_Nullable data; } RustBuffer; -typedef int32_t (*ForeignCallback)(uint64_t, int32_t, RustBuffer, RustBuffer *_Nonnull); +typedef int32_t (*ForeignCallback)(uint64_t, int32_t, const RustBuffer *_Nonnull, RustBuffer *_Nonnull); typedef struct ForeignBytes { diff --git a/uniffi_bindgen/src/bindings/swift/templates/CallbackInterfaceTemplate.swift b/uniffi_bindgen/src/bindings/swift/templates/CallbackInterfaceTemplate.swift index 7842511a83..440c3008a9 100644 --- a/uniffi_bindgen/src/bindings/swift/templates/CallbackInterfaceTemplate.swift +++ b/uniffi_bindgen/src/bindings/swift/templates/CallbackInterfaceTemplate.swift @@ -16,17 +16,16 @@ public protocol {{ type_name }} : AnyObject { // The ForeignCallback that is passed to Rust. fileprivate let {{ foreign_callback }} : ForeignCallback = - { (handle: UniFFICallbackHandle, method: Int32, args: RustBuffer, out_buf: UnsafeMutablePointer) -> Int32 in + { (handle: UniFFICallbackHandle, method: Int32, args: UnsafePointer, out_buf: UnsafeMutablePointer) -> Int32 in {% for meth in cbi.methods() -%} {%- let method_name = format!("invoke_{}", meth.name())|fn_name -%} - func {{ method_name }}(_ swiftCallbackInterface: {{ type_name }}, _ args: RustBuffer) throws -> RustBuffer { - defer { args.deallocate() } + func {{ method_name }}(_ swiftCallbackInterface: {{ type_name }}, _ args: UnsafePointer) throws -> RustBuffer { {#- Unpacking args from the RustBuffer #} {%- if meth.arguments().len() != 0 -%} {#- Calling the concrete callback object #} - var reader = createReader(data: Data(rustBuffer: args)) + var reader = createReader(data: Data(rustBuffer: args.pointee)) {% if meth.return_type().is_some() %}let result = {% endif -%} {% if meth.throws() %}try {% endif -%} swiftCallbackInterface.{{ meth.name()|fn_name }}( diff --git a/uniffi_core/src/ffi/foreigncallbacks.rs b/uniffi_core/src/ffi/foreigncallbacks.rs index 54d2125c07..07320cd54f 100644 --- a/uniffi_core/src/ffi/foreigncallbacks.rs +++ b/uniffi_core/src/ffi/foreigncallbacks.rs @@ -142,7 +142,7 @@ use std::sync::atomic::{AtomicUsize, Ordering}; pub type ForeignCallback = unsafe extern "C" fn( handle: u64, method: u32, - args: RustBuffer, + args: &RustBuffer, buf_ptr: *mut RustBuffer, ) -> c_int; @@ -195,19 +195,26 @@ impl ForeignCallbackInternals { }; } - pub fn call_callback(&self, handle: u64, method: u32, args: RustBuffer, ret_rbuf: &mut RustBuffer) -> c_int { + fn call_callback( + &self, + handle: u64, + method: u32, + args: RustBuffer, + ret_rbuf: &mut RustBuffer, + ) -> c_int { let ptr_value = self.callback_ptr.load(Ordering::SeqCst); - unsafe { + unsafe { let callback = std::mem::transmute::>(ptr_value) .expect("Callback interface handler not set"); - callback(handle, method, args, ret_rbuf) + callback(handle, method, &args, ret_rbuf) } } /// 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 - where R: FfiConverter + where + R: FfiConverter, { let mut ret_rbuf = RustBuffer::new(); let callback_result = self.call_callback(handle, method, args, &mut ret_rbuf); @@ -250,10 +257,16 @@ impl ForeignCallbackInternals { /// Invoke a callback interface method that can throw on the foreign side / returnns a Result<> /// on the Rust side - pub fn invoke_callback_with_error(&self, handle: u64, method: u32, args: RustBuffer) -> Result - where R: FfiConverter, - E: FfiConverter, - E: From + pub fn invoke_callback_with_error( + &self, + handle: u64, + method: u32, + args: RustBuffer, + ) -> Result + where + R: FfiConverter, + E: FfiConverter, + E: From, { let mut ret_rbuf = RustBuffer::new(); let callback_result = self.call_callback(handle, method, args, &mut ret_rbuf); From 14e0bddcca89306b895f9d4294f449075335c40d Mon Sep 17 00:00:00 2001 From: Ben Dean-Kawamura Date: Fri, 3 Mar 2023 17:08:01 -0500 Subject: [PATCH 04/10] Optimize callback interface methods that return void MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If a callback interface method has a void return, there's no need to overwrite the value in the output buffer. Rust has already initialized an empty buffer. Also, there's no need to construct a RustBuffer in the first place. Refactored the callback interface code to make this possible without too much spaghettification. This gives us a decent speedup on Python/Kotlin: Benchmarking callbacks/python-callbacks-void-return Benchmarking callbacks/python-callbacks-void-return: Warming up for 3.0000 s Benchmarking callbacks/python-callbacks-void-return: Collecting 100 samples in estimated 10.050 s (813k iterations) Benchmarking callbacks/python-callbacks-void-return: Analyzing callbacks/python-callbacks-void-return time: [12.184 µs 12.219 µs 12.266 µs] change: [-26.774% -26.526% -26.286%] (p = 0.00 < 0.05) Performance has improved. Found 9 outliers among 100 measurements (9.00%) 2 (2.00%) high mild 7 (7.00%) high severe Benchmarking callbacks/kotlin-callbacks-void-return Benchmarking callbacks/kotlin-callbacks-void-return: Warming up for 3.0000 s Benchmarking callbacks/kotlin-callbacks-void-return: Collecting 100 samples in estimated 10.007 s (1.9M iterations) Benchmarking callbacks/kotlin-callbacks-void-return: Analyzing callbacks/kotlin-callbacks-void-return time: [5.1736 µs 5.2086 µs 5.2430 µs] change: [-32.627% -26.167% -20.263%] (p = 0.00 < 0.05) Performance has improved. Found 13 outliers among 100 measurements (13.00%) 3 (3.00%) low severe 6 (6.00%) low mild 1 (1.00%) high mild 3 (3.00%) high severe --- .../templates/CallbackInterfaceTemplate.kt | 84 +++++------ .../templates/CallbackInterfaceTemplate.py | 80 ++++++----- .../templates/CallbackInterfaceTemplate.swift | 134 +++++++++--------- 3 files changed, 150 insertions(+), 148 deletions(-) diff --git a/uniffi_bindgen/src/bindings/kotlin/templates/CallbackInterfaceTemplate.kt b/uniffi_bindgen/src/bindings/kotlin/templates/CallbackInterfaceTemplate.kt index 031b04a548..7b05192ecc 100644 --- a/uniffi_bindgen/src/bindings/kotlin/templates/CallbackInterfaceTemplate.kt +++ b/uniffi_bindgen/src/bindings/kotlin/templates/CallbackInterfaceTemplate.kt @@ -37,25 +37,7 @@ internal class {{ foreign_callback }} : ForeignCallback { // Call the method, write to outBuf and return a status code // See docs of ForeignCallback in `uniffi/src/ffi/foreigncallbacks.rs` for info try { - {%- match meth.throws_type() %} - {%- when Some(error_type) %} - try { - val buffer = this.{{ method_name }}(cb, args) - // Success - outBuf.setValue(buffer) - 1 - } catch (e: {{ error_type|type_name }}) { - // Expected error - val buffer = {{ error_type|ffi_converter_name }}.lowerIntoRustBuffer(e) - outBuf.setValue(buffer) - -2 - } - {%- else %} - val buffer = this.{{ method_name }}(cb, args) - // Success - outBuf.setValue(buffer) - 1 - {%- endmatch %} + this.{{ method_name }}(cb, args, outBuf) } catch (e: Throwable) { // Unexpected error try { @@ -84,32 +66,50 @@ internal class {{ foreign_callback }} : ForeignCallback { {% for meth in cbi.methods() -%} {% let method_name = format!("invoke_{}", meth.name())|fn_name %} - private fun {{ method_name }}(kotlinCallbackInterface: {{ type_name }}, @Suppress("UNUSED_PARAMETER")args: RustBuffer.ByReference): RustBuffer.ByValue { - {#- Unpacking args from the RustBuffer #} - {%- if meth.arguments().len() != 0 -%} - {#- Calling the concrete callback object #} - val buf = args.asByteBuffer() ?: throw InternalException("No ByteBuffer in RustBuffer; this is a Uniffi bug") - return kotlinCallbackInterface.{{ meth.name()|fn_name }}( - {% for arg in meth.arguments() -%} - {{ arg|read_fn }}(buf) + @Suppress("UNUSED_PARAMETER") + private fun {{ method_name }}(kotlinCallbackInterface: {{ type_name }}, args: RustBuffer.ByReference, outBuf: RustBufferByReference): Int { + {%- if meth.arguments().len() > 0 %} + val argsBuf = args.asByteBuffer() ?: throw InternalException("No ByteBuffer in RustBuffer; this is a Uniffi bug") + {%- endif %} + + {%- match meth.return_type() %} + {%- when Some with (return_type) %} + fun makeCall() : Int { + val returnValue = kotlinCallbackInterface.{{ meth.name()|fn_name }}( + {%- for arg in meth.arguments() %} + {{ arg|read_fn }}(argsBuf) + {% if !loop.last %}, {% endif %} + {%- endfor %} + ) + outBuf.setValue({{ return_type|ffi_converter_name }}.lowerIntoRustBuffer(returnValue)) + return 1 + } + {%- when None %} + fun makeCall() : Int { + kotlinCallbackInterface.{{ meth.name()|fn_name }}( + {%- for arg in meth.arguments() %} + {{ arg|read_fn }}(argsBuf) {%- if !loop.last %}, {% endif %} - {% endfor -%} - ) - {% else %} - return kotlinCallbackInterface.{{ meth.name()|fn_name }}() - {% endif -%} + {%- endfor %} + ) + return 1 + } + {%- endmatch %} - {#- Packing up the return value into a RustBuffer #} - {%- match meth.return_type() -%} - {%- when Some with (return_type) -%} - .let { - {{ return_type|ffi_converter_name }}.lowerIntoRustBuffer(it) - } - {%- else -%} - .let { RustBuffer.ByValue() } + {%- match meth.throws_type() %} + {%- when None %} + fun makeCallAndHandleError() : Int = makeCall() + {%- when Some(error_type) %} + fun makeCallAndHandleError() : Int = try { + makeCall() + } catch (e: {{ error_type|type_name }}) { + // Expected error, serialize it into outBuf + outBuf.setValue({{ error_type|ffi_converter_name }}.lowerIntoRustBuffer(e)) + -2 + } {%- endmatch %} - // TODO catch errors and report them back to Rust. - // https://github.com/mozilla/uniffi-rs/issues/351 + + return makeCallAndHandleError() } {% endfor %} } diff --git a/uniffi_bindgen/src/bindings/python/templates/CallbackInterfaceTemplate.py b/uniffi_bindgen/src/bindings/python/templates/CallbackInterfaceTemplate.py index c140481711..456a155672 100644 --- a/uniffi_bindgen/src/bindings/python/templates/CallbackInterfaceTemplate.py +++ b/uniffi_bindgen/src/bindings/python/templates/CallbackInterfaceTemplate.py @@ -15,30 +15,48 @@ def {{ meth.name()|fn_name }}({% call py::arg_list_decl(meth) %}): def py_{{ foreign_callback }}(handle, method, args, buf_ptr): {% for meth in cbi.methods() -%} {% let method_name = format!("invoke_{}", meth.name())|fn_name %} - def {{ method_name }}(python_callback, args): + def {{ method_name }}(python_callback, args, buf_ptr): {#- Unpacking args from the RustBuffer #} - {%- if meth.arguments().len() != 0 -%} - {#- Calling the concrete callback object #} - with args.contents.readWithStream() as buf: - rval = python_callback.{{ meth.name()|fn_name }}( - {% for arg in meth.arguments() -%} - {{ arg|read_fn }}(buf) - {%- if !loop.last %}, {% endif %} - {% endfor -%} - ) - {% else %} - rval = python_callback.{{ meth.name()|fn_name }}() - {% endif -%} + def makeCall(): + {%- if meth.arguments().len() != 0 -%} + {#- Calling the concrete callback object #} + with args.contents.readWithStream() as buf: + return python_callback.{{ meth.name()|fn_name }}( + {% for arg in meth.arguments() -%} + {{ arg|read_fn }}(buf) + {%- if !loop.last %}, {% endif %} + {% endfor -%} + ) + {%- else %} + return python_callback.{{ meth.name()|fn_name }}() + {%- endif %} + + def makeCallAndHandleReturn(): + {%- match meth.return_type() %} + {%- when Some(return_type) %} + rval = makeCall() + with RustBuffer.allocWithBuilder() as builder: + {{ return_type|write_fn }}(rval, builder) + buf_ptr[0] = builder.finalize() + {%- when None %} + makeCall() + {%- endmatch %} + return 1 + + {%- match meth.throws_type() %} + {%- when None %} + return makeCallAndHandleReturn() + {%- when Some(err) %} + try: + return makeCallAndHandleReturn() + except {{ err|type_name }} as e: + # Catch errors declared in the UDL file + with RustBuffer.allocWithBuilder() as builder: + {{ err|write_fn }}(e, builder) + buf_ptr[0] = builder.finalize() + return -2 + {%- endmatch %} - {#- Packing up the return value into a RustBuffer #} - {%- match meth.return_type() -%} - {%- when Some with (return_type) -%} - with RustBuffer.allocWithBuilder() as builder: - {{ return_type|write_fn }}(rval, builder) - return builder.finalize() - {%- else -%} - return RustBuffer.alloc(0) - {% endmatch -%} {% endfor %} cb = {{ ffi_converter_name }}.lift(handle) @@ -57,23 +75,7 @@ def {{ method_name }}(python_callback, args): # Call the method and handle any errors # See docs of ForeignCallback in `uniffi/src/ffi/foreigncallbacks.rs` for details try: - {%- match meth.throws_type() %} - {%- when Some(err) %} - try: - # Successful return - buf_ptr[0] = {{ method_name }}(cb, args) - return 1 - except {{ err|type_name }} as e: - # Catch errors declared in the UDL file - with RustBuffer.allocWithBuilder() as builder: - {{ err|write_fn }}(e, builder) - buf_ptr[0] = builder.finalize() - return -2 - {%- else %} - # Successful return - buf_ptr[0] = {{ method_name }}(cb, args) - return 1 - {%- endmatch %} + return {{ method_name }}(cb, args, buf_ptr) except BaseException as e: # Catch unexpected errors try: diff --git a/uniffi_bindgen/src/bindings/swift/templates/CallbackInterfaceTemplate.swift b/uniffi_bindgen/src/bindings/swift/templates/CallbackInterfaceTemplate.swift index 440c3008a9..7cb40a3900 100644 --- a/uniffi_bindgen/src/bindings/swift/templates/CallbackInterfaceTemplate.swift +++ b/uniffi_bindgen/src/bindings/swift/templates/CallbackInterfaceTemplate.swift @@ -17,87 +17,87 @@ public protocol {{ type_name }} : AnyObject { // The ForeignCallback that is passed to Rust. fileprivate let {{ foreign_callback }} : ForeignCallback = { (handle: UniFFICallbackHandle, method: Int32, args: UnsafePointer, out_buf: UnsafeMutablePointer) -> Int32 in - {% for meth in cbi.methods() -%} - {%- let method_name = format!("invoke_{}", meth.name())|fn_name -%} + {% for meth in cbi.methods() -%} + {%- let method_name = format!("invoke_{}", meth.name())|fn_name %} - func {{ method_name }}(_ swiftCallbackInterface: {{ type_name }}, _ args: UnsafePointer) throws -> RustBuffer { - {#- Unpacking args from the RustBuffer #} - {%- if meth.arguments().len() != 0 -%} - {#- Calling the concrete callback object #} + func {{ method_name }}(_ swiftCallbackInterface: {{ type_name }}, _ args: UnsafePointer, _ out_buf: UnsafeMutablePointer) throws -> Int32 { + {%- if meth.arguments().len() > 0 %} + var reader = createReader(data: Data(rustBuffer: args.pointee)) + {%- endif %} - var reader = createReader(data: Data(rustBuffer: args.pointee)) - {% if meth.return_type().is_some() %}let result = {% endif -%} - {% if meth.throws() %}try {% endif -%} - swiftCallbackInterface.{{ meth.name()|fn_name }}( + {%- match meth.return_type() %} + {%- when Some(return_type) %} + func makeCall() throws -> Int32 { + let result = try swiftCallbackInterface.{{ meth.name()|fn_name }}( {% for arg in meth.arguments() -%} {% if !config.omit_argument_labels() %}{{ arg.name()|var_name }}: {% endif %} try {{ arg|read_fn }}(from: &reader) {%- if !loop.last %}, {% endif %} {% endfor -%} ) - {% else %} - {% if meth.return_type().is_some() %}let result = {% endif -%} - {% if meth.throws() %}try {% endif -%} - swiftCallbackInterface.{{ meth.name()|fn_name }}() - {% endif -%} - - {#- Packing up the return value into a RustBuffer #} - {%- match meth.return_type() -%} - {%- when Some with (return_type) -%} - var writer = [UInt8]() - {{ return_type|write_fn }}(result, into: &writer) - return RustBuffer(bytes: writer) - {%- else -%} - return RustBuffer() - {% endmatch -%} - // TODO catch errors and report them back to Rust. - // https://github.com/mozilla/uniffi-rs/issues/351 - + var writer = [UInt8]() + {{ return_type|write_fn }}(result, into: &writer) + out_buf.pointee = RustBuffer(bytes: writer) + return 1 } - {% endfor %} + {%- when None %} + func makeCall() throws -> Int32 { + try swiftCallbackInterface.{{ meth.name()|fn_name }}( + {% for arg in meth.arguments() -%} + {% if !config.omit_argument_labels() %}{{ arg.name()|var_name }}: {% endif %} try {{ arg|read_fn }}(from: &reader) + {%- if !loop.last %}, {% endif %} + {% endfor -%} + ) + return 1 + } + {%- endmatch %} - let cb: {{ cbi|type_name }} + {%- match meth.throws_type() %} + {%- when None %} + return try makeCall() + {%- when Some(error_type) %} do { - cb = try {{ ffi_converter_name }}.lift(handle) - } catch { - out_buf.pointee = {{ Type::String.borrow()|lower_fn }}("{{ cbi.name() }}: Invalid handle") - return -1 + return try makeCall() + } catch let error as {{ error_type|type_name }} { + out_buf.pointee = {{ error_type|lower_fn }}(error) + return -2 } + {%- endmatch %} + } + {%- endfor %} + - 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 - {% for meth in cbi.methods() -%} - {% let method_name = format!("invoke_{}", meth.name())|fn_name -%} - case {{ loop.index }}: - do { - out_buf.pointee = try {{ method_name }}(cb, args) - // Value written to out buffer. - // See docs of ForeignCallback in `uniffi/src/ffi/foreigncallbacks.rs` - return 1 - {%- match meth.throws_type() %} - {%- when Some(error_type) %} - } catch let error as {{ error_type|type_name }} { - out_buf.pointee = {{ error_type|lower_fn }}(error) - return -2 - {%- else %} - {%- endmatch %} - } catch let error { - out_buf.pointee = {{ Type::String.borrow()|lower_fn }}(String(describing: error)) - return -1 - } - {% endfor %} - // This should never happen, because an out of bounds method index won't - // ever be used. Once we can catch errors, we should return an InternalError. - // https://github.com/mozilla/uniffi-rs/issues/351 - default: - // An unexpected error happened. - // See docs of ForeignCallback in `uniffi/src/ffi/foreigncallbacks.rs` + 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 + {% for meth in cbi.methods() -%} + {% let method_name = format!("invoke_{}", meth.name())|fn_name -%} + case {{ loop.index }}: + let cb: {{ cbi|type_name }} + do { + cb = try {{ ffi_converter_name }}.lift(handle) + } catch { + out_buf.pointee = {{ Type::String.borrow()|lower_fn }}("{{ cbi.name() }}: Invalid handle") return -1 - } + } + do { + return try {{ method_name }}(cb, args, out_buf) + } catch let error { + out_buf.pointee = {{ Type::String.borrow()|lower_fn }}(String(describing: error)) + return -1 + } + {% endfor %} + // This should never happen, because an out of bounds method index won't + // ever be used. Once we can catch errors, we should return an InternalError. + // 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 } +} // FfiConverter protocol for callback interfaces fileprivate struct {{ ffi_converter_name }} { From e87748c57d28ff27612a9df993060d31b86a4979 Mon Sep 17 00:00:00 2001 From: Ben Dean-Kawamura Date: Fri, 3 Mar 2023 17:43:37 -0500 Subject: [PATCH 05/10] Optimize passing arguments to callback interface methods MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This changes things so we send the arguments as a data/len component rather than a RustBuffer struct directly. For some reason this is faster on Python/Kotlin, although it's not clear why. Here's some possibilities: - cytpes and JNA take a performance hit when dealing with structs vs primitive values. - RustBuffer has a third component: capacity, that we don't need. By not sending this component, it reduces the stack size. In any case, this really does speed up Python/Kotlin. Swift doesn't seem to be affected. Benchmarking callbacks/python-callbacks-void-return Benchmarking callbacks/python-callbacks-void-return: Warming up for 3.0000 s Benchmarking callbacks/python-callbacks-void-return: Collecting 100 samples in estimated 10.050 s (813k iterations) Benchmarking callbacks/python-callbacks-void-return: Analyzing callbacks/python-callbacks-void-return time: [12.184 µs 12.219 µs 12.266 µs] change: [-26.774% -26.526% -26.286%] (p = 0.00 < 0.05) Performance has improved. Found 9 outliers among 100 measurements (9.00%) 2 (2.00%) high mild 7 (7.00%) high severe Benchmarking callbacks/kotlin-callbacks-void-return Benchmarking callbacks/kotlin-callbacks-void-return: Warming up for 3.0000 s Benchmarking callbacks/kotlin-callbacks-void-return: Collecting 100 samples in estimated 10.007 s (1.9M iterations) Benchmarking callbacks/kotlin-callbacks-void-return: Analyzing callbacks/kotlin-callbacks-void-return time: [5.1736 µs 5.2086 µs 5.2430 µs] change: [-32.627% -26.167% -20.263%] (p = 0.00 < 0.05) Performance has improved. Found 13 outliers among 100 measurements (13.00%) 3 (3.00%) low severe 6 (6.00%) low mild 1 (1.00%) high mild 3 (3.00%) high severe --- .../templates/CallbackInterfaceRuntime.kt | 2 +- .../templates/CallbackInterfaceTemplate.kt | 10 ++++---- .../templates/CallbackInterfaceTemplate.py | 21 ++++++++--------- .../src/bindings/python/templates/Helpers.py | 2 +- .../python/templates/RustBufferTemplate.py | 23 +++++++++++-------- .../swift/templates/BridgingHeaderTemplate.h | 2 +- .../templates/CallbackInterfaceTemplate.swift | 8 +++---- uniffi_core/src/ffi/foreigncallbacks.rs | 15 ++++++++---- uniffi_core/src/ffi/rustbuffer.rs | 5 ++++ 9 files changed, 53 insertions(+), 35 deletions(-) diff --git a/uniffi_bindgen/src/bindings/kotlin/templates/CallbackInterfaceRuntime.kt b/uniffi_bindgen/src/bindings/kotlin/templates/CallbackInterfaceRuntime.kt index e7a7b4a361..302702b8bd 100644 --- a/uniffi_bindgen/src/bindings/kotlin/templates/CallbackInterfaceRuntime.kt +++ b/uniffi_bindgen/src/bindings/kotlin/templates/CallbackInterfaceRuntime.kt @@ -35,7 +35,7 @@ internal class ConcurrentHandleMap( } interface ForeignCallback : com.sun.jna.Callback { - public fun invoke(handle: Handle, method: Int, args: RustBuffer.ByReference, outBuf: RustBufferByReference): Int + public fun invoke(handle: Handle, method: Int, argsData: Pointer, argsLen: Int, outBuf: RustBufferByReference): Int } // Magic number for the Rust proxy to call using the same mechanism as every other method, diff --git a/uniffi_bindgen/src/bindings/kotlin/templates/CallbackInterfaceTemplate.kt b/uniffi_bindgen/src/bindings/kotlin/templates/CallbackInterfaceTemplate.kt index 7b05192ecc..d1fb106c12 100644 --- a/uniffi_bindgen/src/bindings/kotlin/templates/CallbackInterfaceTemplate.kt +++ b/uniffi_bindgen/src/bindings/kotlin/templates/CallbackInterfaceTemplate.kt @@ -22,7 +22,7 @@ public interface {{ type_name }} { // The ForeignCallback that is passed to Rust. internal class {{ foreign_callback }} : ForeignCallback { @Suppress("TooGenericExceptionCaught") - override fun invoke(handle: Handle, method: Int, args: RustBuffer.ByReference, outBuf: RustBufferByReference): Int { + override fun invoke(handle: Handle, method: Int, argsData: Pointer, argsLen: Int, outBuf: RustBufferByReference): Int { val cb = {{ ffi_converter_name }}.lift(handle) return when (method) { IDX_CALLBACK_FREE -> { @@ -37,7 +37,7 @@ internal class {{ foreign_callback }} : ForeignCallback { // Call the method, write to outBuf and return a status code // See docs of ForeignCallback in `uniffi/src/ffi/foreigncallbacks.rs` for info try { - this.{{ method_name }}(cb, args, outBuf) + this.{{ method_name }}(cb, argsData, argsLen, outBuf) } catch (e: Throwable) { // Unexpected error try { @@ -67,9 +67,11 @@ internal class {{ foreign_callback }} : ForeignCallback { {% for meth in cbi.methods() -%} {% let method_name = format!("invoke_{}", meth.name())|fn_name %} @Suppress("UNUSED_PARAMETER") - private fun {{ method_name }}(kotlinCallbackInterface: {{ type_name }}, args: RustBuffer.ByReference, outBuf: RustBufferByReference): Int { + private fun {{ method_name }}(kotlinCallbackInterface: {{ type_name }}, argsData: Pointer, argsLen: Int, outBuf: RustBufferByReference): Int { {%- if meth.arguments().len() > 0 %} - val argsBuf = args.asByteBuffer() ?: throw InternalException("No ByteBuffer in RustBuffer; this is a Uniffi bug") + val argsBuf = argsData.getByteBuffer(0, argsLen.toLong()).also { + it.order(ByteOrder.BIG_ENDIAN) + } {%- endif %} {%- match meth.return_type() %} diff --git a/uniffi_bindgen/src/bindings/python/templates/CallbackInterfaceTemplate.py b/uniffi_bindgen/src/bindings/python/templates/CallbackInterfaceTemplate.py index 456a155672..5702736de4 100644 --- a/uniffi_bindgen/src/bindings/python/templates/CallbackInterfaceTemplate.py +++ b/uniffi_bindgen/src/bindings/python/templates/CallbackInterfaceTemplate.py @@ -12,21 +12,20 @@ def {{ meth.name()|fn_name }}({% call py::arg_list_decl(meth) %}): {% endfor %} -def py_{{ foreign_callback }}(handle, method, args, buf_ptr): +def py_{{ foreign_callback }}(handle, method, args_data, args_len, buf_ptr): {% for meth in cbi.methods() -%} {% let method_name = format!("invoke_{}", meth.name())|fn_name %} - def {{ method_name }}(python_callback, args, buf_ptr): + def {{ method_name }}(python_callback, args_stream, buf_ptr): {#- Unpacking args from the RustBuffer #} def makeCall(): - {%- if meth.arguments().len() != 0 -%} {#- Calling the concrete callback object #} - with args.contents.readWithStream() as buf: - return python_callback.{{ meth.name()|fn_name }}( - {% for arg in meth.arguments() -%} - {{ arg|read_fn }}(buf) - {%- if !loop.last %}, {% endif %} - {% endfor -%} - ) + {%- if meth.arguments().len() != 0 -%} + return python_callback.{{ meth.name()|fn_name }}( + {% for arg in meth.arguments() -%} + {{ arg|read_fn }}(args_stream) + {%- if !loop.last %}, {% endif %} + {% endfor -%} + ) {%- else %} return python_callback.{{ meth.name()|fn_name }}() {%- endif %} @@ -75,7 +74,7 @@ def makeCallAndHandleReturn(): # Call the method and handle any errors # See docs of ForeignCallback in `uniffi/src/ffi/foreigncallbacks.rs` for details try: - return {{ method_name }}(cb, args, buf_ptr) + return {{ method_name }}(cb, RustBufferStream(args_data, args_len), buf_ptr) except BaseException as e: # Catch unexpected errors try: diff --git a/uniffi_bindgen/src/bindings/python/templates/Helpers.py b/uniffi_bindgen/src/bindings/python/templates/Helpers.py index 8afd370e63..6525bc3466 100644 --- a/uniffi_bindgen/src/bindings/python/templates/Helpers.py +++ b/uniffi_bindgen/src/bindings/python/templates/Helpers.py @@ -64,4 +64,4 @@ def rust_call_with_error(error_ffi_converter, fn, *args): # A function pointer for a callback as defined by UniFFI. # Rust definition `fn(handle: u64, method: u32, args: RustBuffer, buf_ptr: *mut RustBuffer) -> int` -FOREIGN_CALLBACK_T = ctypes.CFUNCTYPE(ctypes.c_int, ctypes.c_ulonglong, ctypes.c_ulong, ctypes.POINTER(RustBuffer), ctypes.POINTER(RustBuffer)) +FOREIGN_CALLBACK_T = ctypes.CFUNCTYPE(ctypes.c_int, ctypes.c_ulonglong, ctypes.c_ulong, ctypes.POINTER(ctypes.c_char), ctypes.c_int, ctypes.POINTER(RustBuffer)) diff --git a/uniffi_bindgen/src/bindings/python/templates/RustBufferTemplate.py b/uniffi_bindgen/src/bindings/python/templates/RustBufferTemplate.py index b55eaf5df5..ba050b9d8b 100644 --- a/uniffi_bindgen/src/bindings/python/templates/RustBufferTemplate.py +++ b/uniffi_bindgen/src/bindings/python/templates/RustBufferTemplate.py @@ -46,7 +46,7 @@ def consumeWithStream(self): leak it even if an error occurs. """ try: - s = RustBufferStream(self) + s = RustBufferStream.from_rust_buffer(self) yield s if s.remaining() != 0: raise RuntimeError("junk data left in buffer at end of consumeWithStream") @@ -60,7 +60,7 @@ def readWithStream(self): This is like consumeWithStream, but doesn't free the buffer afterwards. It should only be used with borrowed `RustBuffer` data. """ - s = RustBufferStream(self) + s = RustBufferStream.from_rust_buffer(self) yield s if s.remaining() != 0: raise RuntimeError("junk data left in buffer at end of readWithStream") @@ -80,24 +80,29 @@ class RustBufferStream(object): Helper for structured reading of bytes from a RustBuffer """ - def __init__(self, rbuf): - self.rbuf = rbuf + def __init__(self, data, len): + self.data = data + self.len = len self.offset = 0 + @classmethod + def from_rust_buffer(cls, buf): + return cls(buf.data, buf.len) + def remaining(self): - return self.rbuf.len - self.offset + return self.len - self.offset def _unpack_from(self, size, format): - if self.offset + size > self.rbuf.len: + if self.offset + size > self.len: raise InternalError("read past end of rust buffer") - value = struct.unpack(format, self.rbuf.data[self.offset:self.offset+size])[0] + value = struct.unpack(format, self.data[self.offset:self.offset+size])[0] self.offset += size return value def read(self, size): - if self.offset + size > self.rbuf.len: + if self.offset + size > self.len: raise InternalError("read past end of rust buffer") - data = self.rbuf.data[self.offset:self.offset+size] + data = self.data[self.offset:self.offset+size] self.offset += size return data diff --git a/uniffi_bindgen/src/bindings/swift/templates/BridgingHeaderTemplate.h b/uniffi_bindgen/src/bindings/swift/templates/BridgingHeaderTemplate.h index 01bd28635c..2fc759d343 100644 --- a/uniffi_bindgen/src/bindings/swift/templates/BridgingHeaderTemplate.h +++ b/uniffi_bindgen/src/bindings/swift/templates/BridgingHeaderTemplate.h @@ -28,7 +28,7 @@ typedef struct RustBuffer uint8_t *_Nullable data; } RustBuffer; -typedef int32_t (*ForeignCallback)(uint64_t, int32_t, const RustBuffer *_Nonnull, RustBuffer *_Nonnull); +typedef int32_t (*ForeignCallback)(uint64_t, int32_t, const uint8_t *_Nonnull, int32_t, RustBuffer *_Nonnull); typedef struct ForeignBytes { diff --git a/uniffi_bindgen/src/bindings/swift/templates/CallbackInterfaceTemplate.swift b/uniffi_bindgen/src/bindings/swift/templates/CallbackInterfaceTemplate.swift index 7cb40a3900..4e4267b7f4 100644 --- a/uniffi_bindgen/src/bindings/swift/templates/CallbackInterfaceTemplate.swift +++ b/uniffi_bindgen/src/bindings/swift/templates/CallbackInterfaceTemplate.swift @@ -16,13 +16,13 @@ public protocol {{ type_name }} : AnyObject { // The ForeignCallback that is passed to Rust. fileprivate let {{ foreign_callback }} : ForeignCallback = - { (handle: UniFFICallbackHandle, method: Int32, args: UnsafePointer, out_buf: UnsafeMutablePointer) -> Int32 in + { (handle: UniFFICallbackHandle, method: Int32, argsData: UnsafePointer, argsLen: Int32, out_buf: UnsafeMutablePointer) -> Int32 in {% for meth in cbi.methods() -%} {%- let method_name = format!("invoke_{}", meth.name())|fn_name %} - func {{ method_name }}(_ swiftCallbackInterface: {{ type_name }}, _ args: UnsafePointer, _ out_buf: UnsafeMutablePointer) throws -> Int32 { + func {{ method_name }}(_ swiftCallbackInterface: {{ type_name }}, _ argsData: UnsafePointer, _ argsLen: Int32, _ out_buf: UnsafeMutablePointer) throws -> Int32 { {%- if meth.arguments().len() > 0 %} - var reader = createReader(data: Data(rustBuffer: args.pointee)) + var reader = createReader(data: Data(bytes: argsData, count: Int(argsLen))) {%- endif %} {%- match meth.return_type() %} @@ -83,7 +83,7 @@ fileprivate let {{ foreign_callback }} : ForeignCallback = return -1 } do { - return try {{ method_name }}(cb, args, out_buf) + 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 diff --git a/uniffi_core/src/ffi/foreigncallbacks.rs b/uniffi_core/src/ffi/foreigncallbacks.rs index 07320cd54f..29f1d0cfe0 100644 --- a/uniffi_core/src/ffi/foreigncallbacks.rs +++ b/uniffi_core/src/ffi/foreigncallbacks.rs @@ -128,8 +128,8 @@ use std::sync::atomic::{AtomicUsize, Ordering}; /// * 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. -/// * `args` is a serialized buffer of arguments to the function. UniFFI will deserialize it before -/// passing individual arguments to the user's callback. +/// * `args_data` and `args_len` represents a serialized buffer of arguments to the function. +/// UniFFI will deserialize it before passing individual arguments to the user's callback. /// * `buf_ptr` is a pointer to where the resulting buffer will be written. UniFFI will allocate a /// buffer to write the result into. /// * A callback returns: @@ -142,7 +142,8 @@ use std::sync::atomic::{AtomicUsize, Ordering}; pub type ForeignCallback = unsafe extern "C" fn( handle: u64, method: u32, - args: &RustBuffer, + args_data: *const u8, + args_len: i32, buf_ptr: *mut RustBuffer, ) -> c_int; @@ -206,7 +207,13 @@ impl ForeignCallbackInternals { unsafe { let callback = std::mem::transmute::>(ptr_value) .expect("Callback interface handler not set"); - callback(handle, method, &args, ret_rbuf) + callback( + handle, + method, + args.data_pointer(), + args.len() as i32, + ret_rbuf, + ) } } diff --git a/uniffi_core/src/ffi/rustbuffer.rs b/uniffi_core/src/ffi/rustbuffer.rs index 63af586fb6..e00c67ab69 100644 --- a/uniffi_core/src/ffi/rustbuffer.rs +++ b/uniffi_core/src/ffi/rustbuffer.rs @@ -101,6 +101,11 @@ impl RustBuffer { .expect("buffer length negative or overflowed") } + /// Get a pointer to the data + pub fn data_pointer(&self) -> *const u8 { + self.data + } + /// Returns true if the length of the buffer is 0. pub fn is_empty(&self) -> bool { self.len == 0 From c402ae7ef569108cba4e6ab7ac3e9904266e40a5 Mon Sep 17 00:00:00 2001 From: Ben Dean-Kawamura Date: Fri, 3 Mar 2023 20:09:48 -0500 Subject: [PATCH 06/10] Clippy fixes --- fixtures/benchmarks/benches/benchmarks.rs | 2 +- uniffi_core/src/ffi/foreigncallbacks.rs | 7 ++----- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/fixtures/benchmarks/benches/benchmarks.rs b/fixtures/benchmarks/benches/benchmarks.rs index eba996291c..ac452116f5 100644 --- a/fixtures/benchmarks/benches/benchmarks.rs +++ b/fixtures/benchmarks/benches/benchmarks.rs @@ -40,7 +40,7 @@ fn main() { std::env!("CARGO_TARGET_TMPDIR"), "uniffi-fixture-benchmarks", "benches/bindings/run_benchmarks.swift", - script_args.clone(), + script_args, RunScriptMode::PerformanceTest, ) .unwrap() diff --git a/uniffi_core/src/ffi/foreigncallbacks.rs b/uniffi_core/src/ffi/foreigncallbacks.rs index 29f1d0cfe0..33232aed27 100644 --- a/uniffi_core/src/ffi/foreigncallbacks.rs +++ b/uniffi_core/src/ffi/foreigncallbacks.rs @@ -322,17 +322,14 @@ 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 - () - } + // 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 => { From caca27f7de1cbbb476412a4ef7916865c388038a Mon Sep 17 00:00:00 2001 From: Ben Dean-Kawamura Date: Fri, 3 Mar 2023 20:20:00 -0500 Subject: [PATCH 07/10] Adding changelog entry --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index b1934c0d26..c02aec5f0f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,9 @@ [All changes in [[UnreleasedVersion]]](https://github.com/mozilla/uniffi-rs/compare/v0.23.0...HEAD). +### ⚠️ Breaking Changes ⚠️ +- Implemented a new callback-interface ABI that signifacantly improves performance on Python and Kotlin. + ### What's changed - The `include_scaffolding!()` macro must now either be called from your crate root or you must have `use the_mod_that_calls_include_scaffolding::*` in your crate root. This was always the expectation, but wasn't required before. This will now start failing with errors that say `crate::UniFfiTag` does not exist. From e19e265a22845c2948b6f1b09186edb60199e5fe Mon Sep 17 00:00:00 2001 From: Ben Dean-Kawamura Date: Thu, 23 Mar 2023 08:43:30 -0400 Subject: [PATCH 08/10] Doc fixes from review --- CHANGELOG.md | 2 +- fixtures/benchmarks/src/benchmarks.udl | 12 ++++++------ uniffi_core/src/ffi/foreigncallbacks.rs | 5 +++-- 3 files changed, 10 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c02aec5f0f..3635e9a50d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,7 +15,7 @@ [All changes in [[UnreleasedVersion]]](https://github.com/mozilla/uniffi-rs/compare/v0.23.0...HEAD). ### ⚠️ Breaking Changes ⚠️ -- Implemented a new callback-interface ABI that signifacantly improves performance on Python and Kotlin. +- 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. ### What's changed diff --git a/fixtures/benchmarks/src/benchmarks.udl b/fixtures/benchmarks/src/benchmarks.udl index 9c48b8192a..53ea45cc81 100644 --- a/fixtures/benchmarks/src/benchmarks.udl +++ b/fixtures/benchmarks/src/benchmarks.udl @@ -1,16 +1,16 @@ namespace benchmarks { - // Run all benchmarks and print the results to stdout - void run_benchmarks(string language_name, TestCallbackInterface cb); + // Run all benchmarks and print the results to stdout + void run_benchmarks(string language_name, TestCallbackInterface cb); - // Test functions + // Test functions // // These are intented to test the overhead of Rust function calls including: // popping arguments from the stack, unpacking RustBuffers, pushing return // values back to the stack, etc. - string test_function(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 test_function(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(); }; dictionary TestData { diff --git a/uniffi_core/src/ffi/foreigncallbacks.rs b/uniffi_core/src/ffi/foreigncallbacks.rs index 33232aed27..40a51ecafc 100644 --- a/uniffi_core/src/ffi/foreigncallbacks.rs +++ b/uniffi_core/src/ffi/foreigncallbacks.rs @@ -128,8 +128,9 @@ use std::sync::atomic::{AtomicUsize, Ordering}; /// * 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. -/// * `args_data` and `args_len` represents a serialized buffer of arguments to the function. -/// UniFFI will deserialize it before passing individual arguments to the user's callback. +/// * `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. /// * `buf_ptr` is a pointer to where the resulting buffer will be written. UniFFI will allocate a /// buffer to write the result into. /// * A callback returns: From 76331d761bf970ab4a20b3951d16529269f29503 Mon Sep 17 00:00:00 2001 From: Ben Dean-Kawamura Date: Thu, 23 Mar 2023 13:41:02 -0400 Subject: [PATCH 09/10] More review fixes --- fixtures/benchmarks/README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/fixtures/benchmarks/README.md b/fixtures/benchmarks/README.md index 482d650318..46d234436c 100644 --- a/fixtures/benchmarks/README.md +++ b/fixtures/benchmarks/README.md @@ -8,10 +8,10 @@ the foreign language: the benchmarks for. - `benchmarks.rs` executes a script for each foreign language that we want to benchmark. - Those scripts call the `run_benchmarks()` function from `lib.rs` - - `run_benchmarks()` parses the CLI arguments again, this time to how to setup + - `run_benchmarks()` parses the CLI arguments again, this time to determine how to setup the `Criterion` object. - Testing callback interfaces is relatively straightforward, we simply invoke the callback method. - Testing regular functions requires some extra care, since these are called by the foreign bindings. To test those, `benchmarks.rs` invokes a callback - interface method that call the rust function and reports the time taken. + interface method that calls the Rust function and reports the time taken. From 47b494a9a59f079717507cb9f596ab2e4949bc0c Mon Sep 17 00:00:00 2001 From: Ben Dean-Kawamura Date: Thu, 23 Mar 2023 15:45:30 -0400 Subject: [PATCH 10/10] More fixes from review. Most of these are doc fixes. The one behavior change is that the callback return values were updated. If we're breaking the ABI we might as well make these be a bit more logical. --- CHANGELOG.md | 4 +- fixtures/benchmarks/README.md | 5 ++ .../benches/bindings/run_benchmarks.kts | 6 +- .../benches/bindings/run_benchmarks.py | 6 +- .../benches/bindings/run_benchmarks.swift | 6 +- fixtures/benchmarks/src/benchmarks.udl | 6 +- fixtures/benchmarks/src/lib.rs | 12 ++-- .../templates/CallbackInterfaceRuntime.kt | 4 ++ .../templates/CallbackInterfaceTemplate.kt | 20 +++---- .../templates/CallbackInterfaceRuntime.py | 4 ++ .../templates/CallbackInterfaceTemplate.py | 18 +++--- .../templates/CallbackInterfaceRuntime.swift | 4 ++ .../templates/CallbackInterfaceTemplate.swift | 20 +++---- uniffi_core/src/ffi/foreigncallbacks.rs | 60 +++++++------------ 14 files changed, 88 insertions(+), 87 deletions(-) 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,