Added ffi-buffer scaffolding functions #2039

bendk · 2024-03-18T20:01:06Z

These are alternate versions of the current scaffolding functions that use u64 buffers to handle their input/output.

The main use case is the gecko-js bindings used in Firefox. This allows us to replace the generated C++ code with static code, since the scaffolding functions now always have the exact same signature.

I added ffi buffer versions of all generated scaffolding functions. I didn't make them for "static" functions, like the rust_buffer FFI functions. I don't think we need them there since the signature is always the same.

Added do not merge because I want to test that this will actually work first, but the code is ready for review.

mhammond · 2024-03-29T02:07:08Z

That's crazy! You could refactor the bindings to use only this capability :) Not that you would, but it's just an extra level of ffi indirection over the same ffi.

bendk · 2024-03-29T13:34:25Z

That's crazy! You could refactor the bindings to use only this capability :) Not that you would, but it's just an extra level of ffi indirection over the same ffi.

I've thought about the same thing and at points have had the urge to say we should just move all languages to using this. But then I think about the converse of your point: it's the essentially the same FFI so there's not much benefit to switching over the existing bindings. That said, it could help us avoid some limitations, like the fact that ctypes doesn't let you return structures or the JNA bugs we've dealt with. I don't think that makes it worth it, but it's interesting to think about.

mhammond · 2024-04-01T20:05:50Z

should just move all languages to using this.

I do kinda like the idea that today, a function that only uses "ffi-native" types as args or return types ends up with the "correct and expected" signature - ie, i32 func(i32, i64) meaning there's a great story to be told about how uniffi isn't particularly inefficient for the ffi until you start using richer types. I'm sure you would struggle to measure any such difference in practice, but it seems like a nice story to tell.

bendk · 2024-04-12T13:58:28Z

The more I think about it, the more I wonder if this could be a good system for bindings in general.

I do kinda like the idea that today, a function that only uses "ffi-native" types as args or return types ends up with the "correct and expected" signature - ie, i32 func(i32, i64) meaning there's a great story to be told about how uniffi isn't particularly inefficient for the ffi until you start using richer types. I'm sure you would struggle to measure any such difference in practice, but it seems like a nice story to tell.

This is true for Swift, but for Python and Kotlin it's not so simple because the libffi API uses a similar buffer to pass function arguments. If I compare this system to the current system, I think there will still be some overhead, since now we're using an extra layer of argument buffers, but it's probably small. A simpler FFI layer for Python and Kotlin could unblock some performance wins, like using switching from using RustBuffers for dictionaries/enums and using structs/tagged unions instead.

I think that the system should still be additive in the sense that we should still export the current scaffolding functions for languages like Swift that can call them directly and can handle the fiddly C bits.

badboy

Do you already have some example code in gecko-js how this would be used?

uniffi_core/src/ffi/ffiserialize.rs

badboy · 2024-04-19T09:24:45Z

uniffi_core/src/ffi/ffiserialize.rs

