Experiment: replace protobuf with uniffi #3900

rfk · 2021-03-03T10:43:51Z

I timedboxed a little experiment this afternoon, and it went well enough that I wanted to push it for early feedback (although it's a long way off being landable!)

Our dependency on external protobuf libraries is annoying, and it's particularly troublesome for iOS because iOS consumers need to configure SwiftProtoBuf into their app as an extra dependency. In addition, the presence of SwiftProtoBuf means we can't build our iOS binaries with BUILD_LIBRARY_FOR_DISTRIBUTION, which means iOS consumers have to be using the exact same version of Swift as our build infra, which is regularly a problem.

Our use of protobuf seems fairly entrenched, but what if it didn't have to be?

One of the things I learned from #3876 is that it's pretty easy to have hand-written Kotlin and Swift code side-by-side with code generated by UniFFI. So I wanted to see whether UniFFI could replace our current use of protobuf without having to commit to wholesafe uniffication of all our current crates.

TL;DR I think it probably could, but I'm not sure sure if it worth the effort vs doing a full uniffication.

What we have here is the places component with protobuf messages removed and uniffi dictionary types inserted in their place, on both the Rust side and the Kotlin side (I haven't looked at the swift code yet). It builds! The Kotlin tests don't pass yet though.

I'm going to leave a couple of thoughts on the draft PR here, and I'd love to hear any feedback others might have. But just to be clear: I'm pushing this entirely as a conversation topic, not for any sort of landing at this stage.

rfk

Overall, I think this approach would work. But also, it's not clear to me how much effort it would save versus fully converting the FFI over to using UniFFI. Maybe it would be useful if there are some tricky bits in the FFI that UniFFI isn't well set up to handle just yet.

Still, an interesting experiment! And the promise of letting us decouple from the firefox-ios Swift version may be a nice motivator.

rfk · 2021-03-03T10:48:42Z

components/places/android/src/main/java/mozilla/appservices/places/Bookmarks.kt

+            this.url,
+            emptyList(),
+            null
+        )


The protobuf builder pattern is much nicer than a bunch of null fields, but OTOH, this whole BookmarkNode class is custom designed to work with the grain of protobuf; more thoughts on that below.

rfk · 2021-03-03T10:50:00Z

components/places/android/src/main/java/mozilla/appservices/places/Bookmarks.kt

                    title = title,
-                    url = msg.url
+                    url = node.url!!


It's not clear to me why the protobuf version of this code didn't have to check for the presence of these fields, similar to the null checks I'm doing here. I feel like maybe protobuf just gives us a default value if some bug means that the expected field was not present? Weird spooky action at a distance...

huh, yeah, how the "old" version works here is beyond me

rfk · 2021-03-03T10:50:51Z

components/places/android/src/main/java/mozilla/appservices/places/LibPlacesFFI.kt

@@ -27,55 +26,55 @@ internal interface LibPlacesFFI : Library {
    /** Create a new places api */
    fun places_api_new(
        db_path: String,
-        out_err: RustError.ByReference
+        out_err: RustError2.ByReference


We could probably use the uniffi-generated RustError class here, with a bit of tweaking. I just punted on that for now.

rfk · 2021-03-03T10:51:36Z

components/places/android/src/main/java/mozilla/appservices/places/LibPlacesFFI.kt

    ): Pointer?

    fun places_pinned_sites_import_from_fennec(
        handle: PlacesApiHandle,
        db_path: String,
-        out_err: RustError.ByReference
+        out_err: RustError2.ByReference
    ): RustBuffer.ByValue


By contrast, this is using the uniffi-generated RustBuffer class.

rfk · 2021-03-03T10:52:30Z

components/places/android/src/main/java/mozilla/appservices/places/PlacesConnection.kt

+// Different names between Rust and Kotlin, for...reasons?
+typealias VisitType = VisitTransition
+typealias VisitInfo = HistoryVisitInfo
+typealias VisitInfosWithBound = HistoryVisitInfosWithBound


We do this a lot, where we give things slightly different names on the different sides of the API boundary, but it's not clear to me what motivates any particular naming choice.

FWIW, I'm also not sure where these differences come from, but IMO they are imposing a cost we should try and avoid. I'm somewhat use to expecting camelCase etc differences, but these gratuitous naming difference just cause confusion.

rfk · 2021-03-03T11:05:18Z

components/places/src/places.udl

-/** An array of bookmark nodes, since we can't represent that directly */
-message BookmarkNodeList {
-    repeated BookmarkNode nodes = 1;
-}


We also don't need these special "holder for a list of T" types because we can just have sequence<T>.

rfk · 2021-03-03T11:05:58Z

components/places/src/places.udl

+    string title;
+    string? icon_url;
+    i64 frecency;
+    sequence<MatchReason> reasons;


Some of the fields on SearchResult here not being returned over the FFI, such as icon_url. Should they be?

rfk · 2021-03-03T11:07:47Z

components/places/src/places.udl

+};
+
+// This just makes sure we have some extra helper functions available,
+// to be called from our hand-written bindings.


Now it's UniFFI's turn for some weirdness workarounds!

UniFFI tries not to generate helper code for types that your API doesn't use. We have some functions that want to return e.g. a sequence<SearchResult>, so we want UniFFI's helper code for lifting that type. But UniFFI won't generate that helper code unless it sees the type somewhere in the interface. So, we fake it here, at least until the type appears organically in the signature of a uniffied function.

rfk · 2021-03-03T11:08:46Z

components/places/src/storage/bookmarks.rs

@@ -28,7 +28,7 @@ use sync_guid::Guid as SyncGuid;
 use types::Timestamp;
 use url::Url;

-pub use public_node::PublicNode;
+pub use public_node::InternalBookmarkNode;


The naming here is wierd, I don't think the PublicNode struct ever appears in a public API, does it? It seems more like an internal transition point between the database and the actual public data (previously the message type, now the uniffi dictionary type).

rfk · 2021-03-03T11:09:13Z

components/places/src/storage/bookmarks/conversions.rs

-            Some(n.child_nodes.is_some())
-        } else {
-            None
-        };


