Skip to content

Commit d220d61

Browse files
wuwbobo2021wuwbobo2021
committed
Cache method/field ID; fix jclass memory leaks
1 parent ffc69b1 commit d220d61

File tree

5 files changed

+113
-51
lines changed

5 files changed

+113
-51
lines changed

‎java-spaghetti-gen/src/emit_rust/fields.rs

+33-13
Original file line numberDiff line numberDiff line change
@@ -107,7 +107,7 @@ impl<'a> Field<'a> {
107107
}
108108

109109
let env_param = if self.java.is_static() {
110-
"env: ::java_spaghetti::Env<'env>"
110+
"__jni_env: ::java_spaghetti::Env<'env>"
111111
} else {
112112
"self: &::java_spaghetti::Ref<'env, Self>"
113113
};
@@ -157,28 +157,37 @@ impl<'a> Field<'a> {
157157
out,
158158
"{indent}{attributes}pub fn {get}<'env>({env_param}) -> {rust_get_type} {{",
159159
)?;
160+
writeln!(
161+
out,
162+
"{indent} static __FIELD: ::std::sync::OnceLock<usize> = ::std::sync::OnceLock::new();"
163+
)?;
160164
writeln!(out, "{indent} unsafe {{")?;
161165
if !self.java.is_static() {
162-
writeln!(out, "{indent} let env = self.env();")?;
166+
writeln!(out, "{indent} let __jni_env = self.env();")?;
163167
}
164168
writeln!(
165169
out,
166-
"{} let (__jni_class, __jni_field) = env.require_class_{}field({}, {}, {});",
167-
indent,
170+
"{indent} let __jni_class = Self::__class_global_ref(__jni_env);"
171+
)?;
172+
writeln!(
173+
out,
174+
"{indent} \
175+
let __jni_field = *__FIELD.get_or_init(|| \
176+
__jni_env.require_{}field(__jni_class, {}, {}).addr()\
177+
) as ::java_spaghetti::sys::jfieldID;",
168178
if self.java.is_static() { "static_" } else { "" },
169-
StrEmitter(self.class.path().as_str()),
170179
StrEmitter(self.java.name()),
171180
StrEmitter(FieldSigWriter(self.java.descriptor()))
172181
)?;
173182
if self.java.is_static() {
174183
writeln!(
175184
out,
176-
"{indent} env.get_static_{field_fragment}_field(__jni_class, __jni_field)",
185+
"{indent} __jni_env.get_static_{field_fragment}_field(__jni_class, __jni_field)",
177186
)?;
178187
} else {
179188
writeln!(
180189
out,
181-
"{indent} env.get_{field_fragment}_field(self.as_raw(), __jni_field)",
190+
"{indent} __jni_env.get_{field_fragment}_field(self.as_raw(), __jni_field)",
182191
)?;
183192
}
184193
writeln!(out, "{indent} }}")?;
@@ -202,28 +211,39 @@ impl<'a> Field<'a> {
202211
out,
203212
"{indent}{attributes}pub fn {set}<{lifetimes}>({env_param}, value: {rust_set_type}) {{",
204213
)?;
214+
writeln!(
215+
out,
216+
"{indent} static __FIELD: ::std::sync::OnceLock<usize> = ::std::sync::OnceLock::new();"
217+
)?;
205218
writeln!(out, "{indent} unsafe {{")?;
206219
if !self.java.is_static() {
207-
writeln!(out, "{indent} let env = self.env();")?;
220+
writeln!(out, "{indent} let __jni_env = self.env();")?;
208221
}
209222
writeln!(
210223
out,
211-
"{} let (__jni_class, __jni_field) = env.require_class_{}field({}, {}, {});",
212-
indent,
224+
"{indent} let __jni_class = Self::__class_global_ref(__jni_env);"
225+
)?;
226+
writeln!(
227+
out,
228+
"{indent} \
229+
let __jni_field = *__FIELD.get_or_init(|| \
230+
__jni_env.require_{}field(__jni_class, {}, {}).addr()\
231+
) as ::java_spaghetti::sys::jfieldID;",
213232
if self.java.is_static() { "static_" } else { "" },
214-
StrEmitter(self.class.path().as_str()),
215233
StrEmitter(self.java.name()),
216234
StrEmitter(FieldSigWriter(self.java.descriptor()))
217235
)?;
218236
if self.java.is_static() {
219237
writeln!(
220238
out,
221-
"{indent} env.set_static_{field_fragment}_field(__jni_class, __jni_field, value)",
239+
"{indent} \
240+
__jni_env.set_static_{field_fragment}_field(__jni_class, __jni_field, value)",
222241
)?;
223242
} else {
224243
writeln!(
225244
out,
226-
"{indent} env.set_{field_fragment}_field(self.as_raw(), __jni_field, value)",
245+
"{indent} \
246+
__jni_env.set_{field_fragment}_field(self.as_raw(), __jni_field, value)",
227247
)?;
228248
}
229249
writeln!(out, "{indent} }}")?;

