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
Cleaner optimizaton #1536
base: master
Are you sure you want to change the base?
Cleaner optimizaton #1536
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, this indeed looks like a good improvement. Looking at the logic it seems to be an improvement and good stabilization.
This might need another look:
jna/src/com/sun/jna/internal/Cleaner.java
Lines 158 to 176 in 4a76b10
while ((now = System.currentTimeMillis()) < lastNonEmpty + MAX_LINGER_MS || !deleteIfEmpty()) { | |
if (!cleanerImpls.isEmpty()) { lastNonEmpty = now; } | |
try { | |
Reference<?> ref = impl.referenceQueue.remove(CLEANUP_INTERVAL_MS); | |
if(ref instanceof CleanerRef) { | |
((CleanerRef) ref).clean(); | |
} else { | |
masterCleanup(); | |
} | |
} catch (InterruptedException ex) { | |
// Can be raised on shutdown. If anyone else messes with | |
// our reference queue, well, there is no way to separate | |
// the two cases. | |
// https://groups.google.com/g/jna-users/c/j0fw96PlOpM/m/vbwNIb2pBQAJ | |
break; | |
} catch (Exception ex) { | |
Logger.getLogger(Cleaner.class.getName()).log(Level.SEVERE, null, ex); | |
} | |
} |
Consider this situation:
If new threads allocating native memory are periodically created and end with an interval that is smaller than CLEANUP_INTERVAL_MS
, every iteration of the while loop (158) will yield a CleanerRef
in 161 and masterCleanup()
will never be called. If now multiple threads are created (and are kept alive), that allocate native memory, the last allocation of these threads might not be collected, even if it should be gc
ed, because for these cases cleanup is only done from cleanQueue
, which is only called from masterCleanup
then.
The Callback Cleanup Test failes on windows (also locally reproducible, I tested with JDK8 both 32bit and 64bit):
Appveyor (64bit):
https://ci.appveyor.com/project/matthiasblaesing/jna/build/job/2x995gulpks9il6y?fullLog=true#L2061
https://ci.appveyor.com/project/matthiasblaesing/jna/build/job/2x995gulpks9il6y?fullLog=true#L2092
Appveyor (32bit):
https://ci.appveyor.com/project/matthiasblaesing/jna/build/job/9423p77olylb56ua?fullLog=true#L2063
https://ci.appveyor.com/project/matthiasblaesing/jna/build/job/9423p77olylb56ua?fullLog=true#L2094
Good catch, thanks. Should be fixed now.
I've found and fixed a possible cause. Will see if it works.
That's what I would expect if you ran the first commit only (test case to demonstrate #1535 ). Can you reproduce that with the latest commit? |
Windows is still a problem. The Linux side seems to cleanup nicely (github actions run clean). These are only the amd64 ones:
On my virtual machine I get: The first set of problems might be a to short test time, the latter a too small heap. |
6080be7
to
8512a00
Compare
Sorry for the delay. |
The situation is much improved. What I'm seeing though is, that the cleaner thread does not shut down. From my POV it can't. The static Could you please recheck this? |
Yes and no. The cleaner thread does not shut down as long as there are Cleaner instances alive. This is on purpose - the cleaner thread is required as long as the Cleaner instances could still be used for registering new cleanables. The idea is that in the "interesting" cases, the Cleaner instances will eventually be GC'd because either
If neither happens, the cleaner thread will (and needs to) stay alive. I admit there is one edge case that is not covered by this, i.e. if an application uses JNA only during startup and then never again. In that case, the cleaner thread will stay alive and consume some (few) resources. If that is a problem, it can be circumvented by executing the initial JNA calls in a separate thread that shuts down after JNA is no longer needed. Now that I've explained this, I'm not sure that the class unloading actually works. :-/ Will test. |
#1555 was created as reaction to #1521. It is not easy to get JNA unloaded. The pattern to use a static I disagree with the assessment that it is uncommon to call JNA methods from long running threads. For example can it be necessary to invoke functions from the EDT or using a thread pool. Firing up a separate thread to call into native is the opposite of a performance improvement and also might not be feasible at all. |
Fixes #1535 (also fixes #1521 )
This PR fixes the cleaner overload issue by creating a Cleaner instance per thread so that each thread will clean up the objects it previously created.
An additional "Master Cleaner" cleans up after thread exits. The Master Cleaner terminates itself 1 minute after all registered objects have been cleaned up.