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

Feature: support deleting GlobalRef from Env.classCache to avoid memory leak #83

Merged

Conversation

jairobjunior
Copy link
Contributor

@jairobjunior jairobjunior commented Apr 2, 2024

✨ Types of changes

  • Enhancement
  • New Feature

💡 Motivation and Context

My application was recently experiencing OOM after running for a couple days. The architecture of the app is Java running on top, calling into JNI using jnigi then calling into Go. Java can make many calls to the JNI layer with different types of parameters, including String.

This was a very interesting/hard leak to find, because the memory stats from the Main app running in Java didn't indicate any abnormal growth, but the RSS values definitely had a very linear curve. So we started isolating the components and verified that the leak was coming from the JNI. I then wrote a stress test app and used jemalloc to trace the memory on the stack. jemalloc gave me this chart:

leak

That showed me that there is a large % of memory used by jni_NewGlobalRef that wasn't getting freed. For some reason the map symbols were not working on the memory boxes above the global ref. I then started tracing where jnigi calls jni_NewGlobalRef and that was how I got to the problem.

🐛 Problem

Here is how my JNI layer looks with the leak:

//export Java_com_example_process
func Java_com_example_process(jniEnv unsafe.Pointer, clazz uintptr, jname uintptr) {
	env := jnigi.WrapEnv(jniEnv)
	strObj := jnigi.WrapJObject(jname, "java/lang/String", false)
	defer env.DeleteLocalRef(strObj)
	var goString []byte
	err := strObj.CallMethod(env, "getBytes", &goString)
	if err != nil {
		return
	}
	//Call into go with goString
}

That looked sort of harmless in the beginning, until we had to run the app for days.

The issue lives inside of the env.classCache. When env gets out of scope, all of the env.classCache will also get destroyed. The catch is that those objects inside of env.classCache were created using NewGlobalRef by callFindClass and are not deleted when env gets out of scope, creating dangling references.

🎯 Solution

This PR is introducing a new method called env.DeleteGlobalRefCache(), this method should be called after you want to dispose an instance of *jnigi.Env. It'll call deleteGlobalRef in all of the classCache and clear the map as well.

//export Java_com_example_process
func Java_com_example_process(jniEnv unsafe.Pointer, clazz uintptr, jname uintptr) {
	env := jnigi.WrapEnv(jniEnv)
        defer env.DeleteGlobalRefCache()
	strObj := jnigi.WrapJObject(jname, "java/lang/String", false)
	defer env.DeleteLocalRef(strObj)
	var goString []byte
	err := strObj.CallMethod(env, "getBytes", &goString)
	if err != nil {
		return
	}
	//Call into go with goString
}

@timob
Copy link
Owner

timob commented Apr 2, 2024

I ran into the same problem just recently too. Looks good. Thanks for all the details, writing the tests and comment doc! 🎉

Currently we do delete the cache in func (j *JVM) DetachCurrentThread(env *Env) error here. We should do this in the more general way using the method you provided, replacing the loop.

@jairobjunior jairobjunior force-pushed the add-delete-global-ref-cache-method branch from f6927f6 to b5c478e Compare April 2, 2024 15:42
@jairobjunior jairobjunior force-pushed the add-delete-global-ref-cache-method branch from b5c478e to 29e974f Compare April 2, 2024 15:44
@jairobjunior
Copy link
Contributor Author

@timob Sounds good. I just replaced the loop you mentioned with a call to the method env.DeleteGlobalRefCache().

@timob timob merged commit 2b02f0a into timob:master Apr 3, 2024
1 check passed
@timob
Copy link
Owner

timob commented Apr 3, 2024

Thanks!

@jairobjunior jairobjunior deleted the add-delete-global-ref-cache-method branch April 3, 2024 15:41
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.

2 participants