JNI: Remove cleaned objects in memory cleaner #13378
Conversation
Signed-off-by: Chong Gao <[email protected]>
|
build |
res-life
added
3 - Ready for Review
| @@ -199,6 +200,12 @@ static void setDefaultGpu(int defaultGpuId) { | |||
| log.error("CAUGHT EXCEPTION WHILE TRYING TO CLEAN " + next, t); | |||
| } | |||
| all.remove(next); | |||
| } else { | |||
| long currentTime = System.currentTimeMillis(); | |||
| if (currentTime - lastRemoveCleanedObjectsTime > 10 * 1000L) { // every 10 seconds | |||
There was a problem hiding this comment.
We could future-proof it by adding a system property with the default the PR suggests so the value can be changed in situations 10s is not optimal one way or another
There was a problem hiding this comment.
Now proactively remove.
| } else { | ||
| long currentTime = System.currentTimeMillis(); | ||
| if (currentTime - lastRemoveCleanedObjectsTime > 10 * 1000L) { // every 10 seconds | ||
| all.removeIf(e -> e.cleaner.isClean()); |
There was a problem hiding this comment.
Why are we polling to clean objects when we can proactively remove things from all when they are properly closed? Saves from doing a full scan of potentially very many live objects in all and should be more efficient.
There's a couple of ways to do this. One is to implement equals and hashcode for CleanerWeakReference so we can remove our cleaner from all (by building a temporary instance to do the remove). Those methods would simply delegate to the cleaner instance, ignoring everything else. Not ideal, but it should allow us to build a helper instance to lookup the cleaner to remove from all.
More efficiently would be to rework the all and Cleaner setups so instead of CleanerWeakReference pointing to Cleaner, we remove CleanerWeakReference entirely and have Cleaner have the isRmmBlocker field and a normal WeakReference to the object it is associated with. all would simply be a hash set of the cleaner instances, and then it's trivial for a cleaner to remove itself from all when it cleans.
There was a problem hiding this comment.
Done.
Now proactively remove.
There was a problem hiding this comment.
Put the weak references into a Map, use cleaner id as the key of the Map. The id is unique and can represent a unique resource.
Signed-off-by: Chong Gao <[email protected]>
|
build |
|
The pipeline shows: Base branch is not under active development |
|
build |
|
build |
|
build |
|
/merge |
vyasr
added
4 - Needs Review
Description
closes #13412
Remove weak references of cleaned resources when a resource is cleaned.
The cleaned objects are never leaked, it's safe to remove the weak references.
This is to reduce the memory usage.
Checklist
No need.
No need.