On Wed, 18 Sep 2024 05:01:00 GMT, David Holmes <dhol...@openjdk.org> wrote:
>> Ioi Lam has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fixed ZERO build
>
> src/hotspot/share/cds/aotLinkedClassBulkLoader.cpp line 66:
>
>> 64:
>> 65: void AOTLinkedClassBulkLoader::load_classes_in_loader(JavaThread*
>> current, AOTLinkedClassCategory class_category, oop class_loader_oop) {
>> 66: ExceptionMark em(current);
>
> Why do you need the EM when you are explicitly checking for exceptions?
Removed.
> src/hotspot/share/cds/aotLinkedClassBulkLoader.cpp line 67:
>
>> 65: void AOTLinkedClassBulkLoader::load_classes_in_loader(JavaThread*
>> current, AOTLinkedClassCategory class_category, oop class_loader_oop) {
>> 66: ExceptionMark em(current);
>> 67: ResourceMark rm(current);
>
> The RM should go where it is actually needed for the logging.
Fixed
> src/hotspot/share/cds/aotLinkedClassBulkLoader.cpp line 68:
>
>> 66: ExceptionMark em(current);
>> 67: ResourceMark rm(current);
>> 68: HandleMark hm(current);
>
> Why do you need a HM here?
It was needed when this code was called from a different path. It's no longer
needed now, so I removed it.
> src/hotspot/share/cds/aotLinkedClassBulkLoader.cpp line 95:
>
>> 93:
>> 94: if (Universe::is_fully_initialized() && VerifyDuringStartup) {
>> 95: // Make sure we're still in a clean slate.
>
> Suggestion:
>
> // Make sure we're still in a clean state.
Fixed
> src/hotspot/share/cds/aotLinkedClassBulkLoader.cpp line 132:
>
>> 130: break;
>> 131: case AOTLinkedClassCategory::UNREGISTERED:
>> 132: ShouldNotReachHere(); // Currently aot-linked classes are not
>> supported for this category.
>
> Suggestion:
>
> case AOTLinkedClassCategory::UNREGISTERED:
> default:
> ShouldNotReachHere(); // Currently aot-linked classes are not supported
> for this category.
Fixed.
> src/hotspot/share/cds/aotLinkedClassTable.hpp line 34:
>
>> 32: class SerializeClosure;
>> 33:
>> 34: // Classes to be buik-loaded, in the "linked" state, at VM bootstrap.
>
> Suggestion:
>
> // Classes to be bulk-loaded, in the "linked" state, at VM bootstrap.
Fixed.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1766121555
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1766121383
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1766121348
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1766121642
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1766121689
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1766121855
