RFR(L) 8244778 Archive full module graph in CDS

[Lois Foltan]

Hi Ioi,

Overall looks promising.  I have some review comments below, but not a 
complete review.  I concentrated on the JVM side primarily how the 
archived module graph is read in, how the ModuleEntry and PackageEntry 
tables are created from the archive for the 3 builtin loaders and 
potential timing issues during start up.

On 7/22/2020 3:36 PM, Ioi Lam wrote:
> https://bugs.openjdk.java.net/browse/JDK-8244778
> http://cr.openjdk.java.net/~iklam/jdk16/8244778-archive-full-module-graph.v01/ 
>
>
> Please review this patch that stores the full module graph in the CDS
> archive heap. This reduces the initialization time of the basic JVM by
> about 22%:
>
> $ perf stat -r 100 bin/java -version
> before: 98,219,329 instructions 0.03971 secs elapsed (+- 0.44%)
> after:  55,835,815 instructions 0.03109 secs elapsed (+- 0.65%)
>
> [1] Start with ModuleBootstrap.java. The current implementation is
>     quite restrictive: the archived module graph is used only when no
>     module options are specified.
>
>     See ModuleBootstrap.mayUseArchivedBootLayer().
>
>     We can probably support options such as main module and module path
>     in a future RFE.

Yes, I noticed the restrictions.  The JBS issue discusses the 
possibility of using the dumped module graph in the event that the value 
of the options are the same.  I think for this implementation to be 
viable and have a positive impact you should consider comparing at least 
the options --add-modules, --add-exports and --add-reads.

>
> [2] In the current JDK implementation, there is no single object
>     that represents "the module graph". Most of the information
>     is stored in the archive bootLayer object, but a few additional
>     restoration operations need to be performed:
>
>     + See ModuleBootstrap.getArchivedBootLayer()
>     + Some static fields need to be archived/restored in
>       Module.java, BuiltinClassLoader.java, ClassLoaders.java
>       and BootLoader.java
>
> [3] I ran into a complication with two loader instances of
>     PlatformClassLoader and AppClassLoader. They are stored in
>     multiple tables inside the module graph (e.g.,
>     BuiltinClassLoader$LoadedModule) so I cannot easily recreate
>     them at runtime.
>
>     However, these two loaders contain information specific to the
>     dump time VM lifecycle (such as the classes that were loaded
>     during CDS dumping) that need to be scrubbed. I couldn't find an
>     elegant way of doing this, so I added a private "resetArchivedStates"
>     method to a few classes. They are called inside
>     HeapShared::reset_archived_object_states().
>
> [4] Related native data structures (PackageEntry and ModuleEntry)
>     are also archived. Start with classLoaderData.cpp
>
> Passes mach5 tiers 1-4. I will test with additional tiers.
>
> Thanks
> - Ioi

classfile/classLoader.cpp
- line #1644 pulling the javabase_moduleEntry() check outside of the 
Module_lock maybe problematic if after you check it another
   thread initializes in the time between the check and the obtaining of 
the Module_lock on line #1645.

classfile/classLoader.hpp
- somewhere in ArchivedClassLoaderData there should be an assert to make 
sure that the CLD, whose package entries and module entries are
being archived is not a "_has_class_mirror_holder" CLD for a weakly 
defined hidden class.  Those dedicated CLDs should never have
package entries or module entries.

classfile/moduleEntry.cpp
- line #400, typo "conver" --> "convert"
- line #500, maybe sort if n is greater than 1 instead of 0 (same 
comment for packageEntry.cpp line #270)

classfile/systemDictionary.cpp
- It looks like the PackageEntry and ModuleEntry tables for the system 
and platform class loaders are  added within 
SystemDictionary::compute_java_loaders() which is called from 
Thread::create_vm() but after the call in Thread::create_vm() to 
call_initPhase2 which is when Universe::_module_initialized is set to 
true.  Did I read this correctly?  If yes, then I think you have to be 
sure that Universe::_module_initialized is not set until the module 
graph for the 3 builtin loaders is loaded from the archive.

memory/filemap.cpp
- line #237 typo "modue" --> "module"

oops/instanceKlass.cpp
- line #2545, comment "TO DO  -- point it to the archived PackageEntry 
(JDK-8249262)"
are you thinking that since the module graph is read in ahead of time 
that it can be set when an InstanceKlass is created?  There is a point 
before java.base is defined that InstanceKlass::set_package takes into 
account that could be a timing issue.

Clarification/timing questions:

- Since the CDS support for capturing the module graph does not archive 
unnamed modules, I assume Modules::set_bootloader_unnamed_module() is 
still called.  Is this true or does the unnamed module for the boot 
loader get archived?

- There are some checks in modules.cpp that are valuable to have during 
bootstrapping.  For example, packages that are encountered during 
bootstrapping before java.base is defined are put into the java.base 
module but then verified after java.base is defined via 
verify_javabase_packages.  So at what point in the bootstrapping process 
does the boot loader's archived module's become known? Could classes 
have been defined to the VM before this point?  Then their packages will 
have to be verified to be sure that they are indeed packages within 
java.base.  Consider looking at other checks that occur within 
classfile/modules.cpp as well.

I may have more review comments as I continue to look at this!

Thanks,
Lois