Fix JNA bug MethodTooLargeException by splitting big UniffiLib interface

[Sajjon]

This PR solves #2340 Kotlin bug which is caused by a limit in JNA causing Kotlin generated code to throw MethodTooLargeException at runtime.

Note

The name MethodTooLargeException is a red herring! This is super confusing since it is, in fact, not at all
any method being too large, it is an interface being too large, specifically the UniffiLib.

Tip

Apart from all unit tests passing locally:
I've asserted that this change works. See failing Kotlin (JVM) tests in Sargon CI before this fix (many jdk.internal.org.objectweb.asm.MethodTooLargeException all over the place) and see successful CI when I use my UniFFI fork with this fix

We at Radix got hit with this bug because apparently Sargon is the worlds biggest UniFFI crate with Kotlin binding :). The reason why Mozilla (or any other company) has not been hit with this JNA bug is probably because you use a multi-crate setup - Sargon does not (yet).

This PR pushes the limit of methods/functions in a UniFFI Kotlin project by a factor of ~2x. The solution is actually quite simple:

• I've moved all uniffi_***_uniffi_checksum_func_*** functions and the check contract version function out from interface UniffiLib and put them into a new interface IntegrityCheckingUniffiLib
• We note that this is an unintrusive change, since when UniffiLib's singleton INSTANCE is loaded the checksums is just called once. And not referenced from anywhere else.
• I've modified the internal val INSTANCE: UniffiLib lazy init, to load the library with loadIndirect first as IntegrityCheckingUniffiLib and then call uniffiCheckContractApiVersion and then uniffiCheckApiChecksums and then load the library again with loadIndirect as UniffiLib.
• I've tried calling loadIndirect just once, and casting it, but that does not work since JNA load creates a proxy instance which cannot be cast later on, however...
• ... the cost of calling loadIndirect is ~50ms on JVM macOS Sonoma | Mac Studio M1 Max (slower on slow Android devices ofc). Since this is a one time cost (well two times now) and typically still fast, this seems acceptable.

Changes expressed in generated Kotlin Code

+ internal interface IntegrityCheckingUniffiLib: Library {
+     fun ffi_uniffi_contract_version(): FfiFunction
+
+     fun uniffi_sargon_uniffi_checksum_func_new_foobar1(): Short
+     fun uniffi_sargon_uniffi_checksum_func_new_foobar2(): Short
+ }

internal interface UniffiLib: Library {
    companion object {
        internal val INSTANCE: UniffiLib by lazy {
+            val componentName = "{{ ci.namespace() }}" // PR Comment: e.g. "sargon"
+            loadIndirect<IntegrityCheckingUniffiLib>(componentName)
+                .also { lib: IntegrityCheckingUniffiLib ->
+                    uniffiCheckContractApiVersion(lib)
+                    uniffiCheckApiChecksums(lib)
+                }
-            loadIndirect<UniffiLib>("sargon")
+            loadIndirect<UniffiLib>(componentName)
-                .also { lib: UniffiLib ->
-                   uniffiCheckContractApiVersion(lib)
-                   uniffiCheckApiChecksums(lib)
-                }
+ /* COND */     .also { lib: UniffiLib -> // PR Comment: only included if `initialization_fns()` isnt empty
+ /* COND */          // initialization functions 
+ /* COND */      }
        }
    }

-   fun ffi_uniffi_contract_version(): FfiFunction

-   fun uniffi_sargon_uniffi_checksum_func_new_foobar1(): Short
-   fun uniffi_sargon_uniffi_checksum_func_new_foobar2(): Short
}

