RFR(L) 8244778 Archive full module graph in CDS

[Coleen Phillimore]

The delta version looks good.

On 8/31/20 11:41 AM, Ioi Lam wrote:
> Hi Coleen,
>
> Thanks for your comments. Please see the new version:
>
> http://cr.openjdk.java.net/~iklam/jdk16/8244778-archive-full-module-graph.v03/ 
>
> http://cr.openjdk.java.net/~iklam/jdk16/8244778-archive-full-module-graph.v03.delta/ 
>
>
>
> Replies in-line below
>
>
> On 8/25/20 1:24 PM, Coleen Phillimore wrote:
>>
>> 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?
>>
> These are left-overs. I removed them.
>
>> 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.
>>
>
> Fixed.
>
>> 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");
>>
>
> I fixed as you suggested.
>
>> 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.
>
>
> The code of PackageEntry and PackageEntryTable are somewhat mixed up 
> in packageEntry.cpp. I've tried to put my new code for PackageEntry 
> after the first block of code for PackageEntry. Same for 
> PackageEntryTable. If we want to reorder packageEntry.cpp, maybe we 
> should do that in a separate RFE?
>
> (Same for moduleEntry.cpp).

Yes, this is fine as another RFE.
thanks,
Coleen
>
> Thanks
> - Ioi
>
>
>>
>> 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
>>>>
>>>
>>
>