Conversation
conversations/src/api.rs
Outdated
| /// Negative numbers symbolize an error has occured. See `ErrorCode` | ||
| /// | ||
| #[unsafe(no_mangle)] | ||
| pub unsafe extern "C" fn create_new_private_convo( |
There was a problem hiding this comment.
The interface looks complex, also the following logic looks like a lot of check and conversion.
From my previous work in DR FFI, safer_ffi seems brings good candidate to resolve unsafe methods and clear interface.
Any specific reason to keep this FFI in this style?
There was a problem hiding this comment.
I did look into safer_ffi - as on paper it would be the preferred approach.
I found that at this point the leverage the crate provides is not really being put to use. Ultimately my decision to propose using the C-style interface was:
- Our API contains minimal risk as only simple primitives are passed.
- Clarity looking at both the caller and callee sites.
- Memory ownership simplicity.
For context: here is a rough example of safer_ffi with strict type safety: jazzz/nim_api...jazzz/saferffi_version
I think there is an middle ground where we use caller provided buffers and use safer_ffi to handle wrapping those buffers into slices - I'll check that out before merging as that would be my preference
TLDR Details:
- Callee provided buffers, meant that careful memory management would be needed in calling code. Which added complexity up stream.
- The Libchat API is specifically designed such that only byte arrays and primitives are passed over the FFI.
- If we wanted to pass Results, Options, and complex string errors back to callers then safer_ffi would be very advantageous. But that is currently not the case as the only data passed in are simple byte arrays.
- Handling these structs on the nim-side is more convoluted.
- Reasoning about copies and memory layouts becomes more difficult.
- specifically since LogosCore modules are calling via C, the C-buffers here can be piped directly to Rust in future PRs to produce zero copy invocations.
- The C api approach isn't complex in my perspective, its just verbose. While it may contain many lines, there is a symmetry that is easier to follow along with especially for logos-core/nim developers who do not want to worry about Libchat internals.
- Exception: see note about middle approach.
8d1a830 to
ff34340
Compare
|
I've updated this to use safer_ffi. The intention is that functions which return Functions which return content destined for applications will use callee provided buffers. This allows the application to manage the memory, and reduce copies by forwarding pointers through into LibChat The overall code is safer without incurring multiple unnecessary copies. There are many improvements to be made, but I'll defer those until after the API sees some real testing. |
031ccf5 to
f6c3467
Compare
conversations/src/context.rs
Outdated
| inbox, | ||
| buf_size: 0, | ||
| convo_handle_map: HashMap::new(), | ||
| next_convo_handle: 0xF5000001, |
There was a problem hiding this comment.
Should be a constant defined outside of new, and maybe a little comment.
This PR adds the Rust side api for creating Private V1 conversations.
The api is implemented using Caller provided buffers, to avoid memory management across the FFI.
ConvoHandlewhich acts as an opaque handle for conversations which are based over the FFI interface. Passing strings is costly and adds complexity.create_intro_bundle,create_new_private_convoNotes