Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MethodTooLargeException runtime error when executing Android tests for our Kotlin artifact #2340

Closed
Sajjon opened this issue Dec 3, 2024 · 11 comments

Comments

@Sajjon
Copy link
Contributor

Sajjon commented Dec 3, 2024

I recently started getting MethodTooLargeException errors, e.g.

jdk.internal.org.objectweb.asm.MethodTooLargeException: Method too large: jdk/proxy3/$Proxy28.<clinit> ()V
	at java.base/jdk.internal.org.objectweb.asm.MethodWriter.computeMethodInfoSize(MethodWriter.java:2118)
	at java.base/jdk.internal.org.objectweb.asm.ClassWriter.toByteArray(ClassWriter.java:527)
	at java.base/java.lang.reflect.ProxyGenerator.generateClassFile(ProxyGenerator.java:506)
	at java.base/java.lang.reflect.ProxyGenerator.generateProxyClass(ProxyGenerator.java:178)
	at java.base/java.lang.reflect.Proxy$ProxyBuilder.defineProxyClass(Proxy.java:558)
	at java.base/java.lang.reflect.Proxy$ProxyBuilder.build(Proxy.java:670)
	at java.base/java.lang.reflect.Proxy.lambda$getProxyConstructor$0(Proxy.java:429)
	at java.base/jdk.internal.loader.AbstractClassLoaderValue$Memoizer.get(AbstractClassLoaderValue.java:329)
	at java.base/jdk.internal.loader.AbstractClassLoaderValue.computeIfAbsent(AbstractClassLoaderValue.java:205)
	at java.base/java.lang.reflect.Proxy.getProxyConstructor(Proxy.java:427)
	at java.base/java.lang.reflect.Proxy.newProxyInstance(Proxy.java:1037)
	at com.sun.jna.Native.load(Native.java:624)
	at com.sun.jna.Native.load(Native.java:596)
	at com.radixdlt.sargon.UniffiLib$Companion$INSTANCE$2.invoke(sargon.kt:59715)
	at com.radixdlt.sargon.UniffiLib$Companion$INSTANCE$2.invoke(sargon.kt:1213)
	at kotlin.SynchronizedLazyImpl.getValue(LazyJVM.kt:74)
	at com.radixdlt.sargon.UniffiLib$Companion.getINSTANCE$sargon_android_debug(sargon.kt:1213)
	at com.radixdlt.sargon.SargonKt.newAccessControllerAddressSampleMainnet(sargon.kt:50684)
	at com.radixdlt.sargon.samples.AccessControllerAddressSampleMainnet.invoke(AccessControllerAddressSample.kt:16)
	at com.radixdlt.sargon.samples.AccessControllerAddressSampleMainnet.invoke(AccessControllerAddressSample.kt:12)
	at com.radixdlt.sargon.SampleTestable$DefaultImpls.testEquality(SampleTestable.kt:15)
	at com.radixdlt.sargon.AccessControllerAddressTest.testEquality(AccessControllerAddressTest.kt:15)
	at java.base/java.lang.reflect.Method.invoke(Method.java:568)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)

I've never had this issue before. I got it when working on a fairly large addition to our UniFFI layer in this PR

I've added some #[derive(uniffi::Object)] builders which contain some RwLock state.

I'm unable to understand why I got hit with this, if it is because the size of our whole Kotlin artifact or if it relates to implementation (disregarding other code).

The mention com.radixdlt.sargon.samples.AccessControllerAddressSampleMainnet mentioned packages in the stack trace is a red herring, those types have not changed! Which makes me thing that we get hit by this because of size of or artifact?

I'm surprised first of all that no one else have had this issue? Or at least if I search for it in issues here on GH there are no results.

Any help whatsoever what be greatly appreciated!

@CyonAlexRDX
Copy link

CyonAlexRDX commented Dec 4, 2024

I fixed the issue and then got it again, this small commit causes the exception:
radixdlt/sargon@7c1f671

It add one more [uniffi::export]ed method on SecurityShieldBuilder:

