8354897: Support Soft/Weak Reference in AOT cache #24757

iklam · 2025-04-18T18:31:05Z

This PR contains 2 parts

Upstream of Soft/Weak Reference support authored by @macarte from the Leyden repo
New C++ class AOTReferenceObjSupport and new Java method ReferencedKeyMap::prepareForAOTCache() developed by @iklam on the advice of @fisk from the GC team. These control the lifecycles of reference objects during the assembly phase to simplify the implementation.

One problem we faced in this PR is the handling of Reference objects that are waiting for clean up. Currently, the only cached Reference objects that require clean up are the WeakReferenceKeys used by ReferencedKeyMap (which is used by MethodType::internTable):

When the referent of a WeakReferenceKey K has been collected, the key will be placed on Universe::reference_pending_list(). It's linked to other pending references with the Reference::discovered field. At this point, K is still stored in the ReferencedKeyMap.
When heapShared.cpp discovered the ReferencedKeyMap, it will discover K, and it may also discover other pending references that are not intended for the AOT cache. As a result, we end up caching unnecessary objects.

ReferencedKeyMap::prepareForAOTCache() avoids the above problem. It goes over all entries in the table:

If an entry has not yet been collected, we make sure it will never be collected.
If an entry has been collected, we remove it from the table

Therefore, by the time heapShared.cpp starts scanning the ReferencedKeyMap, it will never see any keys that are on the pending list, so we will not see unintended objects.

This implementation is the very first step of Reference support in the AOT cache, so we chose a simplified approach that makes no assumptions on when the pending reference list is processed. This is sufficient for the current set of references objects in the AOT cache.

In the future, we may relax the implementation to allow for other use cases.

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-8354897: Support Soft/Weak Reference in AOT cache (Enhancement - P3)

Reviewers

Erik Österlund (@fisk - Reviewer) Review applies to 70216566
Chen Liang (@liach - Reviewer)

Contributors

Ioi Lam <[email protected]>

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 24757

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

Using diff file

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

Using Webrev

Link to Webrev Comment

…en repo)

bridgekeeper · 2025-04-18T18:31:54Z

👋 Welcome back iklam! 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 · 2025-04-18T18:32:46Z

@iklam 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:

8354897: Support Soft/Weak Reference in AOT cache

Co-authored-by: Ioi Lam <[email protected]>
Reviewed-by: liach, eosterlund

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 21 new commits pushed to the master branch:

50145bb: 8355773: Some nsk/jdi tests can fetch ThreadReference from static field in the debuggee
486acc0: 8355453: nsk.share.jdi.Debugee.waitingEvent() does not timeout properly
6612281: 8342886: Update MET timezone in TimeZoneNames files
... and 18 more: https://git.openjdk.org/jdk/compare/ead6757486fb58f35f6e93b691519673bca1a549...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.

openjdk · 2025-04-18T18:33:29Z

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

core-libs
hotspot

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.

iklam · 2025-04-18T20:34:40Z

/author @macarte
/author @iklam

openjdk · 2025-04-18T20:35:50Z

@iklam
Setting overriding author to Mat Carter <[email protected]>. When this pull request is integrated, the overriding author will be used in the commit.

openjdk · 2025-04-18T20:35:58Z

@iklam
Setting overriding author to Ioi Lam <[email protected]>. When this pull request is integrated, the overriding author will be used in the commit.

iklam · 2025-04-18T20:51:16Z

/author @macarte
/contributor add @iklam

openjdk · 2025-04-18T20:51:51Z

@iklam
Setting overriding author to Mat Carter <[email protected]>. When this pull request is integrated, the overriding author will be used in the commit.

openjdk · 2025-04-18T20:51:55Z

@iklam
Contributor Ioi Lam <[email protected]> successfully added.

@fisk

…ptions on GC behavior. Suggested by @fisk

mlbridge · 2025-04-24T16:10:09Z

Webrevs

liach · 2025-04-24T19:52:25Z

src/java.base/share/classes/jdk/internal/util/ReferencedKeyMap.java

+            Object referent = key.get();
+            if (referent == null) {
+                // We don't need this key anymore. Add to stale queue
+                ((Reference)key).enqueue();


Is enqueue necessary here? Afaik this map only uses the queue to be alert of member reference being garbage collected and then remove stale elements. Since at this stage this map is no longer used, maybe key.unused() is sufficient?

I'm not sure what you are proposing. But enqueue is used to ensure that inactive references are pruned from the data structure so that dumping only includes active references. This way, the JVM that loads these objects create these references as active and hence follow the usual life cycle that every other reference does. I want to avoid having a new "special" life cycles for dumped references.

The map is still being used (and will be stored into the AOT cache). The key no longer has a referent, so we need to remove the key from the map. Adding the key to the stale queue and calling removeStaleReferences() will accomplish this. key.unused() does not remove the key from the map.

Thanks. I am fine with the rest of the core library changes.

fisk · 2025-04-24T22:23:17Z

src/hotspot/share/cds/heapShared.cpp

@@ -1401,12 +1405,17 @@ class HeapShared::ReferentPusher: public BasicOopIterateClosure {
  template <class T> void do_oop_work(T *p) {
    oop obj = RawAccess<>::oop_load(p);
    if (!CompressedOops::is_null(obj)) {
-      size_t field_delta = pointer_delta(p, _referencing_obj, sizeof(char));
+      int field_offset = pointer_delta_as_int((char*)p, cast_from_oop<char*>(_referencing_obj));


The RawAccess oop load above should probably be a HeapAccess<ON_UNKNOWN_OOP_REF>::oop_load_at. By using the unknown oop ref and supplying the field offset, we allow GCs to infer the actual reference strength in the backend when loading and apply appropriate barriers for non-strong references, while also applying appropriate load barriers for conc GCs.

…/hotspot/jtreg/runtime/invokedynamic"

DanHeidinga · 2025-04-25T19:11:41Z

src/java.base/share/classes/jdk/internal/misc/CDS.java

    private static native int getCDSConfigStatus();
    private static native void logLambdaFormInvoker(String line);

+
+    private static ArrayList<Object> keepAliveList;


Is it worth adding a comment stating this list should only be populated during an assembly phase? That it should be null in both training and production runs?

I added comments and asserts for clarification.

DanHeidinga · 2025-04-25T19:17:51Z

src/hotspot/share/cds/aotReferenceObjSupport.cpp

+// Reference handling is complex. In this version, we implement only enough functionality to support
+// the use of Weak/Soft references used by java.lang.invoke.
+//
+// We intent to evolve the implementation in the future by


Suggested change

// We intent to evolve the implementation in the future by

// We intend to evolve the implementation in the future by

src/hotspot/share/cds/aotReferenceObjSupport.cpp

…DanHeidinga

…DanHeidinga comment -- renamed to MethodType::assemblySetup()

fisk

Other than reading the referent with the wrong accessor in the assert, I think this looks very good. Thanks for sorting this out in a clean and robust way.

liach

The core library changes look good.

…r AOTReferenceObjSupport::is_enabled()

zhengyu123 · 2025-04-30T00:28:40Z

src/hotspot/share/cds/aotReferenceObjSupport.cpp

+  InstanceKlass* ik = InstanceKlass::cast(k);
+  ik->initialize(CHECK);
+
+  TempNewSymbol field_name = SymbolTable::new_symbol("N""ULL");


Why "N""ULL", not "NULL"?

That's because a stand-lone word "NULL" will cause hotspot/jtreg/sources/TestNoNULL.java to fail. I have renamed the internal field to NULL_QUEUE to avoid the failure, per recommendation of @kimbarrett

Thank you for the explanation!

… failing hotspot/jtreg/sources/TestNoNULL.java

xmas92

lgtm. Just a few comments.

xmas92 · 2025-04-30T08:42:15Z

test/hotspot/jtreg/runtime/cds/appcds/aotClassLinking/WeakReferenceTest.java

+        if (ref2.get() == null) {
+            throw new RuntimeException("ref2.get() should not be null");
+        }


The referent of ref2 is never strong. If a GC occurs between makeRef2() and this check, we could hit this Exception. The test should either keep it strongly reachable until after this check or just not test wether it is reachable before the call to System.gc().

I removed the check that was before the GC.

xmas92 · 2025-04-30T08:45:54Z

src/hotspot/share/cds/aotReferenceObjSupport.cpp

+
+

Suggested change

xmas92 · 2025-04-30T08:47:59Z

src/hotspot/share/cds/aotArtifactFinder.cpp

@@ -73,6 +74,7 @@ void AOTArtifactFinder::find_artifacts() {
  // Note, if a class is not excluded, it does NOT mean it will be automatically included
  // into the AOT cache -- that will be decided by the code below.
  SystemDictionaryShared::finish_exclusion_checks();
+  AOTReferenceObjSupport::init_keep_alive_objs_table();


This should be guarded with INCLUDE_CDS_JAVA_HEAP somehow. I guess the cleanest way would be to use NOT_CDS_JAVA_HEAP_RETURN in aotReferenceObjSupport.hpp

I added NOT_CDS_JAVA_HEAP_RETURN to the declaration of init_keep_alive_objs_table. All the other functions are called only inside INCLUDE_CDS_JAVA_HEAP blocks, so I didn't change them to avoid cluttering the code.

AlanBateman · 2025-04-30T13:43:28Z

src/java.base/share/classes/java/lang/ref/Reference.java

+        runtimeSetup();
+    }
+
+    private static void runtimeSetup() {


I don't know if you are planning an annotation or something to mark these "runtimeSetup" methods but each one will minimally need a comment so that it's clear to anyone touching this code that it may be called by the VM when loading the AOT cache.

Yes, the plan is to use annotation to mark the runtimeSetup and assemblySetup methods.

I added a comment above the new runtimeSetup() method in Reference.java. For the other existing occurrences, I plan to address them in a separate RFE, probably JDK-8342481

kimbarrett · 2025-04-30T14:16:27Z

src/java.base/share/classes/jdk/internal/util/ReferencedKeyMap.java

+
+        for (ReferenceKey<K> key : map.keySet()) {
+            Object referent = key.get();
+            if (referent == null) {


Shouldn't this be key.refersTo(null)?

It is immediately passed to CDS.keepAlive, two separate calls of refersTo and get may produce null on the second call, I presume.

Sorry, I missed that referent was used later if non-null.

liach

Thanks for the comment addition requested by Alan.

iklam · 2025-04-30T18:04:17Z

Thanks everyone for the review. Passed tiers 1-4, as well as the ".*aot.jdkcache.*" tasks up to tier 10.
/integrate

openjdk · 2025-04-30T18:06:00Z

Going to push as commit 1ff7e81.
Since your change was applied there have been 23 commits pushed to the master branch:

9a2a2c5: 8354547: REDO: Force clients to explicitly pass mem_tag value, even if it is mtNone
18983b6: 8355391: Use Long::hashCode in java.time
50145bb: 8355773: Some nsk/jdi tests can fetch ThreadReference from static field in the debuggee
... and 20 more: https://git.openjdk.org/jdk/compare/ead6757486fb58f35f6e93b691519673bca1a549...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-04-30T18:06:10Z

@iklam Pushed as commit 1ff7e81.

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

ExE-Boss · 2025-05-01T02:01:36Z

src/java.base/share/classes/java/lang/ref/Reference.java

+    // Called from JVM when loading an AOT cache
    static {
+        runtimeSetup();
+    }
+
+    private static void runtimeSetup() {


The comment was added in the wrong spot:

Suggested change

// Called from JVM when loading an AOT cache

static {

runtimeSetup();

}

private static void runtimeSetup() {

static {

runtimeSetup();

}

// Called from JVM when loading an AOT cache

private static void runtimeSetup() {

Oops, I will fix in an related PR #24979

Mat Carter and others added 2 commits April 17, 2025 20:13

8354897: Support Soft/Weak Reference in AOT cache (imported from Leyd…

a23331a

…en repo)

Added code to process Universe::reference_pending_list()

c6beb85

openjdk bot added hotspot [email protected] core-libs [email protected] labels Apr 18, 2025

iklam added 4 commits April 23, 2025 21:48

Avoid the need to Universe::reference_pending_list() and remove assum…

2d82e36

…ptions on GC behavior. Suggested by @fisk

Added missing files

dc5ad26

cleaned up test cases

2544cc7

Merge branch 'master' into 8354897-soft-weak-references-in-aot-cache

6678482

iklam marked this pull request as ready for review April 24, 2025 16:05

openjdk bot added the rfr Pull request is ready for review label Apr 24, 2025

liach reviewed Apr 24, 2025

View reviewed changes

fisk suggested changes Apr 24, 2025

View reviewed changes

iklam added 5 commits April 24, 2025 18:08

Work around hotspot/jtreg/sources/TestNoNULL.java

1898399

Fixed error when running "make test JTREG=AOT_JDK=true TEST=open/test…

73786f2

…/hotspot/jtreg/runtime/invokedynamic"

Refactored code

2c31ca7

Fixed windows path issues

643eec2

@fisk comment

bba16ee

graalvmbot mentioned this pull request Apr 25, 2025

[JDK-8355586] Adapt JDK-8354897: Support Soft/Weak Reference in AOT cache oracle/graal#11088

Merged

DanHeidinga reviewed Apr 25, 2025

View reviewed changes

@DanHeidinga comments

a25aab0

iklam added 2 commits April 29, 2025 11:48

@fisk offline comments -- tighten up and simplify eligibility check; @…

331acfa

…DanHeidinga comment -- renamed to MethodType::assemblySetup()

Merge branch 'master' into 8354897-soft-weak-references-in-aot-cache

7021656

fisk approved these changes Apr 29, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Apr 29, 2025

liach approved these changes Apr 29, 2025

View reviewed changes

@fisk comment -- use proper HeapAccess to load referent; Also refacto…

01f0ea0

…r AOTReferenceObjSupport::is_enabled()

openjdk bot removed the ready Pull request is ready to be integrated label Apr 29, 2025

iklam mentioned this pull request Apr 29, 2025

8354890: AOT-initialize j.l.i.MethodHandleImpl and inner classes #24700

Closed

3 tasks

openjdk bot mentioned this pull request Apr 29, 2025

8354890: AOT-initialize j.l.i.MethodHandleImpl and inner classes #24956

Open

3 tasks

zhengyu123 reviewed Apr 30, 2025

View reviewed changes

Renamed the internal field ReferenceQueue.NULL to NULL_QUEUE to avoid…

4aa6874

… failing hotspot/jtreg/sources/TestNoNULL.java

xmas92 reviewed Apr 30, 2025

View reviewed changes

AlanBateman reviewed Apr 30, 2025

View reviewed changes

kimbarrett reviewed Apr 30, 2025

View reviewed changes

iklam added 2 commits April 30, 2025 10:37

@xmas92 comments

ec735cb

@AlanBateman comments

f8aff12

liach approved these changes Apr 30, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Apr 30, 2025

openjdk bot added the integrated Pull request has been integrated label Apr 30, 2025

openjdk bot closed this Apr 30, 2025

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Apr 30, 2025

ExE-Boss suggested changes May 1, 2025

View reviewed changes

	// We intent to evolve the implementation in the future by
	// We intend to evolve the implementation in the future by

8354897: Support Soft/Weak Reference in AOT cache #24757

8354897: Support Soft/Weak Reference in AOT cache #24757

Conversation

iklam commented Apr 18, 2025 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Contributors

Reviewing

bridgekeeper bot commented Apr 18, 2025

openjdk bot commented Apr 18, 2025 • edited Loading

openjdk bot commented Apr 18, 2025

iklam commented Apr 18, 2025

openjdk bot commented Apr 18, 2025

openjdk bot commented Apr 18, 2025

iklam commented Apr 18, 2025

openjdk bot commented Apr 18, 2025

openjdk bot commented Apr 18, 2025

mlbridge bot commented Apr 24, 2025 • edited Loading

Webrevs

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fisk left a comment

Choose a reason for hiding this comment

liach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xmas92 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liach left a comment

Choose a reason for hiding this comment

iklam commented Apr 30, 2025

openjdk bot commented Apr 30, 2025

openjdk bot commented Apr 30, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iklam commented Apr 18, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Apr 18, 2025 •

edited

Loading

mlbridge bot commented Apr 24, 2025 •

edited

Loading