‎java-spaghetti-gen/src/emit_rust/methods.rs

+31-25
Original file line numberDiff line numberDiff line change
@@ -184,22 +184,17 @@ impl<'a> Method<'a> {
184184

185185
writeln!(out)?;
186186
for reason in &emit_reject_reasons {
187-
writeln!(out, "{}// Not emitting: {}", indent, reason)?;
187+
writeln!(out, "{indent}// Not emitting: {reason}")?;
188188
}
189189
if let Some(url) = KnownDocsUrl::from_method(context, self) {
190-
writeln!(out, "{}/// {}", indent, url)?;
190+
writeln!(out, "{indent}/// {url}")?;
191191
} else {
192-
writeln!(out, "{}/// {}", indent, self.java.name())?;
192+
writeln!(out, "{indent}/// {}", self.java.name())?;
193193
}
194194
writeln!(
195195
out,
196-
"{}{}{}fn {}<'env>({}) -> ::std::result::Result<{}, ::java_spaghetti::Local<'env, {}>> {{",
197-
indent,
198-
attributes,
199-
access,
200-
method_name,
201-
params_decl,
202-
ret_decl,
196+
"{indent}{attributes}{access}fn {method_name}<'env>({params_decl}) -> \
197+
::std::result::Result<{ret_decl}, ::java_spaghetti::Local<'env, {}>> {{",
203198
context.throwable_rust_path(mod_)
204199
)?;
205200
writeln!(
@@ -211,43 +206,54 @@ impl<'a> Method<'a> {
211206
self.java.name(),
212207
MethodSigWriter(self.java.descriptor())
213208
)?;
214-
writeln!(out, "{} unsafe {{", indent)?;
215-
writeln!(out, "{} let __jni_args = [{}];", indent, params_array)?;
209+
210+
writeln!(
211+
out,
212+
"{indent} static __METHOD: ::std::sync::OnceLock<usize> = ::std::sync::OnceLock::new();"
213+
)?;
214+
writeln!(out, "{indent} unsafe {{")?;
215+
writeln!(out, "{indent} let __jni_args = [{params_array}];")?;
216216
if !self.java.is_constructor() && !self.java.is_static() {
217-
writeln!(out, "{} let __jni_env = self.env();", indent)?;
217+
writeln!(out, "{indent} let __jni_env = self.env();")?;
218218
}
219-
220219
writeln!(
221220
out,
222-
"{} let (__jni_class, __jni_method) = __jni_env.require_class_{}method({}, {}, {});",
223-
indent,
221+
"{indent} let __jni_class = Self::__class_global_ref(__jni_env);"
222+
)?;
223+
writeln!(
224+
out,
225+
"{indent} \
226+
let __jni_method = *__METHOD.get_or_init(|| \
227+
__jni_env.require_{}method(__jni_class, {}, {}).addr()\
228+
) as ::java_spaghetti::sys::jmethodID;",
224229
if self.java.is_static() { "static_" } else { "" },
225-
StrEmitter(self.class.path().as_str()),
226230
StrEmitter(self.java.name()),
227231
StrEmitter(MethodSigWriter(self.java.descriptor()))
228232
)?;
229233

230234
if self.java.is_constructor() {
231235
writeln!(
232236
out,
233-
"{} __jni_env.new_object_a(__jni_class, __jni_method, __jni_args.as_ptr())",
234-
indent
237+
"{indent} \
238+
__jni_env.new_object_a(__jni_class, __jni_method, __jni_args.as_ptr())",
235239
)?;
236240
} else if self.java.is_static() {
237241
writeln!(
238242
out,
239-
"{} __jni_env.call_static_{}_method_a(__jni_class, __jni_method, __jni_args.as_ptr())",
240-
indent, ret_method_fragment
243+
"{indent} \
244+
__jni_env.call_static_{}_method_a(__jni_class, __jni_method, __jni_args.as_ptr())",
245+
ret_method_fragment
241246
)?;
242247
} else {
243248
writeln!(
244249
out,
245-
"{} __jni_env.call_{}_method_a(self.as_raw(), __jni_method, __jni_args.as_ptr())",
246-
indent, ret_method_fragment
250+
"{indent} \
251+
__jni_env.call_{}_method_a(self.as_raw(), __jni_method, __jni_args.as_ptr())",
252+
ret_method_fragment
247253
)?;
248254
}
249-
writeln!(out, "{} }}", indent)?;
250-
writeln!(out, "{}}}", indent)?;
255+
writeln!(out, "{indent} }}")?;
256+
writeln!(out, "{indent}}}")?;
251257
Ok(())
252258
}
253259
}

