RFR(L) 8244778 Archive full module graph in CDS

[Ioi Lam]

Hi Coleen,

Thanks for the comments. I have an updated webrev

http://cr.openjdk.java.net/~iklam/jdk16/8244778-archive-full-module-graph.v02/
http://cr.openjdk.java.net/~iklam/jdk16/8244778-archive-full-module-graph.v02.delta/


On 8/4/20 7:49 AM, Coleen Phillimore wrote:
> This is sort of massive, so I'll only make a couple initial comments.
>
> http://cr.openjdk.java.net/~iklam/jdk16/8244778-archive-full-module-graph.v01/src/hotspot/share/classfile/classLoaderData.cpp.udiff.html 
>
>
> All this new code in ClassLoaderData should go somewhere else, like 
> maybe classLoaderDataShared.hpp/cpp ?  I don't see how it needs to be 
> owned by CLD, except maybe the serialized function.
>
>
Fixed

> + if (loader_data) {
>
>
> There are many checks that should be != NULL here.
>

Fixed and added comments


> http://cr.openjdk.java.net/~iklam/jdk16/8244778-archive-full-module-graph.v01/src/hotspot/share/classfile/javaClasses.cpp.udiff.html 
>
>
> I don't see why compute_offset/s() has been exported outside 
> javaClasses.cpp.
>

It was left-over code that shouldn't be there. I removed it.


> http://cr.openjdk.java.net/~iklam/jdk16/8244778-archive-full-module-graph.v01/src/hotspot/share/classfile/moduleEntry.cpp.udiff.html 
>
> http://cr.openjdk.java.net/~iklam/jdk16/8244778-archive-full-module-graph.v01/src/hotspot/share/classfile/packageEntry.cpp.udiff.html 
>
>
> These look like the same code to me.  Maybe they can be generalized 
> and put together in a moduleEntryShared.cpp/hpp ?
>

They look similar but also do different things. Any suggestion for how 
to generalize them?


> http://cr.openjdk.java.net/~iklam/jdk16/8244778-archive-full-module-graph.v01/src/hotspot/share/classfile/systemDictionary.cpp.udiff.html 
>
>
> Why do you register the loaders here?
>

It's to force the creation of the ClassLoaderData if a loader oop 
doesn't have one yet. Also, in the new version, I moved the code to 
Modules::define_archived_modules().


Thanks
- Ioi

> I don't know enough about the Java code to review that.
>
> I assume that this going to be rebased on other changes, so this is 
> only an incomplete review.
>
> Coleen
>
>
> On 7/22/20 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.
>>
>> [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
>