Improve performance of get_string

[olivergillespie]

Overview

(Sorry for the false start in #530)

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 #529. Also incidentally fixes #527.

Add new dependency to changelog

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

[argv-minus-one]

This makes the assumption that the currently running JVM is the only one that will ever run in this process.

That assumption will usually hold true, but not always. It is possible to shut down the JVM (with JavaVM::destroy or equivalent) and then start a new JVM. It's also theoretically possible to run more than one JVM in a single process.

If that assumption is ever wrong, then calling this method will cause undefined behavior.

I realize that it is uncommon to do such a thing, but this change would make it outright impossible.

I suppose we could check if the JavaVM pointer's address has changed since the last time this method was called, but it's not impossible that shutting down the JVM and starting a new one would result in a JavaVM pointer with the exact same address.

It's too bad JNI doesn't give us any way to ask it to hold on to some per-JVM data for us. The native API for Node.js does have such a thing, for example. Even a “hey, JVM, call this callback for me when you shut down or unload me” would work—we could keep a global map of JavaVM pointers to GlobalRefs, and remove the entry for a JVM when it shuts down—but JNI doesn't have that, either.

Anyway, maybe this is an actual concern, maybe not. Just pointing it out.

[olivergillespie]

Hmm, interesting point, thanks.

Do you think we could make the lazy value a field of JNIEnv instead of static? That way it cannot be reused across VMs? I had some trouble with the #[repr(transparent)] requirement here, not sure the best way to implement the idea.
Or could we reset the value whenever a new JNIEnv is initialized? Not sure about the safety here, just an initial thought.

[olivergillespie]

I just saw in https://docs.oracle.com/en/java/javase/17/docs/specs/jni/invocation.html#jni_createjavavm:

Creation of multiple VMs in a single process is not supported.

So I don't think we need to worry about Create->Destroy->Create.

[argv-minus-one]

Storing fields in JNIEnv is not possible, because it is passed directly from the JVM upon calling a native method.

I had interpreted the JNI specification's statement as meaning it's not allowed to create multiple concurrent VMs. But, according to the output of this program, it looks like create→destroy→create isn't allowed, either.

# Cargo.toml

[package]
name = "multijvm"
edition = "2021"

[dependencies]
jni = { version = "0.21.1", features = ["invocation"] }

// main.rs

use jni::JavaVM;

fn new_jvm() -> JavaVM {
	JavaVM::new(
		jni::InitArgsBuilder::new()
		.version(jni::JNIVersion::V8)
		.build()
		.unwrap()
	).unwrap()
}

fn main() {
	let jvm = new_jvm();

	unsafe {
		jvm.destroy().unwrap();
	}

	let jvm = new_jvm();

	let mut env = jvm.attach_current_thread_permanently().unwrap();
	let system_out = env.get_static_field("java/lang/System", "out", "Ljava/io/PrintStream;").unwrap().l().unwrap();
	let message = env.new_string("Hello, world!").unwrap();
	env.call_method(&system_out, "println", "(Ljava/lang/String;)V", &[(&message).into()]).unwrap();
}

Output:

thread 'main' panicked at src/main.rs:9:7:
called `Result::unwrap()` on an `Err` value: Create(JniCall(Unknown))
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

If the jvm.destroy() and second new_jvm() calls are commented out, the program prints Hello, world! successfully.

So, if the JVM is storing global state, I guess we can do it too. 🤷‍♂️

[olivergillespie]

Thanks :). Yes, at least in hotspot it's explicitly denied:

If a previous creation attempt succeeded and we then
// destroyed that VM, we will be prevented from trying to recreate
// the VM in the same process, as the value will still be 0.

So do you think my approach can be approved? And are you a maintainer that can approve the workflow?

[argv-minus-one]

No, sorry. I'm only commenting. @rib is the maintainer.

Edit: I misunderstood your question, sorry! Yes, I've approved the workflow. It's not up to me to actually merge your PR, though.

[olivergillespie]

Looks like clippy is failing on a pre-existing issue in vm.rs, I can address it if desired.