RFR(L) 8244778 Archive full module graph in CDS

[Ioi Lam]

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).

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
>>>
>>
>