On Wed, 16 Oct 2024 23:16:00 GMT, Ashutosh Mehra <[email protected]> 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/dumpTimeClassInfo.hpp line 45:
>
>> 43: bool _is_early_klass;
>> 44: bool _has_checked_exclusion;
>> 45: bool _is_required;
>
> Is this flag only used for hidden classes?
I changed it to `_is_required_hidden_class`.
> src/hotspot/share/cds/heapShared.cpp line 717:
>
>> 715: };
>> 716:
>> 717: void HeapShared::find_archivable_hidden_classes_in_object(oop root) {
>
> If this function find archivable hidden classes, shouldn't there be a check
> for`is_hidden()` on the InstanceKlass being marked as `required`. Am I
> missing something?
I added the `ik->is_hidden()` check as you suggested. I also renamed the
`find_archivable_hidden_classes_xxx` functions to
`find_required_hidden_classes_xxx()`. This is to avoid confusion with
`HeapShared::is_archivable_hidden_klass()`, which means "it is possible to
archive X", but not "it is necessary to archive X".
> src/hotspot/share/cds/heapShared.cpp line 790:
>
>> 788:
>> 789: init_seen_objects_table();
>> 790: {
>
> Why was this scope introduced?
To indicate that these calls must be made while the "see objects" table is
available.
> src/hotspot/share/classfile/systemDictionaryShared.cpp line 675:
>
>> 673:
>> 674: // Returns true if the class should be excluded. This can be called
>> before
>> 675: // SystemDictionaryShared::check_excluded_classes().
>
> There are a couple of references to check_excluded_classes() in the comments
> but this function no longer exists. Can you please update the comment
> accordingly.
I replaced with the new name of the function `find_all_archivable_classes()`.
> src/hotspot/share/classfile/systemDictionaryShared.cpp line 759:
>
>> 757: if (k->is_hidden() && should_hidden_class_be_archived(k)) {
>> 758: SystemDictionaryShared::check_for_exclusion(k, &info);
>> 759: if (info.is_excluded()) {
>
> Is it possible that a hidden class for which
> `should_hidden_class_be_archived()` returns true is marked for exclusion by
> `SDS::check_for exclusion`? I guess only possibility is if for some reason
> its super or interface is excluded. Is that possible?
We filter out hidden classes that cannot be supported (e.g., when a Lambda uses
an interface using "old" bytecodes). See
https://github.com/openjdk/jdk/pull/21143/files#diff-fc60af483d061250f01054372fd56eba517a6b8dd37a742a66dfb0e15cb68e40R366-R372
https://github.com/openjdk/jdk/pull/21143/files#diff-2520dedd703ec600cd3037ec0008e9d7f45c6a41acaefac019339641bc749163R149-R152
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/21143#discussion_r1809709541
PR Review Comment: https://git.openjdk.org/jdk/pull/21143#discussion_r1809709627
PR Review Comment: https://git.openjdk.org/jdk/pull/21143#discussion_r1809709670
PR Review Comment: https://git.openjdk.org/jdk/pull/21143#discussion_r1809709979
PR Review Comment: https://git.openjdk.org/jdk/pull/21143#discussion_r1809710069
