RFR(L) 8244778 Archive full module graph in CDS
Coleen Phillimore
coleen.phillimore at oracle.com
Mon Aug 31 22:02:08 UTC 2020
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
>>>>
>>>
>>
>
More information about the hotspot-runtime-dev
mailing list