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

Improve performance of get_string #530

Closed

Conversation

olivergillespie
Copy link

@olivergillespie olivergillespie commented Mar 25, 2024

Overview

Speeds up get_string in two ways:

  1. Cache the java/lang/String class instead of calling FindClass repeatedly.
  2. Use IsInstanceOf instead of GetObjectClass + IsAssignableFrom. java/lang/String is final, so these are equivalent.

Running the benchmark on my machine, change 1:

jni_get_string          time:   [445.86 ns 448.89 ns 454.23 ns]
                        change: [-53.617% -45.104% -39.388%] (p = 0.00 < 0.05)
                        Performance has improved.

Changes 1+2:

jni_get_string          time:   [406.30 ns 406.37 ns 406.45 ns]
                        change: [-9.6565% -9.1695% -8.8758%] (p = 0.00 < 0.05)
                        Performance has improved.

Fixed #530. Also incidentally fixes #528.

Questions/notes

  • I don't write Rust, so sorry if I made any basic errors.
  • I tried to use std::OnceLock, but get_or_try_init is not yet stable, and I couldn't find a neat way to handle errors, hence pulling in once_cell. Open to ideas here.
  • Let me know your thoughts on the changelog and doc changes.

Thanks!

Definition of Done

  • There are no TODOs left in the code
  • Change is covered by automated tests
  • The coding guidelines are followed
  • Public API has documentation
  • User-visible changes are mentioned in the Changelog
  • The continuous integration build passes
    • TBD

Speeds up `get_string` in two ways:

1. Cache the `java/lang/String` class instead of calling `FindClass`
   repeatedly.
2. Use `IsInstanceOf` instead of `GetObjectClass` + `IsAssignableFrom`.
   `java/lang/String` is final, so these are equivalent.

Running the benchmark on my machine, change 1:

```
jni_get_string          time:   [445.86 ns 448.89 ns 454.23 ns]
                        change: [-53.617% -45.104% -39.388%] (p = 0.00 < 0.05)
                        Performance has improved.
```

Changes 1+2:
```
jni_get_string          time:   [406.30 ns 406.37 ns 406.45 ns]
                        change: [-9.6565% -9.1695% -8.8758%] (p = 0.00 < 0.05)
                        Performance has improved.
```

Also incidentally fixes jni-rs#528.
@olivergillespie olivergillespie deleted the get_string_perf branch March 25, 2024 15:43
olivergillespie added a commit to olivergillespie/jni-rs that referenced this pull request Mar 25, 2024
Speeds up `get_string` in two ways:

1. Cache the `java/lang/String` class instead of calling `FindClass`
   repeatedly.
2. Use `IsInstanceOf` instead of `GetObjectClass` + `IsAssignableFrom`.
   `java/lang/String` is final, so these are equivalent.

Running the benchmark on my machine, change 1:

```
jni_get_string          time:   [445.86 ns 448.89 ns 454.23 ns]
                        change: [-53.617% -45.104% -39.388%] (p = 0.00 < 0.05)
                        Performance has improved.
```

Changes 1+2:
```
jni_get_string          time:   [406.30 ns 406.37 ns 406.45 ns]
                        change: [-9.6565% -9.1695% -8.8758%] (p = 0.00 < 0.05)
                        Performance has improved.
```

Fixes jni-rs#530. Also incidentally fixes jni-rs#528.
@olivergillespie
Copy link
Author

Accidentally closed by deleting the branch on my fork. Opened #531 instead.

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

Successfully merging this pull request may close these issues.

None yet

1 participant