On Fri, 25 Oct 2024 15:02:30 GMT, Ashutosh Mehra <asme...@openjdk.org> wrote:
>> Ioi Lam has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - 8342907: Implement AOT testing mode for jtreg tests (authored by
>> @katyapav)
>> - disable test that fails with hotspot_runtime_non_cds_mode
>
> src/hotspot/share/cds/aotClassLinker.cpp line 117:
>
>> 115:
>> 116: void AOTClassLinker::add_candidate(InstanceKlass* ik) {
>> 117: _candidates->put_when_absent(ik, true);
>
> I just noticed the use of `put_when_absent` here. This means the caller
> should ensure that `ik` has not already been added to the `_candidates`. We
> do that currently (`try_add_candidate` calls `is_candidate` before calling
> `add_candidate`) but probably worth mentioning this contract explicitly in a
> comment somewhere for future readers.
I renamed it to `add_new_candidate()` and added comments so that it's clear
what the contract is. I also added assert to check for thread safety.
> src/hotspot/share/cds/aotConstantPoolResolver.cpp line 113:
>
>> 111:
>> 112: if (CDSConfig::is_dumping_aot_linked_classes()) {
>> 113: // Need to call try_add_candidate instead of is_candidate, as
>> this may be called
>
> I think in this version of the code this method is not used before
> `AOTClassLinker::add_candidates`. Can we switch back to `is_candidate` then?
In the future, these functions may be called before we enter the safepoint
(e.g., to check if some constant pool entries can be resolved), so I think it's
better to keep the `try_add_candidate()` call.
> src/hotspot/share/cds/heapShared.hpp line 298:
>
>> 296: static SeenObjectsTable *_seen_objects_table;
>> 297:
>> 298: // The "special subgraph" contains all the all archived objects that
>> are reachable
>
> an extra `all` in the comment
Fixed.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/21642#discussion_r1823682059
PR Review Comment: https://git.openjdk.org/jdk/pull/21642#discussion_r1823684639
PR Review Comment: https://git.openjdk.org/jdk/pull/21642#discussion_r1823684779