- private fun uniffiCheckContractApiVersion(lib: UniffiLib) {
+ private fun uniffiCheckContractApiVersion(lib: IntegrityCheckingUniffiLib) {

- private fun uniffiCheckApiChecksums(lib: UniffiLib) {
+ private fun uniffiCheckApiChecksums(lib: IntegrityCheckingUniffiLib) {

[Sajjon]

Let me know if I should adjust interface UniffiLibChecksums to contain the ffi_uniffi_contract_version function so that we can call uniffiCheckContractApiVersion(lib) inside this .also block, which would be closer to what we do on main branch (call uniffiCheckContractApiVersion before uniffiCheckApiChecksums).

Alternatively I can change to this to ensure uniffiCheckContractApiVersion is called before uniffiCheckApiChecksums:

internal val INSTANCE: UniffiLib by lazy {
    val lib =  loadIndirect<UniffiLib>(componentName)
        .also { lib: UniffiLib ->
            uniffiCheckContractApiVersion(lib)
        }
    
    // check checksums
    loadIndirect<UniffiLibChecksums>(componentName)
       .also { lib: UniffiLibChecksums ->
            uniffiCheckApiChecksums(lib)
       }
    
    lib // return lib
}

Let me know what you prefer:

1. keep as is
2. move ffi_uniffi_contract_version into UniffiLibChecksums (and maybe rename the interface UniffiLibChecks)
3. change internal val INSTANCE: UniffiLib so that we load lib: UniffiLib lib first then do checksum checks and return lib

[bendk]

I prefer 2, since a contract version change could potentially affect the checksum function signature. However, if this makes the interface code more complicated, it's not worth it.

[Sajjon]

I will give it (option 2) a go and will ping you tomorrow

[Sajjon]

@bendk OK I've renamed to new lib IntegrityCheckingUniffiLib (instead of UniffiLibChecksums) since it now also performs the contract version check. I think "integrity check" is OK terminology to describe the aggregation of "contract version check" and "checksum checks".

[Sajjon]

@bendk I've ensured to not get lint errors (which I got at first) if lib: UniffiLib is not used - which can be the case now if initialization_fns() is empty.

I've updated to only conditionally include the .also after loading lib, only if initialization_fns() is not empty.

I think that is the best solution. Let me know if you are happy with it.

[micbakos-rdx]

We run some tests with @Sajjon measuring the time difference it takes to resolve the INSTANCE on our wallet

We didn't notice any relative performance hit. We built sargon-uniffi crate in debug mode and exported two android aar libraries. One without the fix, but with lower amount of methods (so as not to hit the MethodTooLargeException) and one with the fix.

The resulting android app was also built in debug mode and run on android emulator and in a relatively low spec Samsung device (Samsung A53). Keep in mind that these measurements are always inflated in debug mode.

Emulator: ~1ms of max difference (sometimes the instance was resolved quicker with the fix). (65.8ms ~ 66.8ms)
Samsung A53: ~12ms of max difference (sometimes the instance was resolved quicker with the fix). (247ms ~ 259ms)

Although we were not able to minimise the noise affected by other factors (especially with the real device), we didn't observe any noticeable performance hit.

[Sajjon]

@bendk let me know if you need something else to get this merged! :)

[bendk]

Thanks for doing this. Hopefully this is enough to avoid the issue although I guess if your library was 2x as big then you'd run into it again. I'm flagging this as requesting changes, just because there was the question of how to handle the library version check. If you think the current way is the cleanest, just say so and I'm happy to take it as-is.

I'm really wondering if all these checksum checks are worth the complexity, but I don't think we're going to make a decision on that soon. In the meantime, this seems like a good workaround.

[bendk]

I prefer 2, since a contract version change could potentially affect the checksum function signature. However, if this makes the interface code more complicated, it's not worth it.

[Sajjon]

@bendk

Hopefully this is enough to avoid the issue although I guess if your library was 2x as big then you'd run into it again.

Yes if we were to 2x our lib we would be hit by this again, that is true. But I expect us to grow our API with 30-50% only (i.e. less than 100%) in the coming months, and then actually decrease the UniFFI exported API "surface" as we are able to use more complex flows in Rust, thus reducing need for some methods and records :)

For the future

Tip

If future readers are hit by MethodTooLargeException after this PR has been merged we
Can fix it, and support even bigger libs.
Just not sure we want to. You probably should start using multi-crate! (as we in Radix should...)

We can note that it is possible to accommodate even bigger libraries if we were to split both IntegrityCheckingUniffiLib and UniffiLib lib in many interfaces each. This will be easy for IntegrityCheckingUniffiLib. It is sliiightly tricker for UniffiLib since we would need to know which "batch" of UniffiLibBatchX we would call to for our FFI functions/methods, e.g.:

fun gradient(ln: Line): kotlin.Double {
    FfiConverterDouble.lift(
        uniffiRustCall { _status ->
            UniffiLibBatch1.INSTANCE.uniffi_uniffi_geometry_fn_func_gradient(
                FfiConverterTypeLine.lower(ln), _status
            )
        }
    )
}

Should it be UniffiLibBatch1.INSTANCE or UniffiLibBatch2.INSTANCE ? We would need to keep track of that. Not hard, just some more changes... Maybe would would be able to know it deterministically through a hash of the signature and then modulus and map to BatchX or something like that. Or maybe OK to keep a Map in Rinja of method/function -> Batch number.

[mhammond]

@skeet70 do you have any thoughts here? uniffi-bindgen-java will have the same problem I assume?

[skeet70]

Ah that's unfortunate, along with the fix introducing more initial delay and if needed again doesn't scale well. I don't have a better solution though. You're right @mhammond that the Java bindings are likely affected by the same bug, I'll make a ticket. All classes/interfaces are split out into individual files there, but if the problem is a single interface getting too large, it'll still be an issue.

[bendk]

We can note that it is possible to accommodate even bigger libraries if we were to split both IntegrityCheckingUniffiLib and UniffiLib lib in many interfaces each. This will be easy for IntegrityCheckingUniffiLib. It is sliiightly tricker for UniffiLib since we would need to know which "batch" of UniffiLibBatchX we would call to for our FFI functions/methods, e.g:...

I think #2333 might help with this if we ever needed it (especially after I make some changes suggested by @mhammond). The general idea is to add a transformation pass where we convert the generalized interface into the Kotlin-specific interface. Part of that could be to take the FFI functions and split them up into different batches and also associate the batch number with each FFI function reference.

[bendk]

The changes look great to me, I'd be good with merging. Are there any remaining concerns?

[Sajjon]

The changes look great to me, I'd be good with merging. Are there any remaining concerns?

Good to merge from my perspective, what say @mhammond ?