On Wed, 30 Apr 2025 00:56:19 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:
>
> Renamed the internal field ReferenceQueue.NULL to NULL_QUEUE to avoid
> failing hotspot/jtreg/sources/TestNoNULL.java
lgtm. Just a few comments.
src/hotspot/share/cds/aotArtifactFinder.cpp line 77:
> 75: // into the AOT cache -- that will be decided by the code below.
> 76: SystemDictionaryShared::finish_exclusion_checks();
> 77: 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`
src/hotspot/share/cds/aotReferenceObjSupport.cpp line 95:
> 93: #if INCLUDE_CDS_JAVA_HEAP
> 94:
> 95:
Suggestion:
test/hotspot/jtreg/runtime/cds/appcds/aotClassLinking/WeakReferenceTest.java
line 233:
> 231: if (ref2.get() == null) {
> 232: throw new RuntimeException("ref2.get() should not be null");
> 233: }
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().
-------------
PR Review: https://git.openjdk.org/jdk/pull/24757#pullrequestreview-2806261263
PR Review Comment: https://git.openjdk.org/jdk/pull/24757#discussion_r2068189381
PR Review Comment: https://git.openjdk.org/jdk/pull/24757#discussion_r2068186097
PR Review Comment: https://git.openjdk.org/jdk/pull/24757#discussion_r2068180118
