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