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
