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