From 6307aa0561312472557c8770da1042a7d74a24ad Mon Sep 17 00:00:00 2001 From: Ben Dean-Kawamura Date: Tue, 21 Nov 2023 15:41:51 -0500 Subject: [PATCH] Adding ADR-0009 This revisits the decision in `ADR-0005` and explores using going back to handle maps to pass objects across the FFI. --- docs/adr/0005-arc-pointers.md | 2 +- docs/adr/0009-handles.md | 124 ++++++++++++++++++++++++ docs/handles.md | 174 ++++++++++++++++++++++++++++++++++ 3 files changed, 299 insertions(+), 1 deletion(-) create mode 100644 docs/adr/0009-handles.md create mode 100644 docs/handles.md diff --git a/docs/adr/0005-arc-pointers.md b/docs/adr/0005-arc-pointers.md index 2bb8ef8d4b..d4affa47f8 100644 --- a/docs/adr/0005-arc-pointers.md +++ b/docs/adr/0005-arc-pointers.md @@ -1,6 +1,6 @@ # Use raw `Arc` pointers to pass objects across the FFI -* Status: proposed +* Status: accepted * Deciders: mhammond, rfkelly * Consulted: travis, jhugman, dmose * Date: 2021-04-19 diff --git a/docs/adr/0009-handles.md b/docs/adr/0009-handles.md new file mode 100644 index 0000000000..e55c40b813 --- /dev/null +++ b/docs/adr/0009-handles.md @@ -0,0 +1,124 @@ +# Use handle map handles to pass objects across the FFI + +* Status: proposed +* Deciders: +* Consulted: + +Discussion and approval: + +ADR-0005 discussion: [PR 430](https://github.com/mozilla/uniffi-rs/pull/430). + +## Context and Problem Statement + +UniFFI currently passes objects from Rust to the foreign side by leaking an Arc reference into a word-sized opaque pointer and passing it across the FFI. +The basic approach uses `Arc::into_raw` / `Arc::from_raw` and was chosen in [ADR-0005](./0005-arc-pointers.md) for several reasons: + + 1. Clearer generated code. + 2. Ability to pass objects as arguments (https://github.com/mozilla/uniffi-rs/issues/40). + This was deemed difficult to do with the existing codegen + HandleMap. + 3. Ability to track object identity (https://github.com/mozilla/uniffi-rs/issues/197). If two function calls return the same object, then this should result in an identical object on the foreign side. + 4. Increased performance. + +Recently, this approach was extended to work with unsized types (`Arc`), which are normally wide pointers (i.e double-word sized). +For these types, we box the Arc to create `Box>`, then leak the box pointer. +This results in a regular, word-sized, pointer since `Arc` is sized (2 words) even when `dyn Trait` is not. + +Now that we have several years of experience, it's a good time to revisit some of the reasoning in ADR-0005 because it seems like we're not getting the benefits we wanted: + +* The code that deals with these isn't so clear, especially when we have to deal with unsized types (for example + the RustFuture + [allocation](https://github.com/mozilla/uniffi-rs/blob/fbc6631953a889c7af6e5f1af94de9242589b75b/uniffi_core/src/ffi/rustfuture/mod.rs#L56-L63) / [dellocation](https://github.com/mozilla/uniffi-rs/blob/fbc6631953a889c7af6e5f1af94de9242589b75b/uniffi_core/src/ffi/rustfuture/mod.rs#L124-L125) or the similar code for trait interfaces). +* The codegen has progressed and it would be easy to support `[2]`. + We could simply `clone` the handle as part of the `lower()` call. +* We've never implemented the reverse identity map needed for `[3]`. + The `NimbusClient` example given in https://github.com/mozilla/uniffi-rs/issues/419 would still fail today. + Given that there has been little to no demand for this feature, this should be changed to a non-goal. +* The performance benefit decreases when discussing unsized types which require an additional layer of boxing. + In that case, instead of a strict decrease in work, we are trading a `HandleMap` insertion for a Box allocation. + This is a complex tradeoff, with the box allocation likely being faster, but not by much. + +Furthermore, practice has shown that dealing with raw pointers makes debugging difficult, with errors often resulting in segfaults or UB. +Dealing with any sort of FFI handle is going to be error prone, but at least with a handle map we can generate better error messages and correct stack traces. +There are also more error modes with this code. + +### Safety + +ADR-0005 says "We believe the additional safety offered by `HandleMap`s is far less important for this use-case, because the code using these pointers is generated instead of hand-written." + +While it's certainly true safety benefits matter less for generated code, it's also true that UniFFI is much more complex now then when ADR-0005 was decided. +We have introduced callback interfaces, trait interfaces, Future handles for async functions, etc. +All of these introduce additional failure cases, for example #1797, which means that relatively small safety benefits are more valuable. + +### Foreign handles + +A related question is how to handle handles to foreign objects that are passed into Rust. +However, that question is orthogonal to this one and is out-of-scope for this ADR. + +## Considered Options + +### [Option 1] Continue using raw Arc pointers to pass Rust objects across the FFI + +Stay with the current status quo. + +### [Option 2] Use the old `HandleMap` to pass Rust objects across the FFI + +We could switch back to the old handle map code, which is still around in the [ffi-support crate](https://github.com/mozilla/ffi-support/blob/main/src/handle_map.rs). +This implements a relatively simple handle-map that uses a `RWLock` to manage concurrency. + +See [../handles.md] for details on how this would work. + +Handles are passed as a `u64` values, but they only actually use 48 bits. +This works better with JS, where the `Value` type only supports integers up to 53-bits wide. + +### [Option 3] Use a `HandleMap` with more performant/complex concurrency strategy + +We could switch to something like the [handle map implementation from #1808](https://github.com/bendk/uniffi-rs/blob/d305f7e47203b260e2e44009e37e7435fd554eaa/uniffi_core/src/ffi/slab.rs). +The struct in that code was named `Slab` because it was inspired by the `tokio` `slab` crate. +However, it's very similar to the original UniFFI `HandleMap` and this PR will call it a `HandleMap` to follow in that tradition. + +See [../handles.md] for details on how this would work. + +### [Option 4] Use a 3rd-party crate to pass Rust objects across the FFI + +We could also use a 3rd-party crate to handle this. +The `sharded-slab` crate promises lock-free concurrency and supports generation counters. + +## Decision Drivers + +## Decision Outcome + +??? + +## Pros and Cons of the Options + +### [Option 1] Continue using raw Arc pointers to pass Rust objects across the FFI + +* Good, because it has the fastest performance, especially for sized types. +* Good, because it doesn't require code changes. +* Bad, because it's hard to debug errors. + +### [Option 2] Use the original handle map to pass Rust objects across the FFI + +* Good, because it's easier to debug errors. +* Bad, because it requires a read-write lock. + In particular, it seems bad that `insert`/`remove` can block `get`. +* Good, because it works better with Javascript +* Good, because it works with any type, not just `Arc`. + For example, we might want to pass a handle to a [oneshot::Sender](https://docs.rs/oneshot/latest/oneshot/) across the FFI to implement async callback interface methods. + +### [Option 3] Use a handle map with a simpler concurrency strategy + +* Good, because it's easier to debug errors. +* Good because `get` doesn't require a lock. +* Bad because `insert` and `remove` requires a lock. +* Bad, because it requires consumers to depend on `append-only-vec`. + However, this is a quite small crate. +* Good, because it works better with Javascript +* Good, because it works with any type, not just `Arc`. + +### [Option 4] Use a 3rd-party crate to pass Rust objects across the FFI + +* Good, because it's easier to debug errors. +* Bad, because it requires consumers to take this dependency. +* Bad, because it makes it harder to implement custom functionality. + For example, supporting clone to fix https://github.com/mozilla/uniffi-rs/issues/1797 or adding a foreign bit to improve trait interface handling. diff --git a/docs/handles.md b/docs/handles.md new file mode 100644 index 0000000000..4904f29aca --- /dev/null +++ b/docs/handles.md @@ -0,0 +1,174 @@ +# How do UniFFI handles and handle maps work? + +UniFFI uses handles to pass Rust objects across the FFI to the foreign code. +The handles point to an entry inside a `HandleMap` + +## HandleMap + +A `HandleMap` is a `Vec` where each item is either: + +- **Occupied** + - The foreign side holds a handle that's associated with the entry. + - Stores a `T` value (typically an `Arc<>`). + - Stores a `generation` counter used to detect use-after-free bugs + - Stores a `map-id` value used to detect handles used with the wrong `HandleMap` +- **Vacant** + - Each vacant entry stores the index of the next vacant entry. + These form a linked-list of available entries and allow us to quickly allocate a new entry in the `HandleMap` + - Stores the `generation` counter value from the last time it was occupied, or 0 if it was never occupied. + +Furthermore, each `HandleMap` stores its own `next` value, which points to the first vacant entry on the free list. +The value u32::MAX indicates indicates there is no next item and is represented by the `EOL` const. + +Here's an example `HandleMap`: + +``` +---------------------------------------------------------------- +| OCCUPIED | VACANT | OCCUPIED | +| item: Foo | next: EOL | item: Bar | +| generation: 100 | generation: 40 | generation: 30 | +---------------------------------------------------------------- + +HandleMap.next: 1 +``` + +### Inserting entries + +To insert a new entry: + - Remove the first entry in the free list, + - Convert it to an `OCCUPIED` + - Increment the generation counter + +For example inserting a `Baz` entry in the above `HandleMap` would result in: + +``` +---------------------------------------------------------------- +| OCCUPIED | OCCUPIED | OCCUPIED | +| item: Foo | item: Baz | item: Bar | +| generation: 100 | generation: 41 | generation: 30 | +---------------------------------------------------------------- + +HandleMap.next: EOL +``` + +If there are no vacant entries, then we append an entry to the end of the list. +For example, inserting a `Qux` entry in the above `HandleMap` would result in: + +``` +------------------------------------------------------------------------------------- +| OCCUPIED | OCCUPIED | OCCUPIED | OCCUPIED | +| item: Foo | item: Baz | item: Bar | item: Qux | +| generation: 100 | generation: 41 | generation: 30 | generation: 0 | +------------------------------------------------------------------------------------- + +HandleMap.next: EOL +``` + +### Removing entries + +To remove an entry: + - Convert it to `VACANT` + - Add it to the head of the free list. + +For example, removing the `Foo` entry from the above handle map would result in: + +``` +------------------------------------------------------------------------------------- +| VACANT | OCCUPIED | OCCUPIED | OCCUPIED | +| next: EOL | item: Baz | item: Bar | item: Qux | +| generation: 100 | generation: 41 | generation: 30 | generation: 0 | +------------------------------------------------------------------------------------- + +HandleMap.next: 0 +``` + +Removing the `Bar` entry after that would result in: + +``` +------------------------------------------------------------------------------------- +| VACANT | OCCUPIED | OCCUPIED | OCCUPIED | +| next: EOL | item: Baz | next: 0 | item: Qux | +| generation: 100 | generation: 41 | generation: 30 | generation: 0 | +------------------------------------------------------------------------------------- + +HandleMap.next: 2 +``` + +### Getting entries + +When an entry is inserted, we return a `Handle`. +This is a 64-bit integer, segmented as follows: +- Bits 0-32: `Vec` index +- Bit 32: foreign bit that's set for handles for foreign objects, but not Rust objects. + This allows us to differentiate trait interface implementations. +- Bits 33-40: map id -- a unique value that corresponds to the map that generated the handle +- Bits 40-48: generation counter +- Bits 48-64: unused + +When the foreign code passes the Rust code a handle, we use it to get the entry as follows: + +- Use the index to get the entry in the raw `Vec` +- Check that the entry is `OCCUPIED`, the generation counter value matches, and the map_id matches. +- Get the stored item and do something with it. Usually this means cloning the `Arc<>`. + +These checks can usually ensure that handles are only used with the `HandleMap` that generated them and that they aren't used after the entry is removed. +However, this is limited by the bit-width of the handle segments: + +- Because the generation counter is 8-bits, we will fail to detect use-after-free bugs if an entry has been reused exactly 256 items or some multiple of 256. +- Because the map id is only 7-bits, we may fail to detect handles being used with the wrong map if we generate over 128 `HandleMap` tables. + This can only happen if there more than 100 user-defined types and less than 1% of the time in that case. + +### Handle map creation / management + +The Rust codegen creates a static `HandleMap` for each object type that needs to be sent across the FFI, for example: + - `HandleMap>` for each object type exposed by UniFFI. + - `HandleMap>` for each trait interface exposed by UniFFI. + - `HandleMap>>` for each FFI type. This is used to implement async Rust functions. + - `HandleMap>` for each FFI type. This will be used to implement async callback methods. + +The `HandleAlloc` trait manages access to the static `HandleMap` instances and provides the following methods: + - `insert(value: Self) -> Handle` insert a new entry into the handle map + - `remove(handle: Handle) -> Self` remove an entry from the handle map + - `get(handle: Handle) -> Self` get a cloned object from the handle map without removing the entry. + - `clone_handle(handle: Handle) -> Handle` get a cloned handle that refers to the same object. + +If the user defines a type `Foo` in their interface then: + - ` as HandleAlloc>::insert` is called when lowering `Foo` to pass it across the FFI to the foreign side. + - ` as HandleAlloc>::get` is called when lifting `Foo` after it's passed back across the FFI to Rust. + - ` as HandleAlloc>::clone_handle` is called when the foreign side needs to clone the handle. + See https://github.com/mozilla/uniffi-rs/issues/1797 for why this is needed. + - ` as HandleAlloc>::remove` is called when the foreign side calls the `free` function for the object. + +Extra details: + - The trait is actually `HandleAlloc`, where `UT` is the "UniFFI Tag" type. See `uniffi_core/src/ffi_converter_traits.rs` for details. + - The last two `HandleAlloc` methods are only implemented for `T: Clone`, which is true for the `Arc<>` cases, but not `oneshot::Sender`. + This is fine because we only use `insert`/`remove` for the `oneshot::Sender` case. + +### Concurrency + +`insert` and `remove` require serialization since there's no way to atomically update the free list. +In general, `get` does not require any serialization since it will only read occupied entries, while `insert` and `remove` only access vacant entries. +However, there are 2 edge cases where `get` does access the same entries as `insert`/`remove`: + + - If `insert` causes the Vec to grow this may cause the entire array to be moved, which will affect `get` + - If the foreign code has a use-after-free bug, then `get` may access the same entry as an `insert`/`remove` operation. + +UniFFI uses the following system to handle this: + +- A standard `Mutex` is used to serialize `insert` and `remove`. +- We use the `append_only_vec` crate, which avoids moving the array when the `Vec` grows. +- Each entry has a 8-bit read-write spin-lock to avoid issues in the face of use-after-free bugs. + This lock will only be contested if there's a use-after-free bug. + +### Concurrency: alternative option if we choose 2 from the ADR. This one is simpler, but slower. + +To allow concurrent access, a `RwLock` is used to protect the entire `HandleMap`. +`insert` and `remove` acquire the write lock while accessing entries acquires the read lock. + +### Space usage + +The `HandleMap` adds an extra 64-bits of memory to each occupied item, which is the lower-limit on a 64-bit machine. +This means that `HandleMap` tables that store normal `Arc` pointers add ~100% extra space overhead and ones that store wide-pointers add ~50% overhead. +`HandleMap` tables don't have any way of reclaiming unused space after items are removed. + +This is can be a very large amount of memory, but in practice libraries only generate a relatively small amount of handles.