#[derive(Debug, uniffi::Object)]
pub struct SecurityShieldBuilder {
    wrapped: RwLock<sargon::SecurityShieldBuilder>,
}
impl SecurityShieldBuilder {
    fn set(
        &self,
        mut write: impl FnMut(
            &mut sargon::SecurityShieldBuilder,
        ) -> &sargon::SecurityShieldBuilder,
    ) {
        let mut binding = self.wrapped.write().expect("No poison");
        _ = write(&mut binding);
    }
}

#[uniffi::export]
impl SecurityShieldBuilder {
    pub fn set_name(&self, name: String) {
        self.set(|builder| builder.set_name(&name));
    }
   ...
}

And adding one more method - reset_recovery_and_confirmation_role_state leads to MethodTooLargeException. What the hell... 🙃

#[uniffi::export]
impl SecurityShieldBuilder {
    pub fn set_name(&self, name: String) {
        self.set(|builder| builder.set_name(&name));
    }
   ...
+  pub fn reset_recovery_and_confirmation_role_state(&self) { // <-- ❌ addition of this method => `MethodTooLargeException`
+       self.set(|builder| builder.reset_recovery_and_confirmation_role_state());
+    }
}

@CyonAlexRDX
Copy link

Update, we think we know the issue, we believe the issue to be that UniFFI generates a too big

