POC for step 1 of #419 - replace Handlemaps with Arcs. #420

mhammond · 2021-03-30T04:26:29Z

The context for this is Ryan's proposal for passing around object instances - which has steps 1 and 2 in a rough way - replacing Handlemaps with Arc<> (or to-be-deprecated Arc<Mutex<>>) and allowing them to be args and results.

This is turning out better than I hoped - for example, see this new test which exercises most of what this does.

I think the Kotlin generation works, but the Swift generation most certainly does not. @jhugman now you are back from PTO I'd love your feedback and to gauge your interest in collaborating on this? Note that my intent is to iterate slowly, just in time for Ryan to get back and review.

rfk

This looks pretty neat! I'm kind of strangely glad to see a SIGSEGV in the circle failures here, means we're definitely in raw pointer territory :-)

I'd like to consider whether we can embed the use of Arc here more thoroughly in the mechanics of how we work with object instances, so will share some rambling thoughts about how that might look architecturally.

First, observe that when we generate the support code for records, we generate an implementation of the ViaFfi trait that knows how to read/write then in a buffer. We currently do not implement ViaFfi for object types at all.

What if we did update ObjectTemplate.rs to produce a ViaFfi implementation for object types? It might look something like this:

// This won't work as written IIRC, because this code doesn't define `ViaFfi` and
// doesn't define `Arc`, so Rust's trait coherence rules will stop us from impling
// the trait on `Arc<ObjectName>`. But it's interesting to think about architecturally.
// Maybe we could define a newtype wrapper that would let this work?

unsafe impl uniffi::ViaFfi for Arc<ObjectName> {

    // When passing object instances over the FFI, we pass a pointer
    // that is the raw form of an Arc<ObjectName> owned by the foreign-language code.
    type FfiType = *const ObjectName;

    // If we return an `Arc<ObjectName>` to the foreign-language code, then it
    // owns the reference and is responsible for freeing it.
    fn lower(self) -> Self::FfiType {
        self.into_raw()
    }

    // If we want to receive an `Arc<ObjectName>` from the foreign-language code,
    // we have to make our own clone of it.
    fn try_lift(v: Self::FfiType) -> Result<Self> {
        // The received pointer is an Arc owned by foreign-language code, don't drop it.
        let foreign_arc = ManuallyDrop::new(unsafe { Self::from_raw(v) });
        // Take a (cheap) clone for our own use.
        Ok(Arc::clone(foreign_arc))
    }

    // If we want to send a data structure with an `Arc<ObjectName>` in one of its
    // fields, we pass ownership of a clone by passing the pointer.
    fn write<B: bytes::BufMut>(&self, buf: &mut B) {
        let foreign_arc = Arc::clone(self);
        buf.put_u64(foreign_arc.into_raw() as u64) // Probably need some sizeof() checks here :-/
    }

    // If we receive a serialized data structure with an `Arc<ObjectName>` in one of its
    // fields, we need to take a clone for our own purposes.
    fn try_read<B: bytes::Buf>(buf: &mut B) -> Result<Self> {
      Self::try_lift(buf.get_u64() as const* ObjectName)
    }
}

I wonder if something like this would help make the generated code any cleaner?

rfk · 2021-03-30T05:10:21Z

uniffi_bindgen/src/interface/object.rs

@@ -232,6 +232,7 @@ impl Constructor {
        self.ffi_func.name.push_str("_");
        self.ffi_func.name.push_str(&self.name);
        self.ffi_func.arguments = self.arguments.iter().map(Into::into).collect();
+        // XXX - we need this to be some kind of pointer abstraction - `usize`?


If we're going to work with raw pointers, I think the right thing to do here is add an FFIType::Pointer variant and then map it explicitly to the corresponding pointer abstractions in each foreign language. (The existing FFITypeRustCString variant actually already is sort of this, but specifically for a pointer-to-char).

uniffi_bindgen/src/templates/ObjectTemplate.rs

rfk · 2021-03-30T05:15:25Z