+    }
+
+    /// Write a value to a u64 buffer ref and advance it
+    fn write(buf: &mut &mut [u64], value: Self) {


&mut &mut [u64] -- that's a type you don't see often.

I got that from the bytes crate, that implements a bunch of &mut methods on &mut u8.

badboy · 2024-04-19T09:28:33Z

uniffi_core/src/ffi/ffiserialize.rs

+    u32,
+    i64,
+    u64,
+    *const std::ffi::c_void,


followers of pointer provenance will not like us for this.

I'm not that comfortable with it myself. That's one of the reasons I want to move away from pointers and towards handles. Someday I'll get back to working on #1823.

I'm thinking that maybe instead of making the elements all u64 it should be a C union of all the "primitive" FFI types (u8 - i64, float, double and void *). That way we don't need to cast things so much. In addition to messing with pointer provenance, it's actually not so trivial to do the reverse casts on the C++ side for the int types. The floating point casts also get complicated, since C++ doesn't technically have a f32 or f64 type before C++ 23.

badboy · 2024-04-19T09:29:19Z

uniffi_core/src/ffi/ffiserialize.rs

+    const SIZE: usize = 1;
+
+    fn get(buf: &[u64]) -> Self {
+        // Safety: this relies on the foreign code passing us valid pointers


Uff. Dangerous.
But I guess not the only time we assume stuff like this in UniFFI.

On top of unsafety this also makes up a lifetime, doesn't it? We better prove all usage of this correct.

Good point. I think we maybe can get rid of this one. I think the only reason we needed to support references was the &mut RustCallStatus out param. But my last force-push got rid of that. If we're going with the ffi-buffer system then it's simpler to write a tuple into the return buffer vs. inputing an out-param.

bendk · 2024-04-19T14:21:44Z

Do you already have some example code in gecko-js how this would be used?

Coming soon...

bendk · 2024-04-22T20:34:48Z

I put some more work on this over the last week or so and I think it's ready to start talking about this for real:

See https://phabricator.services.mozilla.com/D208231 for how this would change the gecko-js bindings. There's a lot of changes there, but it seems positive to me.
I changed the buffer elements from being u64 to being a union of the various primitive types. Unions are dangerous and fiddly by nature, but less so than trying to manually cast things to u64 values.
We no longer generate ffi-buffer versions for clone/free and checksum functions. When I started using this in moz-central, I realized I had no need for ffi-buffer versions of those functions since the signature was always the same. In general, I think we only to generate ffi-buffer versions for scaffolding funcitons where the signature can change.

bendk · 2024-04-22T21:46:21Z

I'm really confused as to why these tests are failing. cargo test -p uniffi-fixture-ext-types --doc passes when I run it locally (reporting 0 tests run). I can't see how these changes would cause that test to fail either. Does it work when others run it locally?

mhammond · 2024-04-29T20:52:40Z

I'm really confused as to why these tests are failing. cargo test -p uniffi-fixture-ext-types --doc passes when I run it locally (reporting 0 tests run). I can't see how these changes would cause that test to fail either. Does it work when others run it locally?

No! The command you specify does what you said it does, but a plain cargo test eventually generates:

   Doc-tests uniffi_ext_types_lib
error[E0463]: can't find crate for `uniffi_sublib`
  --> fixtures/ext-types/lib/src/lib.rs:11:5
   |
11 | use uniffi_sublib::SubLibType;
   |     ^^^^^^^^^^^^^ can't find crate

error: aborting due to 1 previous error

For more information about this error, try `rustc --explain E0463`.
error: doctest failed, to rerun pass `-p uniffi-fixture-ext-types --doc`

So still with the same bad advice about how to re-run it, but the error does appear to be real?

mhammond

This seems great to me! I've a question re features that I don't think we need to block on, and you still have that odd test issue, but 👍

mhammond · 2024-05-05T19:36:33Z

uniffi_core/src/ffi/rustbuffer.rs

@@ -2,7 +2,10 @@
 * 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::ffi::{rust_call, ForeignBytes, RustCallStatus};
+use crate::{


should uniffi_core also grow a feature which enables this?

bendk · 2024-05-06T16:33:30Z

Still not sure what's causing the doctest failure, the crates compile file when you build them and when you run the normal tests. I'm just going to change the ext-types fixture to not enable the feature.

These are alternate versions of the current scaffolding functions that use u64 buffers to handle their input/output. The main use case is the gecko-js bindings used in Firefox. This allows us to replace the generated C++ code with static code, since the scaffolding functions now always have the exact same signature. This commit generates FFI buffer versions for scaffolding functions defined for user functions/methods. It does not generate them for functions with known/static signatures like the rust buffer FFI functions, or the object clone/delete functions. If the signature is always the same, then there isn't a problem calling the normal scaffolding functions.

bendk added the do not merge label Mar 18, 2024

bendk requested a review from a team as a code owner March 18, 2024 20:01

bendk requested review from skhamis and removed request for a team March 18, 2024 20:01

bendk force-pushed the ffi-buffer branch 3 times, most recently from 9d6063a to 53832f6 Compare March 18, 2024 21:15

bendk force-pushed the ffi-buffer branch from 53832f6 to 37710de Compare March 29, 2024 17:35

skhamis requested review from a team and mhammond and removed request for skhamis and a team April 1, 2024 18:49

bendk force-pushed the ffi-buffer branch 2 times, most recently from 0582bde to 43f2890 Compare April 11, 2024 19:58

bendk force-pushed the ffi-buffer branch 5 times, most recently from c31a83a to 2dcd78a Compare April 19, 2024 13:46

badboy reviewed Apr 19, 2024

View reviewed changes

bendk force-pushed the ffi-buffer branch from 0403bcd to 3f8ba5b Compare April 19, 2024 14:20

bendk force-pushed the ffi-buffer branch 4 times, most recently from 09397a5 to e142976 Compare April 22, 2024 20:29

bendk removed the do not merge label Apr 22, 2024

bendk force-pushed the ffi-buffer branch from e142976 to bc2793a Compare April 22, 2024 21:17

mhammond approved these changes May 5, 2024

View reviewed changes

bendk force-pushed the ffi-buffer branch from bc2793a to 89cc98a Compare May 6, 2024 16:32

bendk force-pushed the ffi-buffer branch from 89cc98a to a4dae7e Compare May 6, 2024 16:57

bendk merged commit 96453d7 into mozilla:main May 6, 2024
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added ffi-buffer scaffolding functions #2039

Added ffi-buffer scaffolding functions #2039

bendk commented Mar 18, 2024

mhammond commented Mar 29, 2024

bendk commented Mar 29, 2024

mhammond commented Apr 1, 2024

bendk commented Apr 12, 2024

badboy left a comment

badboy Apr 19, 2024

bendk Apr 19, 2024

badboy Apr 19, 2024

bendk Apr 19, 2024

bendk Apr 22, 2024

badboy Apr 19, 2024

bendk Apr 19, 2024 •

edited

Loading

bendk commented Apr 19, 2024

bendk commented Apr 22, 2024

bendk commented Apr 22, 2024

mhammond commented Apr 29, 2024

mhammond left a comment

mhammond May 5, 2024

bendk commented May 6, 2024

Added ffi-buffer scaffolding functions #2039

Added ffi-buffer scaffolding functions #2039

Conversation

bendk commented Mar 18, 2024

mhammond commented Mar 29, 2024

bendk commented Mar 29, 2024

mhammond commented Apr 1, 2024

bendk commented Apr 12, 2024

badboy left a comment

Choose a reason for hiding this comment

badboy Apr 19, 2024

Choose a reason for hiding this comment

bendk Apr 19, 2024

Choose a reason for hiding this comment

badboy Apr 19, 2024

Choose a reason for hiding this comment

bendk Apr 19, 2024

Choose a reason for hiding this comment

bendk Apr 22, 2024

Choose a reason for hiding this comment

badboy Apr 19, 2024

Choose a reason for hiding this comment

bendk Apr 19, 2024 • edited Loading

Choose a reason for hiding this comment

bendk commented Apr 19, 2024

bendk commented Apr 22, 2024

bendk commented Apr 22, 2024

mhammond commented Apr 29, 2024

mhammond left a comment

Choose a reason for hiding this comment

mhammond May 5, 2024

Choose a reason for hiding this comment

bendk commented May 6, 2024

bendk Apr 19, 2024 •

edited

Loading