RFR: 8309240: Array classes should be stored in dynamic CDS archive [v2]

[Ioi Lam]

On Wed, 26 Jul 2023 00:05:56 GMT, Calvin Cheung <ccheung at openjdk.org> wrote:

>> Please review this RFE for adding array classes in the dynamic CDS archive.
>> 
>> During dynamic dump, only the array classes with the `element_klass` already in the static archive will be stored in the dynamic archive. See `ArchiveBuilder::gather_array_klasses()`. It is because the array classes which are in the same archive (either static or dynamic) are already "connected" together. Higher dimension primitive array classes are also stored in the dynamic archive. Note that one-dimensional primitive array classes are store in the static archive.
>> 
>> During runtime, the `DynamicArchive::setup_array_klasses()` is for setting up the array classes. If only the `bottom_klass` is in the static archive, the function will point the `array_klasses` to the stored array classes in the dynamic archive. If a lower dimension array class is already in the static archive, its `higher_dimension` will be set to refer to the stored array classes in the dynamic archive.
>> 
>> Some logging is added to `ObjArrayKlass::array_klass(int n, TRAPS)` to indicate an array class is being loaded from the archive.
>> 
>> Passed tiers 1 - 4 testing.
>
> Calvin Cheung has updated the pull request incrementally with one additional commit since the last revision:
> 
>   simplify the fix

Changes requested by iklam (Reviewer).

src/hotspot/share/cds/archiveBuilder.cpp line 237:

> 235:   iterate_roots(&doit);
> 236:   if (DynamicDumpSharedSpaces) {
> 237:     iterate_primitive_array_klasses(&doit);

This call can be moved in `DynamicArchiveBuilder::iterate_roots`. That way, you don't need the dummy implementation of `StaticArchiveBuilder::iterate_primitive_array_klasses`.

src/hotspot/share/cds/archiveBuilder.cpp line 823:

> 821:   }
> 822:   log_debug(cds)("Total array klasses gathered for dynamic archive: %d", DynamicArchive::num_array_klasses());
> 823: }

This function can be put inside DynamicArchiveBuilder, since it's not used by the static dump.

src/hotspot/share/cds/dynamicArchive.cpp line 193:

> 191:       }
> 192:       if (ak != nullptr && (ak->dimension() > 1)) {
> 193:         // this is the lowest dimension that's not in the static archive

The `ak->dimension() > 1` should either be removed, or changed into an assert.

src/hotspot/share/oops/objArrayKlass.cpp line 323:

> 321:     DynamicArchive::log_array_class_load(THREAD, this);
> 322:   }
> 323: #endif

The `ObjArrayKlass::array_klass(int n, TRAPS)` function can be called multiple times, so you'd have duplicated output of "class, load, cds" for the same array class.

I think it's better to move the logging code from `DynamicArchive::log_array_class_load()` into a function like `ArrayClass::log_array_class_load()`. It can be called from:

- `ArrayKlass(Symbol* name, KlassKind kind)` constructor
- `ArrayKlass::restore_unshareable_info()`

That way we can catch the actual loading event, and also print out whether the class is in static CDS archive, dynamic CDS archive, or not in CDS.

Instead of `(class, load, cds)`, maybe we can use a new tag like `(class,load,array)`?

-------------

PR Review: https://git.openjdk.org/jdk/pull/14959#pullrequestreview-1546668082
PR Review Comment: https://git.openjdk.org/jdk/pull/14959#discussion_r1274264236
PR Review Comment: https://git.openjdk.org/jdk/pull/14959#discussion_r1274263669
PR Review Comment: https://git.openjdk.org/jdk/pull/14959#discussion_r1274271986
PR Review Comment: https://git.openjdk.org/jdk/pull/14959#discussion_r1274271197