On Thu, 16 Oct 2025 01:16:54 GMT, Ioi Lam <[email protected]> wrote:
>> Erik Österlund has updated the pull request incrementally with four
>> additional commits since the last revision:
>>
>> - Dont print error message when there are no archived objects
>> - Fix issue in new test
>> - Change is_aot_thread implementation
>> - update AotJdk problem lists
>
> src/hotspot/share/cds/cds_globals.hpp line 79:
>
>> 77: "the CDS archive, in the specified file")
>> \
>> 78:
>> \
>> 79: product(bool, AOTStreamableObjects, true,
>> \
>
> The default should be `false`, as that will be the mode that the user will
> get with no GC options are specified.
Makes sense. Fixed.
> src/hotspot/share/cds/heapShared.cpp line 355:
>
>> 353: }
>> 354:
>> 355: bool HeapShared::is_loading_mapping_mode() {
>
> I think `is_loading_mapping_mode()` should be removed and we should use only
> `is_loading_streaming_mode()`. This will make it easy to find all the places
> that checks between mapping/streaming. Same for `is_writing_xxx_mode()`.
>
> Also, instead of having a tri-state of `_heap_load_mode`, it's better to have
> two boolean states:
>
> - _is_loading_heap
> - _is_loading_heap_streaming_mode
>
> The first boolean is the main switch, but most of the code will be checking
> only the second boolean.
Okay. @stefank is having a look at fixing this.
> src/hotspot/share/cds/heapShared.cpp line 410:
>
>> 408: FLAG_SET_ERGO(AOTStreamableObjects, true);
>> 409: _heap_write_mode = HeapArchiveMode::_streaming;
>> 410: return;
>
> This should be changed to `if (!UseZGC) {`, as all other GCs support writing
> the "mapping" format.
Fixed.
> src/hotspot/share/cds/heapShared.cpp line 418:
>
>> 416:
>> 417: // Select default mode
>> 418: if (UseG1GC && UseCompressedOops) {
>
> This is different than specified in JEP 516. The condition should be `if
> (UseCompressedOops)`.
Okay, I changed it and cleaned up some comments and code suggesting dumping
must be done with G1.
> test/hotspot/jtreg/runtime/cds/appcds/TestSerialGCWithCDS.java line 117:
>
>> 115: small2,
>> 116: coops,
>> 117: "-XX:-AOTStreamableObjects",
>
> Is this change necessary?
No, I removed it. Nice catch.
> test/hotspot/jtreg/runtime/cds/appcds/TestSerialGCWithCDS.java line 147:
>
>> 145: // Regardless of which GC dumped the heap, there will be an
>> object archive, either
>> 146: // created with mapping if dumped with G1, or streaming if
>> dumped with serial GC.
>> 147: // At exec time, try to load them into a small SerialGC
>> heap that may be too small.
>
> Actually the original check of `dumpWithSerial == false` was obsolete, as
> SerialGC can also dump heap objects. `dumpWithSerial == false` should be
> removed. The first two lines of comments should also be removed.
Okay, fixed.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/27732#discussion_r2440306099
PR Review Comment: https://git.openjdk.org/jdk/pull/27732#discussion_r2440305408
PR Review Comment: https://git.openjdk.org/jdk/pull/27732#discussion_r2440309294
PR Review Comment: https://git.openjdk.org/jdk/pull/27732#discussion_r2440308642
PR Review Comment: https://git.openjdk.org/jdk/pull/27732#discussion_r2440303912
PR Review Comment: https://git.openjdk.org/jdk/pull/27732#discussion_r2440309778