@Suppress("UNUSED_PARAMETER")
private fun uniffiCheckApiChecksums(lib: UniffiLib) {

method. Ours is 3484 lines, 251kb, which seems to be too large for JDK?

I'm gonna try to manually split it and re-run our Kotlin JVM tests and see if it works.

@Sajjon
Copy link
Contributor Author

Sajjon commented Dec 4, 2024

(Sorry for switching between private and work Github handlers. Im switching between work computer and personal sometimes)

Confirmed that it is both the uniffiCheckApiChecksums and the interface UniffiLib: Library {} causing this.

I first started by splitting the huge uniffiCheckApiChecksums into "batches" - then retried running our JVM Kotlin tests on my Apple Silicon mac and it also failed.

Then I also commented out four methods in the interface UniffiLib and any reference to those four methods and tried again; It now worked!!

So I tried reverting the uniffiCheckApiChecksums split change and kept the change to interface UniffiLib which resulted in the same error!

Now ofc I do need those four commented out methods... so hmm so maybe I can "hack around" the limitation on number of methods on an interface? I'm gonna try:

interface UniffiLibPart1: Library {
 // half of all methods go here
}
interface UniffiLibPart2: UniffiLibPart1 {
// second half of all methods go here
}
interface UniffiLib: UniffiLibPart2 {}

@Sajjon
Copy link
Contributor Author

Sajjon commented Dec 4, 2024

@bendk @mhammond or do you have any other suggestions ? Any contributor here in UniFFI lib who's great at JDK/Kotlin stuff?

(I can barely write hello world in Kotlin)

@Sajjon
Copy link
Contributor Author

Sajjon commented Dec 4, 2024

I shall mentioned that we are still on UniFFI 0.27.1:

uniffi = { git = "https://github.com/mozilla/uniffi-rs/", rev = "6f33088e8100a2ea9586c8c3ecf98ab51d5aba62", features = [
  "cli",
] }

I might try to bump to latest and see if anything has changed.

@mhammond
Copy link
Member

mhammond commented Dec 4, 2024

I guess that makes sense to me and I don't have any other ideas for solving it. It seems fairly lame of JDK though :)

I doubt this has changed since 0.27

@Sajjon
Copy link
Contributor Author

Sajjon commented Dec 4, 2024

I think it is unlikely that Sargon is the biggest UniFFI using project in the world? Sure it is a pretty big project - but surely some Mozilla projects must be bigger.

So I guess Sargon is the biggest UniFFI using crate, and that is why we are first with hitting this JDK issue - and Mozilla (or any other) is not hit (yet) since Mozilla uses multi-crate setup?

I might try yet again the multi-crate setup now that we have @bendk s uniffi-bindgen swift change.

But I would like to understand if I can try some other JDK version to see if it works? Or hmm, Im really out of my depth here with this JDK stuff... I guess I need to know if UniFFI imposes some JDK version? Or if I can try upgrading locally on my machine and UniFFI will use/respect that? Or is that up to cargo ndk?? You see! Im completely lost. @mhammond can you perhaps ask internally at Mozilla if anyone can help me with that question?

Because OpenJDK fixed this semi-recently (1 year ago, which maybe counts as recently in the Java world)

openjdk/jdk#14408

@mhammond
Copy link
Member

mhammond commented Dec 4, 2024

So I guess Sargon is the biggest UniFFI using crate, and that is why we are first with hitting this JDK issue - and Mozilla (or any other) is not hit (yet) since Mozilla uses multi-crate setup?

Exactly - we have many small(-ish) crates.

Re the jdk version, uniffi doesn't have any opinion there. I'm not sure what "cargo ndk" is, but we don't use it IIUC. Updating to a new jdk should be fine - it looks like we are currently on jdk 17 (which seems early, so I'm not 100% sure what we actually release with, but that's what I seem to have locally)

@CyonAlexRDX
Copy link

CyonAlexRDX commented Dec 5, 2024

@mhammond Good news, I've fixed the issue - by manually edit the huge (generated) sargon.kt file, and this means that I can do a Pull Request with my fix, ensuring that UniFFI generates the Kotlin code in the same manner as my manual edits.

So I will soon create a PR, let me know if you want me to gate it behind a UniFFI build flag experimental_split_large_kotlin_libraries or similar!

Here is what I did to get it to work - let me know if anyone has any better idea:

I've split the interface UniffiLib: Library into two interfaces:

internal interface UniffiLib : Library {}
internal interface UniffiLibBatch2: Library { ... }

I've moved out some methods that I know to be called from our JVM tests from UniffiLib into UniffiLibBatch2 and I've updated the body all those global functions,

This is the complete change:

+ internal interface UniffiLibBatch2: Library {
+     companion object {
+         internal val INSTANCE: UniffiLibBatch2 by lazy {
+             loadIndirect<UniffiLibBatch2>(componentName = "sargon")
+                 .also { lib: UniffiLibBatch2 ->
+                     uniffiCheckContractApiVersionBatch2(lib)
+                     uniffiCheckApiChecksumsBatch2(lib)
+                 }
+         }
+ 
+         // The Cleaner for the whole library
+         internal val CLEANER: UniffiCleaner by lazy {
+             UniffiCleaner.create()
+         }
+     }
+ 
+     fun uniffi_sargon_uniffi_fn_func_new_foobar1(uniffi_out_err: UniffiRustCallStatus): RustBuffer.ByValue
+     fun uniffi_sargon_uniffi_fn_func_new_foobar2(uniffi_out_err: UniffiRustCallStatus): RustBuffer.ByValue
+ 
+     fun uniffi_sargon_uniffi_checksum_func_new_foobar1(): Short
+     fun uniffi_sargon_uniffi_checksum_func_new_foobar2(): Short
+ 
+ }
+
+@Suppress("UNUSED_PARAMETER")
+private fun uniffiCheckApiChecksumsBatch2(lib: UniffiLibBatch2) {
+    if (lib.uniffi_sargon_uniffi_checksum_func_new_foobar1() != 21850.toShort()) {
+        throw RuntimeException("UniFFI API checksum mismatch: try cleaning and rebuilding your project")
+    }
+    if (lib.uniffi_sargon_uniffi_fn_func_new_foobar2() != 33734.toShort()) {
+        throw RuntimeException("UniFFI API checksum mismatch: try cleaning and rebuilding your project")
+    }
+}

@Suppress("UNUSED_PARAMETER")
private fun uniffiCheckApiChecksums(lib: UniffiLib) {
    if (lib.uniffi_sargon_uniffi_checksum_func_frobnicate0001() != 16739.toShort()) {
        throw RuntimeException("UniFFI API checksum mismatch: try cleaning and rebuilding your project")
    }
    /* Many THOUSANDS of lines omitted */
-    if (lib.uniffi_sargon_uniffi_checksum_func_new_foobar1() != 21850.toShort()) {
-        throw RuntimeException("UniFFI API checksum mismatch: try cleaning and rebuilding your project")
-    }
-    if (lib.uniffi_sargon_uniffi_fn_func_new_foobar2() != 33734.toShort()) {
-        throw RuntimeException("UniFFI API checksum mismatch: try cleaning and rebuilding your project")
-    }
    if (lib.uniffi_sargon_uniffi_checksum_func_frobnicate1337() != 16739.toShort()) {
        throw RuntimeException("UniFFI API checksum mismatch: try cleaning and rebuilding your project")
    }
}

internal interface UniffiLib: Library {
    companion object {
        /* UNCHANGED */
    }

    fun uniffi_sargon_uniffi_fn_func_frobnicate0001(uniffi_out_err: UniffiRustCallStatus): RustBuffer.ByValue
    /* Thousands of lines omitted */
    fun uniffi_sargon_uniffi_fn_func_frobnicate1337(uniffi_out_err: UniffiRustCallStatus): RustBuffer.ByValue

-    fun uniffi_sargon_uniffi_fn_func_new_foobar1(uniffi_out_err: UniffiRustCallStatus): RustBuffer.ByValue
-    fun uniffi_sargon_uniffi_fn_func_new_foobar2(uniffi_out_err: UniffiRustCallStatus): RustBuffer.ByValue

    fun uniffi_sargon_uniffi_checksum_func_frobnicate0001(): Short
    /* Thousands of lines omitted */
    fun uniffi_sargon_uniffi_checksum_func_frobnicate1337(): Short

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

}

/* Thousands of lines omitted */

fun `newFoobar1`(): Foobar {
    return FfiConverterTypeFoobar.lift(
        uniffiRustCall { _status ->
-            UniffiLib.INSTANCE.uniffi_sargon_uniffi_fn_func_new_foobar1(_status)
+            UniffiLibBatch2.INSTANCE.uniffi_sargon_uniffi_fn_func_new_foobar1(_status)
        },
    )
}


fun `newFoobar2`(): Foobar {
    return FfiConverterTypeFoobar.lift(
        uniffiRustCall { _status ->
-            UniffiLib.INSTANCE.uniffi_sargon_uniffi_fn_func_new_foobar2(_status)
+            UniffiLibBatch2.INSTANCE.uniffi_sargon_uniffi_fn_func_new_foobar2(_status)
        },
    )
}

So it is a matter of coming up with good thresholds for when the interface UniffiLib: Library becomes too large and one has to split. It is probably easiest to express in terms of number of methods.

@mhammond
Copy link
Member

mhammond commented Dec 5, 2024

So I will soon create a PR, let me know if you want me to gate it behind a UniFFI build flag experimental_split_large_kotlin_libraries or similar!

I suspect the devil will be in the detail, but if the PR makes uniffi work where it otherwise would not, then there would be no need for a feature flag.

It is probably easiest to express in terms of number of methods.

eg, this is where the devil will live :) I suspect that the number of methods might indicate that "maybe there's a problem around here, although some objects with many fewer might trigger it, and some objects with many more might be fine" - which probably wouldn't be an actual fix.

Maybe the other solution is "don't do that - just use multiple crates"?

Sajjon added a commit to Sajjon/uniffi-rs that referenced this issue Dec 6, 2024
…hecksum fn from UniffiLib interface into separate one.
Sajjon added a commit to Sajjon/uniffi-rs that referenced this issue Dec 6, 2024
…hecksum fn from UniffiLib interface into separate one.
Sajjon added a commit to Sajjon/uniffi-rs that referenced this issue Dec 6, 2024
…hecksum fn from UniffiLib interface into separate one.
Sajjon added a commit to Sajjon/uniffi-rs that referenced this issue Dec 6, 2024
…_fn_into_seperate_interface

Fix JNA bug `MethodTooLargeException` mozilla#2340 by splitting out checksum…
@Sajjon
Copy link
Contributor Author

Sajjon commented Dec 6, 2024

Fixed in #2344

Sajjon added a commit to Sajjon/uniffi-rs that referenced this issue Dec 10, 2024
…hecksum fn from UniffiLib interface into separate one.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants