RFR: 8354897: Support Soft/Weak Reference in AOT cache [v4]

[Dan Heidinga]

On Fri, 25 Apr 2025 05:56:15 GMT, Ioi Lam <iklam at openjdk.org> wrote:

>> This PR contains 2 parts
>> 
>> - Upstream of Soft/Weak Reference support authored by @macarte from [the Leyden repo](https://github.com/openjdk/leyden/commit/4ca75d156519596e23abc8a312496b7c2f0e0ca5)
>> - 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 `WeakReferenceKey`s  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.
>
> Ioi Lam has updated the pull request incrementally with one additional commit since the last revision:
> 
>   @fisk comment

src/hotspot/share/cds/aotReferenceObjSupport.cpp line 76:

> 74: // the use of Weak/Soft references used by java.lang.invoke.
> 75: //
> 76: // We intent to evolve the implementation in the future by

Suggestion:

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

src/hotspot/share/cds/aotReferenceObjSupport.cpp line 106:

> 104:     assert(CDSConfig::allow_only_single_java_thread(), "Required");
> 105: 
> 106:     TempNewSymbol method_name = SymbolTable::new_symbol("prepareForAOTCache");

I'm slightly uncomfortable with using the same method name (`prepareForAOTCache`) on MethodType and on ReferenceKeyMap & ReferenceKeySet as they have different expected use cases.  The one on MT is the "front door" the VM calls to remove stale Reference objects while the RKMap/RKSet are internal mechanisms that the VM would never call except for MT triggering it.

Does it make sense to use different names for these methods?  The MT one is a hook that could be extended to other classes to prepare them for cache creation while we wouldn't treat the RKMap/RKSet ones in the same way.  Maybe append "_internal" or "_helper" to the RFMap/Set methods to distinguish them?

src/java.base/share/classes/jdk/internal/misc/CDS.java line 93:

> 91: 
> 92: 
> 93:     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?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/24757#discussion_r2060769019
PR Review Comment: https://git.openjdk.org/jdk/pull/24757#discussion_r2060794048
PR Review Comment: https://git.openjdk.org/jdk/pull/24757#discussion_r2060762727