RFR: 8321288: [JVMCI] HotSpotJVMCIRuntime doesn't clean up WeakReferences in resolvedJavaTypes

[Doug Simon]

On Tue, 5 Dec 2023 19:00:51 GMT, Tom Rodriguez <never at openjdk.org> wrote:

> HotSpotJVMCIRuntime.resolvedJavaTypes implements a weak value map but is lacking code to clean out cleared weak references.  In normal mixed execution this isn't likely to get big and generally isolates are shutdown frequently so this doesn't lead to problems.  In Xcomp mode with tests that stress unloading this becomes more problematic.  In the worst case is still doesn't lead to large heaps but does make the idle heap larger than required.
> 
> This PR adds ReferenceQueue based cleaning of reclaimed values.  Testing in the context of a long running isolate shows that they are no longer accumulating.

src/jdk.internal.vm.ci/share/classes/jdk/vm/ci/hotspot/HotSpotJVMCIRuntime.java line 504:

> 502: 
> 503: 
> 504:     static class KlassWeakReference extends WeakReference<ResolvedJavaType> {

Can you please add javadoc to this class explaining why it's needed/useful.

src/jdk.internal.vm.ci/share/classes/jdk/vm/ci/hotspot/HotSpotJVMCIRuntime.java line 506:

> 504:     static class KlassWeakReference extends WeakReference<ResolvedJavaType> {
> 505: 
> 506:         private final Long klassPointer;

I assume this is `Long` instead of `long` to avoid boxing in `expungeStaleEntries`?

src/jdk.internal.vm.ci/share/classes/jdk/vm/ci/hotspot/HotSpotJVMCIRuntime.java line 703:

> 701:      * Clean up WeakReferences whose referents have been cleared.
> 702:      */
> 703:     private void expungeStaleEntries() {

`expungeStaleEntries` -> `expungeStaleKlassEntries` (or `expungeStaleKlasses`)

src/jdk.internal.vm.ci/share/classes/jdk/vm/ci/hotspot/HotSpotJVMCIRuntime.java line 706:

> 704:         KlassWeakReference current = (KlassWeakReference) resolvedJavaTypesQueue.poll();
> 705:         while (current != null) {
> 706:             // Make sure the entry is still mapped to the weak reference

The atomicity of this test-and-update relies on `fromMetaspace` being synchronized right? Maybe worth pointing this out in the comment.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/16981#discussion_r1418244577
PR Review Comment: https://git.openjdk.org/jdk/pull/16981#discussion_r1418250729
PR Review Comment: https://git.openjdk.org/jdk/pull/16981#discussion_r1418247243
PR Review Comment: https://git.openjdk.org/jdk/pull/16981#discussion_r1418248381