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.

Sign up for GitHub

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

Fix JNA bug MethodTooLargeException by splitting big UniffiLib interface #2344

Merged
merged 2 commits into from
Dec 12, 2024
Merged

Fix JNA bug MethodTooLargeException by splitting big UniffiLib interface #2344

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
20fe81c
Fix JNA bug `MethodTooLargeException` #2340 by splitting out checksum…
Sajjon Dec 6, 2024
8f4ff17
Fix lint errors on CI in the case of `self.initialization_fns` being …
Sajjon Dec 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
92 changes: 76 additions & 16 deletions uniffi_bindgen/src/bindings/kotlin/templates/NamespaceLibraryTemplate.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,20 +55,81 @@ internal open class {{ ffi_struct.name()|ffi_struct_name }}(
{%- endmatch %}
{%- endfor %}


{%- macro decl_kotlin_functions(func_list) -%}
Sajjon marked this conversation as resolved.
Show resolved Hide resolved
{% for func in func_list -%}
fun {{ func.name() }}(
{%- call kt::arg_list_ffi_decl(func) %}
): {% match func.return_type() %}{% when Some with (return_type) %}{{ return_type.borrow()|ffi_type_name_by_value }}{% when None %}Unit{% endmatch %}
{% endfor %}
{%- endmacro %}

// For large crates we prevent `MethodTooLargeException` (see #2340)
Sajjon marked this conversation as resolved.
Show resolved Hide resolved
// N.B. the name of the extension is very misleading, since it is
// rather `InterfaceTooLargeException`, caused by too many methods
// in the interface for large crates.
//
// By splitting the otherwise huge interface into two parts
// * UniffiLib
// * IntegrityCheckingUniffiLib (this)
// we allow for ~2x as many methods in the UniffiLib interface.
//
// The `ffi_uniffi_contract_version` method and all checksum methods are put
// into `IntegrityCheckingUniffiLib` and these methods are called only once,
// when the library is loaded.
internal interface IntegrityCheckingUniffiLib : Library {
// Integrity check functions only
{# newline below wanted #}

{%- call decl_kotlin_functions(ci.iter_ffi_function_integrity_checks()) %}
}

// A JNA Library to expose the extern-C FFI definitions.
// This is an implementation detail which will be called internally by the public API.

internal interface UniffiLib : Library {
companion object {
internal val INSTANCE: UniffiLib by lazy {
loadIndirect<UniffiLib>(componentName = "{{ ci.namespace() }}")
.also { lib: UniffiLib ->
uniffiCheckContractApiVersion(lib)
uniffiCheckApiChecksums(lib)
{% for fn in self.initialization_fns() -%}
{{ fn }}(lib)
{% endfor -%}
}
val componentName = "{{ ci.namespace() }}"
// For large crates we prevent `MethodTooLargeException` (see #2340)
// N.B. the name of the extension is very misleading, since it is
// rather `InterfaceTooLargeException`, caused by too many methods
// in the interface for large crates.
//
// By splitting the otherwise huge interface into two parts
// * UniffiLib (this)
// * IntegrityCheckingUniffiLib
// And all checksum methods are put into `IntegrityCheckingUniffiLib`
// we allow for ~2x as many methods in the UniffiLib interface.
//
// Thus we first load the library with `loadIndirect` as `IntegrityCheckingUniffiLib`
// so that we can call `uniffiCheckApiChecksums`...
loadIndirect<IntegrityCheckingUniffiLib>(componentName)
.also { lib: IntegrityCheckingUniffiLib ->
uniffiCheckContractApiVersion(lib)
uniffiCheckApiChecksums(lib)
Copy link
Contributor Author

@Sajjon Sajjon Dec 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@Sajjon Sajjon Dec 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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".

Copy link
Contributor Author

@Sajjon Sajjon Dec 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

}
// ... and then we load the library as `UniffiLib`
// N.B. we cannot use `loadIndirect` once and then try to cast it to `UniffiLib`
// => results in `java.lang.ClassCastException: com.sun.proxy.$Proxy cannot be cast to ...`
// error. So we must call `loadIndirect` twice. For crates large enough
// to trigger this issue, the performance impact is negligible, running on
// a macOS M1 machine the `loadIndirect` call takes ~50ms.
loadIndirect<UniffiLib>(componentName)
{%- if !self.initialization_fns().is_empty() -%}
{#-
// We only include the `.also` block if there are initialization functions
// otherwise we get linting errors saying `lib` is unused.
-#}
.also { lib: UniffiLib ->
// No need to check the contract version and checksums, since
// we already did that with `IntegrityCheckingUniffiLib` above.
{% for fn in self.initialization_fns() -%}
{{ fn }}(lib)
{% endfor -%}
}
{%- endif -%}

// Loading of library with integrity check done.
}
{% if ci.contains_object_types() %}
// The Cleaner for the whole library
Expand All @@ -78,14 +139,13 @@ internal interface UniffiLib : Library {
{%- endif %}
}

{% for func in ci.iter_ffi_function_definitions() -%}
fun {{ func.name() }}(
{%- call kt::arg_list_ffi_decl(func) %}
): {% match func.return_type() %}{% when Some with (return_type) %}{{ return_type.borrow()|ffi_type_name_by_value }}{% when None %}Unit{% endmatch %}
{% endfor %}
// FFI functions
{# newline below before call decl_kotlin_functions is needed #}

{%- call decl_kotlin_functions(ci.iter_ffi_function_definitions_excluding_integrity_checks()) %}
}

private fun uniffiCheckContractApiVersion(lib: UniffiLib) {
private fun uniffiCheckContractApiVersion(lib: IntegrityCheckingUniffiLib) {
// Get the bindings contract version from our ComponentInterface
val bindings_contract_version = {{ ci.uniffi_contract_version() }}
// Get the scaffolding contract version by calling the into the dylib
Expand All @@ -96,7 +156,7 @@ private fun uniffiCheckContractApiVersion(lib: UniffiLib) {
}

@Suppress("UNUSED_PARAMETER")
private fun uniffiCheckApiChecksums(lib: UniffiLib) {
private fun uniffiCheckApiChecksums(lib: IntegrityCheckingUniffiLib) {
{%- for (name, expected_checksum) in ci.iter_checksums() %}
if (lib.{{ name }}() != {{ expected_checksum }}.toShort()) {
throw RuntimeException("UniFFI API checksum mismatch: try cleaning and rebuilding your project")
Expand Down
33 changes: 29 additions & 4 deletions uniffi_bindgen/src/interface/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -657,10 +657,36 @@ impl ComponentInterface {
/// The set of FFI functions is derived automatically from the set of higher-level types
/// along with the builtin FFI helper functions.
pub fn iter_ffi_function_definitions(&self) -> impl Iterator<Item = FfiFunction> + '_ {
self.iter_user_ffi_function_definitions()
self.iter_ffi_function_definitions_conditionally_include_integrity_checks(true)
}

pub fn iter_ffi_function_definitions_excluding_integrity_checks(
&self,
) -> impl Iterator<Item = FfiFunction> + '_ {
self.iter_ffi_function_definitions_conditionally_include_integrity_checks(false)
}

fn iter_ffi_function_definitions_conditionally_include_integrity_checks(
&self,
include_checksums: bool,
) -> impl Iterator<Item = FfiFunction> + '_ {
let iterator = self
.iter_user_ffi_function_definitions()
.cloned()
.chain(self.iter_rust_buffer_ffi_function_definitions())
.chain(self.iter_futures_ffi_function_definitions())
.chain(self.iter_futures_ffi_function_definitions());

// Conditionally determine if the checksums should be included or not.
if include_checksums {
Box::new(iterator.chain(self.iter_ffi_function_integrity_checks()))
as Box<dyn Iterator<Item = FfiFunction> + '_>
} else {
Box::new(iterator) as Box<dyn Iterator<Item = FfiFunction> + '_>
}
}

pub fn iter_ffi_function_integrity_checks(&self) -> impl Iterator<Item = FfiFunction> + '_ {
iter::empty()
.chain(self.iter_checksum_ffi_functions())
.chain([self.ffi_uniffi_contract_version()])
}
Expand All @@ -672,8 +698,7 @@ impl ComponentInterface {
self.iter_user_ffi_function_definitions()
.cloned()
.chain(self.iter_rust_buffer_ffi_function_definitions())
.chain(self.iter_checksum_ffi_functions())
.chain([self.ffi_uniffi_contract_version()])
.chain(self.iter_ffi_function_integrity_checks())
}

/// List all FFI functions definitions for user-defined interfaces
Expand Down