On Wed, 16 Oct 2024 22:56:06 GMT, Ashutosh Mehra <asme...@openjdk.org> wrote:
>> Ioi Lam has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fixed typo in last commit; fixed header inclusion order
>
> src/hotspot/share/cds/heapShared.cpp line 675:
>
>> 673: const char* klass_name = info->klass_name;
>> 674: for (; fields[i].valid(); i++) {
>> 675: ArchivableStaticFieldInfo* f = &fields[i];
>
> It looks like the pattern of nested for-loops is copied from
> HeapShared::archive_object_subgraphs()
> but this pattern doesn't seem to give any advantage here. If so can we
> replace it with just single for loop to make it easier to understand?
I changed it to a single loop as suggested.
> src/hotspot/share/classfile/systemDictionaryShared.cpp line 775:
>
>> 773: // Now, all hidden classes that have not yet been scanned must be
>> marked as excluded
>> 774: auto exclude_remaining_hidden = [&] (InstanceKlass* k,
>> DumpTimeClassInfo& info) {
>> 775: if (k->is_hidden() && !info.has_checked_exclusion()) {
>
> Suggestion: Check for `has_checked_exclusion()` is not really required as the
> call to `SDS::check_for_exclusion()` does that. And the condition in
> `guarantee()` can be updated as `info.is_excluded() || info.is_required()`.
I changed the code to the following to make sure that
`should_hidden_class_be_archived(k)` is mutually exclusive with
`info.is_excluded()`.
auto exclude_remaining_hidden = [&] (InstanceKlass* k, DumpTimeClassInfo&
info) {
if (k->is_hidden()) {
SystemDictionaryShared::check_for_exclusion(k, &info);
if (CDSConfig::is_dumping_invokedynamic()) {
if (should_hidden_class_be_archived(k)) {
guarantee(!info.is_excluded(), "Must be");
} else {
guarantee(info.is_excluded(), "Must be");
}
}
}
};
Unfortunately I can do this only for `CDSConfig::is_dumping_invokedynamic()`.
In the "legacy CDS" mode of lambda support, we actually would return true from
`should_hidden_class_be_archived(k)` and then later decide to exclude `k`.
> src/hotspot/share/oops/constantPool.cpp line 340:
>
>> 338: src_cp->iterate_archivable_resolved_references([&](int rr_index) {
>> 339: keep_resolved_refs.at_put(rr_index, true);
>> 340: });
>
> Indentation seems off here
Fixed.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/21143#discussion_r1810025512
PR Review Comment: https://git.openjdk.org/jdk/pull/21143#discussion_r1810025031
PR Review Comment: https://git.openjdk.org/jdk/pull/21143#discussion_r1810025626
