On Fri, 25 Apr 2025 05:56:15 GMT, Ioi Lam <ik...@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

Reply via email to