‎java-spaghetti-gen/src/emit_rust/structs.rs

+22-6
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ use std::io;
66
use super::fields::Field;
77
use super::known_docs_url::KnownDocsUrl;
88
use super::methods::Method;
9+
use super::StrEmitter;
910
use crate::emit_rust::Context;
1011
use crate::identifiers::{FieldMangling, RustIdentifier};
1112
use crate::parser_util::{Class, Id, IdPart};
@@ -122,12 +123,12 @@ impl Struct {
122123
}
123124
writeln!(
124125
out,
125-
"unsafe impl ::java_spaghetti::JniType for {rust_name} {{
126-
fn static_with_jni_type<R>(callback: impl FnOnce(&str) -> R) -> R {{
127-
callback({:?})
128-
}}
129-
}}",
130-
self.java.path().as_str().to_string() + "\0",
126+
"unsafe impl ::java_spaghetti::JniType for {rust_name} {{\
127+
\n fn static_with_jni_type<R>(callback: impl FnOnce(&str) -> R) -> R {{\
128+
\n callback({})\
129+
\n }}\
130+
\n}}",
131+
StrEmitter(self.java.path().as_str()),
131132
)?;
132133

133134
// recursively visit all superclasses and superinterfaces.
@@ -152,6 +153,21 @@ impl Struct {
152153

153154
writeln!(out, "impl {rust_name} {{")?;
154155

156+
writeln!(
157+
out,
158+
"\
159+
\nfn __class_global_ref(__jni_env: ::java_spaghetti::Env) -> ::java_spaghetti::sys::jobject {{\
160+
\n static __CLASS: ::std::sync::OnceLock<::java_spaghetti::Global<{}>> = ::std::sync::OnceLock::new();\
161+
\n __CLASS.get_or_init(|| unsafe {{\
162+
\n ::java_spaghetti::Local::from_raw(__jni_env, __jni_env.require_class({})).as_global()\
163+
\n }}).as_raw()\
164+
\n}}",
165+
context
166+
.java_to_rust_path(Id("java/lang/Object"), &self.rust.mod_)
167+
.unwrap(),
168+
StrEmitter(self.java.path().as_str()),
169+
)?;
170+
155171
let mut id_repeats = HashMap::new();
156172

157173
let mut methods: Vec<Method> = self

‎java-spaghetti/src/env.rs

+21-7
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ use std::marker::PhantomData;
22
use std::os::raw::c_char;
33
use std::ptr::{self, null_mut};
44
use std::sync::atomic::{AtomicPtr, Ordering};
5+
use std::sync::OnceLock;
56

67
use jni_sys::*;
78

@@ -147,9 +148,15 @@ impl<'env> Env<'env> {
147148
}
148149

149150
unsafe fn exception_to_string(self, exception: jobject) -> String {
150-
// use JNI FindClass to avoid infinte recursion.
151-
let throwable_class = self.require_class_jni("java/lang/Throwable\0");
152-
let throwable_get_message = self.require_method(throwable_class, "getMessage\0", "()Ljava/lang/String;\0");
151+
static METHOD_GET_MESSAGE: OnceLock<usize> = OnceLock::new();
152+
let throwable_get_message = *METHOD_GET_MESSAGE.get_or_init(|| {
153+
// use JNI FindClass to avoid infinte recursion.
154+
let throwable_class = self.require_class_jni("java/lang/Throwable\0");
155+
let method = self.require_method(throwable_class, "getMessage\0", "()Ljava/lang/String;\0");
156+
((**self.env).v1_2.DeleteLocalRef)(self.env, throwable_class);
157+
method.addr()
158+
}) as jmethodID; // it is a global ID
159+
153160
let message =
154161
((**self.env).v1_2.CallObjectMethodA)(self.env, exception, throwable_get_message, ptr::null_mut());
155162
let e2: *mut _jobject = ((**self.env).v1_2.ExceptionOccurred)(self.env);
@@ -161,6 +168,7 @@ impl<'env> Env<'env> {
161168
StringChars::from_env_jstring(self, message).to_string_lossy()
162169
}
163170

171+
/// Note: the returned `jclass` is actually a new local reference of the class object.
164172
pub unsafe fn require_class(self, class: &str) -> jclass {
165173
// First try with JNI FindClass.
166174
debug_assert!(class.ends_with('\0'));
@@ -183,10 +191,15 @@ impl<'env> Env<'env> {
183191
.collect::<Vec<_>>();
184192
let string = unsafe { self.new_string(chars.as_ptr(), chars.len() as jsize) };
185193

186-
// We still use JNI FindClass for this, to avoid a chicken-and-egg situation.
187-
// If the system class loader cannot find java.lang.ClassLoader, things are pretty broken!
188-
let cl_class = self.require_class_jni("java/lang/ClassLoader\0");
189-
let cl_method = self.require_method(cl_class, "loadClass\0", "(Ljava/lang/String;)Ljava/lang/Class;\0");
194+
static CL_METHOD: OnceLock<usize> = OnceLock::new();
195+
let cl_method = *CL_METHOD.get_or_init(|| {
196+
// We still use JNI FindClass for this, to avoid a chicken-and-egg situation.
197+
// If the system class loader cannot find java.lang.ClassLoader, things are pretty broken!
198+
let cl_class = self.require_class_jni("java/lang/ClassLoader\0");
199+
let cl_method = self.require_method(cl_class, "loadClass\0", "(Ljava/lang/String;)Ljava/lang/Class;\0");
200+
((**self.env).v1_2.DeleteLocalRef)(self.env, cl_class);
201+
cl_method.addr()
202+
}) as jmethodID; // it is a global ID
190203

191204
let args = [jvalue { l: string }];
192205
let result: *mut _jobject =
@@ -275,6 +288,7 @@ impl<'env> Env<'env> {
275288
}
276289

277290
// Multi-Query Methods
291+
// XXX: Remove these unused functions.
278292

279293
pub unsafe fn require_class_method(self, class: &str, method: &str, descriptor: &str) -> (jclass, jmethodID) {
280294
let class = self.require_class(class);

‎java-spaghetti/src/vm.rs

+6
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,12 @@ impl VM {
2929
self.0
3030
}
3131

32+
/// Constructs `VM` with a valid `jni_sys::JavaVM` raw pointer.
33+
///
34+
/// # Safety
35+
///
36+
/// - Make sure the corresponding JVM will keep alive within the lifetime of current native library or application.
37+
/// - Do not use any class redefinition feature, which may break the validity of method/field IDs to be cached.
3238
pub unsafe fn from_raw(vm: *mut JavaVM) -> Self {
3339
Self(vm)
3440
}

0 commit comments

Comments
 (0)