RFR(L) 8244778 Archive full module graph in CDS

[Coleen Phillimore]

http://cr.openjdk.java.net/~iklam/jdk16/8244778-archive-full-module-graph.v02/src/hotspot/share/classfile/classLoaderData.cpp.udiff.html

Are these #includes still needed?

http://cr.openjdk.java.net/~iklam/jdk16/8244778-archive-full-module-graph.v02/src/hotspot/share/classfile/classLoaderDataShared.hpp.html

Looks like the include guards are wrong.  Should have pushed my #pragma 
once change then nobody would have to worry about these.

http://cr.openjdk.java.net/~iklam/jdk16/8244778-archive-full-module-graph.v02/src/hotspot/share/classfile/classLoaderDataShared.cpp.html

  136 void ClassLoaderDataShared::init_archived_tables() {
  137   assert(DumpSharedSpaces, "must be");
  138   if (MetaspaceShared::use_full_module_graph()) {


Shouldn't these functions be called only if 
MetaspaceShared::use_full_module_graph() instead of having them guarded 
entirely by these if statements.
ie. assert(DumpSharedSpaces && MetaspaceShared::use_full_module_graph(), 
"must be");

Thank you for moving the new code to classLoaderDataShared.cpp

http://cr.openjdk.java.net/~iklam/jdk16/8244778-archive-full-module-graph.v02/src/hotspot/share/classfile/packageEntry.cpp.frames.html

Can you organize this so that the archiving functions for PackageEntry 
are after the PackageEntry:: functions, and the ones for 
PackageEntryTable are after the PackageEntryTable:: functions?

They'll still be sort of mixed up, but the CDS special code in the 
middle isn't good.  And if we ever want to change the entries or the 
tables themselves it'll be easier.

http://cr.openjdk.java.net/~iklam/jdk16/8244778-archive-full-module-graph.v02/src/hotspot/share/classfile/moduleEntry.cpp.frames.html

Same comment.  The code in both looks fine, even though it looks the 
same. I don't know how to generalize it without being artificial and not 
resembling either.  Separating the functions will help.

On 8/12/20 6:11 PM, Ioi Lam wrote:
> 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().

Yes, this is a better place.

Thanks,
Coleen
>
>
> 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
>>
>