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

tkrodriguez · 2023-12-05T19:00:51Z

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.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8321288: [JVMCI] HotSpotJVMCIRuntime doesn't clean up WeakReferences in resolvedJavaTypes (Bug - P4)

Reviewers

Doug Simon (@dougxc - Reviewer)
Vladimir Kozlov (@vnkozlov - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/16981/head:pull/16981
$ git checkout pull/16981

Update a local copy of the PR:
$ git checkout pull/16981
$ git pull https://git.openjdk.org/jdk.git pull/16981/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 16981

View PR using the GUI difftool:
$ git pr show -t 16981

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/16981.diff

Webrev

Link to Webrev Comment

…in resolvedJavaTypes

bridgekeeper · 2023-12-05T19:02:38Z

👋 Welcome back never! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2023-12-05T19:04:16Z

@tkrodriguez The following labels will be automatically applied to this pull request:

graal
hotspot-compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

mlbridge · 2023-12-05T19:06:51Z

Webrevs

01: Full - Incremental (34c575a7)
00: Full (e6a60ed0)

dougxc · 2023-12-07T02:47:32Z

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


-    @NativeImageReinitialize private HashMap<Long, WeakReference<ResolvedJavaType>> resolvedJavaTypes;
+
+    static class KlassWeakReference extends WeakReference<ResolvedJavaType> {


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

dougxc · 2023-12-07T02:49:48Z

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

+    /**
+     * Clean up WeakReferences whose referents have been cleared.
+     */
+    private void expungeStaleEntries() {


expungeStaleEntries -> expungeStaleKlassEntries (or expungeStaleKlasses)

dougxc · 2023-12-07T02:52:11Z

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

+    private void expungeStaleEntries() {
+        KlassWeakReference current = (KlassWeakReference) resolvedJavaTypesQueue.poll();
+        while (current != null) {
+            // 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.

dougxc · 2023-12-07T02:56:57Z

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

+
+    static class KlassWeakReference extends WeakReference<ResolvedJavaType> {
+
+        private final Long klassPointer;


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

There's autoboxing in a more few places and I wanted to avoid having multiple copies of the box live so I promoted it to Long on entry to fromMetaspace.

tkrodriguez · 2023-12-11T21:58:33Z

I pushed some expanded comments and renames. I also adjusted some types that were weaker than the actual type which removed a bit of casting. Testing was clean.

openjdk · 2023-12-12T10:17:06Z

@tkrodriguez This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

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

Reviewed-by: dnsimon, kvn

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 49 new commits pushed to the master branch:

cf94854: 8321565: [REDO] Heap dump does not contain virtual Thread stack references
7ece9e9: 8321400: java/foreign/TestStubAllocFailure.java fails with code cache exhaustion
9320ef9: 8321973: Parallel: Remove unused methods in AdaptiveSizePolicy
2a565ff: 8321808: G1: Use unsigned type for non-negative G1 flags
493b5bd: 8293622: Cleanup use of G1ConcRefinementThreads
f573f6d: 8321515: ARM32: Move method resolution information out of the cpCache properly
8a0a6f8: 8321279: Implement hashCode() in Heap-X-Buffer.java.template
3d9d353: 8321825: Remove runtime/CompressedOops/CompressedClassPointers.java from the ProblemList
1b621f5: 8321474: TestAutoCreateSharedArchiveUpgrade.java should be updated with JDK 21
5463c9c: 8321557: Move SOURCE line verification for OracleJDK out of OpenJDK
... and 39 more: https://git.openjdk.org/jdk/compare/af5c49226c3416a9c3bdde06cac2076acf9de5c3...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

vnkozlov

Looks good as far as I can understand.

tkrodriguez · 2023-12-15T17:25:05Z

Thanks for the reviews.

/integrate

openjdk · 2023-12-15T17:25:25Z

Going to push as commit 05f7f0a.
Since your change was applied there have been 70 commits pushed to the master branch:

6311dab: 8322018: Test java/lang/String/CompactString/MaxSizeUTF16String.java fails with -Xcomp
bdebf19: 8322175: test/langtools/tools/javac/classreader/BadMethodParameter.java doesn't compile
20de541: 8322040: Missing array bounds check in ClassReader.parameter
b31454e: 8322098: os::Linux::print_system_memory_info enhance the THP output with /sys/kernel/mm/transparent_hugepage/hpage_pmd_size
0be0775: 8320397: RISC-V: Avoid passing t0 as temp register to MacroAssembler:: cmpxchg_obj_header/cmpxchgptr
6dfb812: 8321823: Remove redundant PhaseGVN transform_no_reclaim
a7dde57: 8322057: Memory leaks in creating jfr symbol array
692be57: 8322065: Initial nroff manpage generation for JDK 23
d02bc87: 8309981: Remove expired flags in JDK 23
8b24851: 8321480: ISO 4217 Amendment 176 Update
... and 60 more: https://git.openjdk.org/jdk/compare/af5c49226c3416a9c3bdde06cac2076acf9de5c3...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-12-15T17:25:35Z

@tkrodriguez Pushed as commit 05f7f0a.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

8321288: [JVMCI] HotSpotJVMCIRuntime doesn't clean up WeakReferences …

e6a60ed

…in resolvedJavaTypes

openjdk bot added the rfr Pull request is ready for review label Dec 5, 2023

openjdk bot added graal graal-dev@openjdk.org hotspot-compiler hotspot-compiler-dev@openjdk.org labels Dec 5, 2023

dougxc reviewed Dec 7, 2023

View reviewed changes

tkrodriguez added 2 commits December 10, 2023 20:35

Comment and types improvements

b0d6a14

Merge remote-tracking branch 'origin/master' into tkr-clean-weak

34c575a

dougxc approved these changes Dec 12, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Dec 12, 2023

vnkozlov approved these changes Dec 13, 2023

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Dec 15, 2023

openjdk bot closed this Dec 15, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Dec 15, 2023


		@NativeImageReinitialize private HashMap<Long, WeakReference<ResolvedJavaType>> resolvedJavaTypes;

		static class KlassWeakReference extends WeakReference<ResolvedJavaType> {


		static class KlassWeakReference extends WeakReference<ResolvedJavaType> {

		private final Long klassPointer;

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

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

Uh oh!

Conversation

tkrodriguez commented Dec 5, 2023 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented Dec 5, 2023

Uh oh!

openjdk bot commented Dec 5, 2023

Uh oh!

mlbridge bot commented Dec 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

dougxc Dec 7, 2023

Choose a reason for hiding this comment

Uh oh!

dougxc Dec 7, 2023

Choose a reason for hiding this comment

Uh oh!

dougxc Dec 7, 2023

Choose a reason for hiding this comment

Uh oh!

dougxc Dec 7, 2023

Choose a reason for hiding this comment

Uh oh!

tkrodriguez Dec 15, 2023

Choose a reason for hiding this comment

Uh oh!

tkrodriguez commented Dec 11, 2023

Uh oh!

openjdk bot commented Dec 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vnkozlov left a comment

Choose a reason for hiding this comment

Uh oh!

tkrodriguez commented Dec 15, 2023

Uh oh!

openjdk bot commented Dec 15, 2023

Uh oh!

openjdk bot commented Dec 15, 2023

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

tkrodriguez commented Dec 5, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Dec 5, 2023 •

edited

Loading

openjdk bot commented Dec 12, 2023 •

edited

Loading