Option<Vec<T>> FTW

mhammond

This is awesome and offers real value in our longer term. I'd also note that if you can resolve the 2 bits, the patch would be a reasonable size.

TL;DR I think it probably could, but I'm not sure sure if it worth the effort vs doing a full uniffication.

I guess I don't have any understanding of how complimentary those efforts would be, and how much extra work is involved in the "full" unification - but if it's significant, or not wasted work if we went "full" later, I'd say there's enough value here to push forward. Given the benefits are longer term, it might also be a good "onboarding" task? (Purposely ambiguous about what "it" is in that last sentence :)

mhammond · 2021-03-04T05:37:37Z

components/places/android/src/main/java/mozilla/appservices/places/Bookmarks.kt

                    title = title,
-                    url = msg.url
+                    url = node.url!!


huh, yeah, how the "old" version works here is beyond me

mhammond · 2021-03-04T05:40:15Z

components/places/android/src/main/java/mozilla/appservices/places/PlacesConnection.kt

+// Different names between Rust and Kotlin, for...reasons?
+typealias VisitType = VisitTransition
+typealias VisitInfo = HistoryVisitInfo
+typealias VisitInfosWithBound = HistoryVisitInfosWithBound


FWIW, I'm also not sure where these differences come from, but IMO they are imposing a cost we should try and avoid. I'm somewhat use to expecting camelCase etc differences, but these gratuitous naming difference just cause confusion.

mhammond · 2021-03-04T05:44:34Z

components/places/ffi/src/lib.rs

 };
+use uniffi::ViaFfi;
 use places::error::*;


It looks like type ByteBuffer = uniffi::RustBuffer would help keep the diff down

mhammond · 2021-03-04T05:46:47Z

components/places/src/api/matcher.rs

