RFR: 8244778: Archive full module graph in CDS [v4]

[Claes Redestad]

On Sat, 12 Sep 2020 06:35:31 GMT, Ioi Lam <iklam at openjdk.org> wrote:

>> This is the same patch as
>> [8244778-archive-full-module-graph.v03](http://cr.openjdk.java.net/~iklam/jdk16/8244778-archive-full-module-graph.v03/)
>> published in
>> [hotspot-runtime-dev at openjdk.java.net](https://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2020-August/041496.html).
>> The rest of the review will continue on GitHub. I will add new commits to respond to comments to the above e-mail.
>
> Ioi Lam has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes
> the unrelated changes brought in by the merge/rebase. The pull request contains eight additional commits since the last
> revision:
>  - Merge branch 'master' into 8244778-archive-full-module-graph
>  - Feedback from Coleen
>  - Removed TODO comment referring to JBS issue
>  - Merge branch 'master' into 8244778-archive-full-module-graph
>  - fixed trailing spaces
>  - Renamed ModuleEntry::write_growable_array
>  - Update to latest repo (JDK-8251557); added comments
>  - 8244778: Archive full module graph in CDS

Excellent work!

Only a few minor comments inline, which you can choose to ignore.

src/hotspot/share/classfile/classLoaderDataShared.cpp line 82:

> 80:   assert_valid(loader_data);
> 81:   if (loader_data != NULL) {
> 82:     // We can't create hashtables at dump time because the hashcode dependes on the

dependes -> depends

src/hotspot/share/classfile/classLoaderDataShared.cpp line 84:

> 82:     // We can't create hashtables at dump time because the hashcode dependes on the
> 83:     // address of the Symbols, which may be relocated at run time due to ASLR.
> 84:     // So we store the packages/modules in a Arrays. At run time, we create

run time -> runtime
a Arrays -> Arrays

src/hotspot/share/classfile/javaClasses.cpp line 4830:

> 4828:   if (klass == SystemDictionary::ClassLoader_klass() ||  // ClassLoader::loader_data is malloc'ed.
> 4829:       // The next 3 classes are used to implement java.lang.invoke, and are not used directly in
> 4830:       // regular Java code. The implementation of java.lang.invoke uses generated anonymoys classes

pre-existing: anonymoys

src/hotspot/share/classfile/modules.cpp line 462:

> 460:
> 461:   // We don't want the classes used by the archived full module graph to be redefined by JVMTI.
> 462:   // Luckily, such classes are loaded in the JVMTI "early" phase, and CDS is disable if a JVMTI

disabled

src/hotspot/share/memory/heapShared.cpp line 76:

> 74: // assigned at runtime.
> 75: static ArchivableStaticFieldInfo closed_archive_subgraph_entry_fields[] = {
> 76:   {"java/lang/Integer$IntegerCache",              0, "archivedCache"},

Could the changes here be simplified or clarified? I think the new field should be a bool, or we could instead
introduce a new array for the fields archived only when archiving the full module graph (the field is ignored on
iteration over closed_archive_subgraph_entry_fields anyhow)

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

Marked as reviewed by redestad (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/80