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

Memory leak on HeavyCallMeFromJava Unit Test #3

Open
iansmirlis opened this issue Oct 30, 2023 · 9 comments
Open

Memory leak on HeavyCallMeFromJava Unit Test #3

iansmirlis opened this issue Oct 30, 2023 · 9 comments
Assignees
Labels
bug Something isn't working runtime

Comments

@iansmirlis
Copy link
Owner

iansmirlis commented Oct 30, 2023

[Test]
[Explicit]
public void HeavyCallMeFromJava()
{
var start = DateTime.Now;
Class testClass = env.FindClass("net/sf/jni4net/tested/JavaToClrReflection");
Object test = testClass.newInstance();
MethodId methodId = env.GetStaticMethodID(test.getClass(), "reflect", "()V");
for (int i = 0; i < 100000; i++)
{
env.CallVoidMethod(test,methodId);
}
var end = DateTime.Now;
Console.WriteLine(end - start);
}

The call loop creates a memory leak in the java vm heap. It seems like JniLocalHandles are not get released by JNI but I will profile this to verify.

Originally posted by @iansmirlis in #1 (comment)

@iansmirlis iansmirlis self-assigned this Oct 30, 2023
@iansmirlis iansmirlis added bug Something isn't working runtime labels Oct 30, 2023
@iansmirlis iansmirlis mentioned this issue Oct 30, 2023
@iansmirlis
Copy link
Owner Author

iansmirlis commented Oct 31, 2023

//JavaToClrReflection test code
Bridge.init();

Type type = system.DateTime.typeof();
for(int i = 0; i < 10000; i++) {
    MethodInfo[] methodInfos = type.GetMethods();
}

@pavelsavara this is what I've got so far
I am not so familiar with java, but if I get this correctly MethodInfo objects are referenced by finalizers, but finalizers just pile up. They aren't referenced by anything else apart from the finalizer. Does this mean that they are in queue to be finalized but they never get to that point?

Unfortunately I haven't managed yet to check native java scenario yet, because of #10

Screenshot_2023-11-01_01-26-25

Screenshot_2023-11-01_01-31-23

core.hprof.gz

@pavelsavara
Copy link

It seems finalize() is deprecated now and we are supposed to use PhantomReference instead

@iansmirlis
Copy link
Owner Author

iansmirlis commented Nov 1, 2023

Thank you.

I created a PhantomReference mechanism inside Object and Exception classes in java and a background thread to run the DisposeClrHandle() cleanup code. This seems to (almost) fix the memory leak on the java side, however uncovered the fact the reference is hold by CLR and waits JNI to release it.

Because of this the DisposeClrHandle() does not run, so there is memory leak in the CLR side too, since at the end of the day no-one calls IntHandle.Free() for the JVMProxy

It begins to look like cyclic reference between CLR and JVM in this scenario. I will try to understand better these proxy dependencies and create a pull request with a fix at some point, however I think that this mess shouldn't happen in normal circumstances.

@pavelsavara
Copy link

however I think that this mess shouldn't happen in normal circumstances.

I'm not so sure. Our best outcome would be to at least have API which allows to dispose things manually (or break cycle).

Usual problematic pattern is to register some UI event callback an forget about it, never un-register.
Also I think any cross-boundary implementation of a interface is prone to cycles.
Add collections and you have a problem.

@iansmirlis
Copy link
Owner Author

iansmirlis commented Nov 2, 2023

Yes something like .forceDispose() in the base object to release the proxy on the other side seems nice. Is this what you had in mind?

Now for this particular issue I faced, I reached a point now that although Object gets garbage collected, the GC does not add the reference to the ReferenceQueue<> thus I loose track of it to call the cleanupcode. I thought that maybe this is some peculiarity since the Object was created through JNI

Today I will also try the following which is supported by Java 9 and later

public class Object implements IClrProxy, system.IObject {
    
    private static final Cleaner cleaner = Cleaner.create();
    private long clrHandle;

    protected Object(net.sf.jni4net.inj.INJEnv env, long handle) {
	clrHandle = handle;
	cleaner.register( this, cleanResourceAction(clrHandle));
    }

    public void initProxy(long handle) {
	    clrHandle = handle;
    }

    private static Runnable cleanResourceAction(long clrHandle) {
        return new Runnable() {
            @Override
            public void run() {
                if (clrHandle != 0 && net.sf.jni4net.Bridge.isRegistered()) {
                    net.sf.jni4net.Bridge.DisposeClrHandle(clrHandle);
                }
            }
        };
    }

......
}

@pavelsavara
Copy link

possibly IObject could extend IDisposable. We would need to start guarding if the proxy was already disposed on each marshaled call. (also when the proxy is just parameter)
Java side would benefit from mirror of that.

@iansmirlis
Copy link
Owner Author

iansmirlis commented Nov 2, 2023

Maybe it's just me and my weirdness :), but I wouldn't extend IDisposable, because this would imply that the Object should get disposed otherwise it will leak resources.

In such case I would use some other more explicit method for enforcing dispose. But I do not insist, it's just semiology.

Now with this memory leak, it's still a headache, I have been a few hours on this. But I will figure this out.

I used the Cleaner and JVM GC behaves like it refuses to collect the cleaner phantom references to Object for some obscure reason. Now they pile up like the finalizers did before. My first thought it that they are kept by the JNI mechanism, this is why I brought up the cyclic reference issue. But I am not sure yet.

At least now, the cleanup code runs and Objects are released from the CLR side.

@pavelsavara
Copy link

But I do not insist, it's just semiology.

I don't insist either. Feel free to make it separate interface.

Now they pile up like the finalizers did before.

I think that JVM spec has no guarantees on when finalizers would be run, it at all. So maybe that's that.
And maybe the JVM GC doesn't understand what kind of memory pressure is created by those external resources.
Is there a way how to force the cleanup ?

Also there are few types of GC that JVM implements. And they are configurable.
https://backstage.forgerock.com/knowledge/kb/article/a75965340#basic

I think we should aim at something which works everywhere. I'm sharing this because it could help you to debug why GC did or didn't run.

https://www.baeldung.com/java-verbose-gc
https://sematext.com/blog/java-garbage-collection-logs/

At least now, the cleanup code runs and Objects are released from the CLR side.

🎉

@iansmirlis
Copy link
Owner Author

iansmirlis commented Nov 4, 2023

The crazy thing is that the Cleaner code runs, which according to documentation this suggests that the garbage collector has indeed run, found that there are no more live references to that object, and it is eligible for garbage collection.

I double checked that there are no other references to Object apart from the cleaner phantom references. Noone touches the Object during cleaning.

I even created a second thread to monitor the clrHandles and call CLR side cleaning from there to make sure that Object gets cleaned without touching JNI directly.

I will revisit the issue after closing #10 to study how this behaves when the process starts from Java, to make sure that it's not some peculiarity due to back and forth between runtimes through JNI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working runtime
Projects
None yet
Development

No branches or pull requests

2 participants