@@ -199,15 +197,15 @@ pub struct SearchResult {

    /// The URL to open when the user confirms a match. This is
    /// equivalent to `nsIAutoCompleteResult.getFinalCompleteValueAt`.
-    pub url: Url,
+    pub url: String,


autofill would (maybe) like this too - there it would be more about 'guid' where we use a guid type internally but strings with uniffi, which jars a little. I haven't looked into how hard that would be though, but just a little +1 for that concept.

mhammond · 2021-03-04T05:48:16Z

components/places/src/places.udl

+};
+
+
+enum BookmarkType {


I notice alot of the diff above is just the case changing of this enum - is there a particular reason you didn't leave the old names?

Ugh, yeah, UniFFI automagically uppercases enum variant names in the Kotlin bindings, since this seems to be the prevailing style convention for Kotlin code.

mhammond · 2021-03-04T05:50:40Z

components/places/src/places.udl

@@ -40,7 +65,7 @@ message HistoryVisitInfosWithBound {
 * Note that these docs comments are internal, and don't necessarily impact the actual
 * API we expose to Kotlin/Swift (this is particularly true around reads).
 */
-message BookmarkNode {
+dictionary BookmarkNode {
    /**


FWIW, autofill has landed on using 2 versions of the struct - one for "incoming" (eg, "create an item" where guid might be null) and one for outgoing (eg "fetch an item" where guid is never null). I assume this is used in both directions which is why everything is nullable.

(Not sure why I'm noting this TBH :)

Right, yes, I think separate "incoming" and "outgoing" would make a big difference here.

rfk · 2021-03-04T06:24:56Z

but if it's significant, or not wasted work if we went "full" later

I think it would mostly not be wasted work, but a concrete step towards more thorough uniffication.

I'd say there's enough value here to push forward. Given the benefits are longer term,
it might also be a good "onboarding" task? (Purposely ambiguous about what "it" is in that last sentence :)

Oooh, I like this idea :-)

This is intended to be a small but significant step on the road to uniffi-ing places. It takes all the hard stuff from Ryan's mozilla#3900, but limits the scope to just the new-ish history_metadata module.

rfk · 2021-07-19T02:50:34Z

There is a much less "work in progress" version of this in #4183, so I'm going to close this experimental PR in favour of that actually-working-towards-landing one.

This is intended to be a small but significant step on the road to uniffi-ing places. It takes all the hard stuff from Ryan's #3900, but limits the scope to just the new-ish history_metadata module.

This is intended to be a small but significant step on the road to uniffi-ing places. It takes all the hard stuff from Ryan's mozilla#3900, but limits the scope to just the new-ish history_metadata module.

…i. (#4183) This is intended to be a small but significant step on the road to uniffi-ing places. It takes all the hard stuff from Ryan's #3900, but limits the scope to just the new-ish history_metadata module. Co-authored-by: Ryan Kelly <[email protected]> Co-authored-by: Sammy Khamis <[email protected]>

WIP experiment with replacing protobuf with uniffi

32b175a

rfk commented Mar 3, 2021

View reviewed changes

This was referenced Mar 3, 2021

Investigation / Enhancement Request: Support building library with multiple stable XCode #3799

Closed

Eliminate duplicate declarations of helper structs and extensions in Swift mozilla/uniffi-rs#409

Merged

mhammond reviewed Mar 4, 2021

View reviewed changes

rfk mentioned this pull request Mar 26, 2021

Allow the sync manager to accept local encryption keys to pass to engines #3995

Merged

mhammond mentioned this pull request Jun 7, 2021

Replace places history_metadata's protobuf implementations with uniffi. #4183

Merged

data-sync-user mentioned this pull request Jun 22, 2021

Investigation / Enhancement Request: Support building library with multiple stable XCode mozilla/uniffi-rs#893

Closed

rfk closed this Jul 19, 2021

tarikeshaq deleted the replace-protobuf-with-uniffi branch June 8, 2023 19:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Experiment: replace protobuf with uniffi #3900

Experiment: replace protobuf with uniffi #3900

rfk commented Mar 3, 2021

rfk left a comment

rfk Mar 3, 2021

rfk Mar 3, 2021

mhammond Mar 4, 2021

rfk Mar 3, 2021

rfk Mar 3, 2021

rfk Mar 3, 2021

mhammond Mar 4, 2021

rfk Mar 3, 2021

rfk Mar 3, 2021

rfk Mar 3, 2021

rfk Mar 3, 2021

rfk Mar 3, 2021

mhammond left a comment

mhammond Mar 4, 2021

mhammond Mar 4, 2021

mhammond Mar 4, 2021

mhammond Mar 4, 2021

mhammond Mar 4, 2021

rfk Mar 4, 2021

mhammond Mar 4, 2021

rfk Mar 4, 2021

rfk commented Mar 4, 2021

rfk commented Jul 19, 2021

Experiment: replace protobuf with uniffi #3900

Experiment: replace protobuf with uniffi #3900

Conversation

rfk commented Mar 3, 2021

rfk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhammond left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rfk commented Mar 4, 2021

rfk commented Jul 19, 2021