RFR: 8207812: Implement Dynamic CDS Archive
Calvin Cheung
calvin.cheung at oracle.com
Mon Apr 29 17:13:38 UTC 2019
Hi Jiangli,
Thanks for the re-review.
Please see my comments in-line below...
On 4/24/19, 7:54 PM, Jiangli Zhou wrote:
> Please see comments inlined.
>
> On Tue, Apr 23, 2019 at 5:08 PM Calvin Cheung<calvin.cheung at oracle.com> wrote:
>> Hi Jiangli,
>>
>> Thanks a lot for your review!
>>
>> On 4/22/19, 2:07 PM, Jiangli Zhou wrote:
>>> Hi Calvin,
>>>
>>> Congrats on finalizing the dynamic archiving work and completing
>>> testing. After the integration of the dynamic archiving, a follow-up
>>> RFE can be done to merge the archiving/copying code in
>>> dynamicArchive.* and metaspaceShared.* for better maintenance in the
>>> future. As there are many duplicates between those two, having shared
>>> implementation for both static and dynamic will be beneficial and
>>> reduce the maintenance cost.
>> I'll file an RFE for the above.
>>> Here are my comments mainly for additional cleanups and some minor issues.
>>>
>>> - src/hotspot/share/classfile/classLoader.cpp
>>>
>>> 1337 // FIXME: DynamicDumpSharedSpaces and --patch-modules are
>>> mutually exclusive
>>> 1338 assert(!DynamicDumpSharedSpaces, "sanity");
>>>
>>> I tagged the comment with 'FIXME' to serve as a reminder to add more
>>> details. The reason DynamicDumpSharedSpaces is 'mutually exclusive'
>>> with with --patch-modules because DynamicDumpSharedSpaces is only
>>> enabled when UseSharedSpaces is also enabled. As --patch-modules is
>>> not supported with UseSharedSpaces, it is not supported with
>>> DynamicDumpSharedSpaces either.
>> I've converted the FIXME to a comment.
>>> 1518 ik->set_shared_classpath_index(UNREGISTERED_INDEX);
>>> 1519 SystemDictionaryShared::set_shared_class_misc_info(ik,
>>> (ClassFileStream*)stream);
>>>
>>> Please add assert(DynamicDumpSharedSpaces, "sanity"); to the above
>>> code. With the new dynamic archiving capability, it's now able to
>>> load/archive a class with user defined classloader via this call path.
>>> A comment explaining this is also needed.
>> I tried the assert but it didn't work. Not only DynamicDumpSharedSpaces
>> will go through that code path.
> I should be more clear. The new code is only intended for the
> DynamicDumpSharedSpaces, since the shared_classpath_index is set to
> UNREGISTERED_INDEX by ClassLoaderExt::load_class when loading class
> with "source:" in the class list file at static dumping time.
>
> 1518 ik->set_shared_classpath_index(UNREGISTERED_INDEX);
> 1519 SystemDictionaryShared::set_shared_class_misc_info(ik,
> (ClassFileStream*)stream);
>
> After thinking more, it's probably better to remove the following
> marked code from ClassLoaderExt::load_class. That avoids setting twice
> in two different places during static dumping. It also makes the code
> cleaner.
>
> InstanceKlass* ClassLoaderExt::load_class(Symbol* name, const char*
> path, TRAPS) {
> ...
> result->set_shared_classpath_index(UNREGISTERED_INDEX);<<<<<<<<<<<
> SystemDictionaryShared::set_shared_class_misc_info(result, stream);
> <<<<<<<<<<<<
http://cr.openjdk.java.net/~ccheung/8207812_dynamic_cds_archive/delta_01_02/src/hotspot/share/classfile/classLoaderExt.cpp.html
Only the first statement is still there. I agree that the
set_shared_classpath_indexd() can be removed.
>
>>> - src/hotspot/share/classfile/classLoaderExt.cpp
>>>
>>> 64 void ClassLoaderExt::setup_app_search_path() {
>>> 65 assert(DumpSharedSpaces || DynamicDumpSharedSpaces,
>>> 66 "this function is only used with -Xshare:dump");
>>>
>>> The above message needs to be updated to reflect the new command-line option.
>> Done.
>>> 304 result->set_shared_classpath_index(UNREGISTERED_INDEX);
>>> 305 SystemDictionaryShared::set_shared_class_misc_info(result,
>>> stream);<<<<<<<<<<
>>>
>>> Why is the set_shared_class_misc_info call being removed? If this is a
>>> bug fix for loading classes from the classlist for user defined
>>> classloaders, it should be handled separately, and with a separate bug
>>> ID as well.
>> It is called in ClassLoader::record_result() from
>> KlassFactory::create_from_stream().
>
> Ok, this is related to the above comment.
>
>>> - src/hotspot/share/classfile/compactHashtable.cpp
>>>
>>> 207 size_t SimpleCompactHashtable::calculate_header_size() {
>>> 208 // We have 5 fields. Each takes up sizeof(intptr_t). See
>>> WriteClosure::do_u4
>>> 209 size_t bytes = sizeof(intptr_t) * 5;
>>> 210 return bytes;
>>> 211 }
>>>
>>> 212
>>> 213 void SimpleCompactHashtable::serialize_header(SerializeClosure* soc) {
>>> 214 // NOTE: if you change this function, you MUST change the number 5 in
>>> 215 // calculate_header_size() accordingly.
>>> ...
>>>
>>> As a cleanup, a better way to handle this is to calculate the size
>>> within SimpleCompactHashtable::serialize_header during serializing the
>>> data and set the size value in a valuable.
>>> SimpleCompactHashtable::calculate_header_size() should simply retrieve
>>> the value. A renaming of
>>> SimpleCompactHashtable::calculate_header_size() can also be done.
>> I've checked with Ioi on this one. The problem is
>> calculate_header_size() needs to be called during size estimation, and
>> serialize_header is called after size estimation.
> Can you please file a RFE for this? The current code is okay for the
> first integration. It deserves some efforts to make it cleaner
> (probably with a different solution) since it can be error-prone.
I've filed:
https://bugs.openjdk.java.net/browse/JDK-8223004
Avoid using a hard-coded number in
SimpleCompactHashtable::calculate_header_size()
>
>>> - src/hotspot/share/classfile/dictionary.cpp
>>>
>>> 315 InstanceKlass* Dictionary::find_class(Symbol* name) {
>>> 316 unsigned int hash = compute_hash(name);
>>> 317 int index = hash_to_index(hash);
>>> 318 return find_class(index, hash, name);
>>> 319 }
>>>
>>> Looks like the new function is not references (unless I'm missing
>>> something). Please remove the function.
>>>
>>> - src/hotspot/share/classfile/dictionary.hpp
>>>
>>> 65 InstanceKlass* find_class(Symbol* name);
>>>
>>> Same comment as the above.
>> I've removed the function.
>>> - src/hotspot/share/classfile/symbolTable.cpp.
>>>
>>> 473 Symbol* const _archived; // used by UseSharedArchived2
>>>
>>> Please removed 'UseSharedArchived2'. The comment also needs more clarifications.
>>>
>>> I couldn't find any references to SymbolTableCreateEntry. Can you
>>> please point to me where it is being used?
>> I've removed the entire SymbolTableCreateEntry class. It was left there
>> probably due to merge error.
>>> - src/hotspot/share/classfile/systemDictionaryShared.cpp
>>>
>>> 1218 if (DynamicDumpSharedSpaces) {
>>> 1219 return false;
>>> 1220 } else {
>>>
>>> The above case for DynamicDumpSharedSpaces needs to be examined
>>> carefully. Can you please ask Harold (and Coleen or Karen) to take a
>>> look? Also, a comment is needed to explain that we can complete all
>>> verification checks at dynamic dumping time.
>> I've added a comment. If it return false, the caller will call
>> VerificationType::resolve_and_check_assignability().
>>> - src/hotspot/share/classfile/systemDictionaryShared.cpp
>>>
>>> 1279 ResourceMark rm;
>>>
>>> You can use 'ResourceMark rm(THREAD)'.
>> Fixed.
>>> - src/hotspot/share/memory/allocation.hpp
>>>
>>> 255 //
>>> 256 // When CDS is not enabled, both pointers are set to NULL.
>>> 257 static void* _shared_metaspace_base; // (inclusive) low address
>>> 258 static void* _shared_metaspace_top; // (exclusive) high addres
>>>
>>> Why the comment at line 256 was removed?
>> I've added back the comment.
>>> - src/hotspot/share/memory/filemap.cpp
>>>
>>> 101 void FileMapInfo::fail_continue(const char *msg, ...) {
>>> 102 va_list ap;
>>> 103 va_start(ap, msg);
>>> 104 if (_runtime_dynamic_info == NULL) {
>>> 105 MetaspaceShared::set_archive_loading_failed();
>>> 106 } else {
>>> 107 DynamicArchive::disable();
>>> 108 }
>>>
>>> The above fail_continue only works if _runtime_dynamic_info is setup
>>> after the mapping the base archive. Comments should be add to explain
>>> that.
>> Comment added.
>>> Can you please rename '_runtime_dynamic_info' so it's more
>>> descriptive? Maybe use 'dynamic_archive_info'.
>> Renamed to '_dynamic_archive_info'.
>>> 587 bool FileMapInfo::same_files(const char* file1, const char* file2) {
>>>
>>> The usage of FileMapInfo::same_files is not necessary and should be
>>> removed. The base archive's CRC checksum values are recorded in the
>>> dynamic archive. The runtime verifies the CRC values to make sure the
>>> same archive is used at dump time and runtime, regardless of the base
>>> archive path or name. It is designed for all use cases:
>> The same_files() function is also used in arguments.cpp:
>> 3530 if (DynamicDumpSharedSpaces) {
>> 3531 if (FileMapInfo::same_files(SharedArchiveFile,
>> ArchiveClassesAtExit)) {
>> 3532 vm_exit_during_initialization(
>> 3533 "Cannot have the same archive file specified for
>> -XX:SharedArchiveFile and -XX:ArchiveClassesAtExit",
>> 3534 SharedArchiveFile);
>> 3535 }
>> 3536 }
>>
>> The function is also needed for the RFE:
>> https://bugs.openjdk.java.net/browse/JDK-8211723
> Ok. It should be treated a bug, not a RFE.
>
> The shared path table check does not verify the path ordering (also
> including the case when new path components are inserted). The bug
> should be handled as a high priority task for dynamic archive.
I saw that you and Karen have had some discussion in the bug report
since you sent this review comment. I take that you're fine with that.
>
>> We still verify the CRC values during runtime.
>>> * base CDS archive is specified in the -XX:SharedArchiveFile at
>>> dynamic dumping time
>>> * -XX:SharedArchiveFile is not specified at dynamic dumping time,
>>> default location for the default CDS archive is used
>>> * default CDS archive is specified in the -XX:SharedArchiveFile at runtime
>>> * default CDS archive is not specified in the -XX:SharedArchiveFile at
>>> runtime, default location for the default CDS archive is used
>> Regarding the fourth point above, the user could have a non-default base
>> archive and only specify the top archive during runtime.
> I would argue against it since it doesn't always work and adds extra
> code. When the archive path/name is changed, the recorded one in the
> dynamic archive would no longer work. User still need to specify the
> path/name in the command-line. The use case only works for the default
> CDS archive. For non-default CDS archive, specifying in the
> command-line option results a cleaner design and less fragile code.
If the base archive is moved, then the user has to modify the
command-line anyway, whether the user initially specified (a) only the
top archive, or (b) both archives.
Requiring both archives to be specified will only hurt the users who
never move the archives.
We'd like to leave it as is.
>
>>> In all above cases, the base archive CRC values check is sufficient.
>>> The use of path/name is fragile and should be avoided. That will allow
>>> you to remove the _base_archive_name_size from the dynamic archive.
>> We still need the _base_archive_name_size and the base archive name in
>> the header because of the above reason.
> Please see my comment above.
>
>>> 752 if (is_static) {
>>> 753 // FIXME check for dynamic header as well
>>> 754 // FIXME Don't just check the last region -- check all regions!
>>>
>>> Can you please address the first FIXME at line 753?
>>>
>>> Checking the last region is sufficient since the archive is written is
>>> sequential order. The second FIXME is not necessary.
>> I've addressed the first FIXME and converted the second one to a comment.
>>> - src/hotspot/share/memory/metaspace.cpp
>>>
>>> 1417 bool Metaspace::contains(const void* ptr) {
>>> 1418 // FIXME: need to check the dynamic archive
>>>
>>> Can you please remove the above FIXME? There is no need for a separate check.
>> Done.
>>> - src/hotspot/share/memory/metaspaceShared.cpp
>>>
>>> 830 intptr_t* MetaspaceShared::fix_cpp_vtable_for_second_archive
>>>
>>> Can you please rename the function to fix_cpp_vtable_for_dynamic_archive?
>> Done.
>>> - src/hotspot/share/oops/klass.cpp
>>>
>>> 527 assert (DumpSharedSpaces || DynamicDumpSharedSpaces,
>>> 528 "only called for DumpSharedSpaces");
>>>
>>> 544 void Klass::remove_java_mirror() {
>>> 545 assert(DumpSharedSpaces || DynamicDumpSharedSpaces, "only
>>> called for DumpSharedSpaces");
>>>
>>> Please fix the messages above.
>> Done.
>>> - src/hotspot/share/prims/whitebox.cpp
>>>
>>> 2332 {CC"getResolvedReferences",
>>> CC"(Ljava/lang/Class;)Ljava/lang/Object;",
>>> (void*)&WB_GetResolvedReferences},
>>> 2333 {CC"linkClass", CC"(Ljava/lang/Class;)V",
>>> (void*)&WB_LinkClass},
>>> 2334 {CC"areOpenArchiveHeapObjectsMapped", CC"()Z",
>>> (void*)&WB_AreOpenArchiveHeapObjectsMapped},
>>>
>>> Can you please align the indentation of line 2333 (to be the same as
>>> line 2332 or 2334)?
>> Aligned (void*) with line 2334. (It doesn't show in the webrev since
>> only blank space changes)
>>> - src/hotspot/share/runtime/arguments.cpp
>>>
>>> 1491 bool Arguments::check_unsupported_cds_runtime_properties() {
>>> 1492 assert(UseSharedSpaces, "this function is only used with
>>> -Xshare:{on,auto}");
>>> 1493 assert(ARRAY_SIZE(unsupported_properties) ==
>>> ARRAY_SIZE(unsupported_options), "must be");
>>> 1494 if (ArchiveClassesAtExit != NULL) {
>>> 1495 // dynamic dumping, just return false,
>>> check_unsupported_dumping_properties() will be called
>>> 1496 // in init_shared_archive_paths().
>>> 1497 return false;
>>> 1498 }
>>>
>>> The check_unsupported_cds_runtime_properties() should be done for the
>>> 'ArchiveClassesAtExit != NULL' case as well. Dynamic dumping is a
>>> combination of both dump time and runtime.
>> The 'ArchiveClassesAtExit != NULL' is for dumping CDS archive to the
>> user's point of view, that's why the comments in lines 1495 and 1496.
>> During runtime, ArchiveClassesAtExit will be NULL, so the
>> check_unsupported_cds_runtime_properties() will be called as usual.
> During dynamic dumping, UseSharedSpace is true. Dynamic dumping is
> special case of the 'runtime', that's why Dynamic dumping it is a
> combination of both dump time and runtime. So
> check_unsupported_cds_runtime_properties() is also need for dynamic
> dumping.
If the check_unsupported_cds_runtime_properties() is called for dynamic
dumping, the user will see a warning message
warning("CDS is disabled when the %s option is specified.",
unsupported_options[i]);
instead of the following error with my current patch:
vm_exit_during_initialization("Cannot use the following option
when dumping the shared archive", unsupported_options[i]);
which gives the user the same feedback on the static dumping case.
We think it is important to give correct error message to the user. We'd
like to leave the code as is but have updated the comment as follows:
if (ArchiveClassesAtExit != NULL) {
// dynamic dumping, just return false for now.
// check_unsupported_dumping_properties() will be called later to
check the same set of
// properties, and will exit the VM with the correct error message
if the unsupported properties
// are used.
return false;
}
Here's an updated delta webrev:
http://cr.openjdk.java.net/~ccheung/8207812_dynamic_cds_archive/delta_01_02%2b/
Additional changes comparing with delta_01_02 include:
- the mentioned one-line removal in classLoaderExt.cpp;
- updated comment in arguments.cpp;
- small fixes to handle tests run in -Xshare:off mode;
- enable building on the zero platform.
Comparing with the delta_01_02 webrev, the additional changed files are:
> make/hotspot/lib/JvmFeatures.gmk
> src/hotspot/share/classfile/symbolTable.hpp
> src/hotspot/share/runtime/arguments.hpp
>
test/hotspot/jtreg/runtime/appcds/dynamicArchive/DynamicArchiveTestBase.java
> test/hotspot/jtreg/runtime/appcds/dynamicArchive/HelloDynamicCustom.java
Thanks again for your review and contribution to this RFE.
Calvin
>>> 2729 // -Xshare:auto || -Xshare:dynamicDump
>>>
>>> As you've renamed the command-line argument for dynamic dumping
>>> support, the comment needs to be fixed.
>> Fixed.
>>> 3125 // Compiler threads may concurrently update the class
>>> metadata (such as method entries), so it's
>>> 3126 // unsafe with DumpSharedSpaces (which modifies the class
>>> metadata in place). Let's disable
>>> 3127 // compiler just to be safe.
>>> 3128 //
>>> 3129 // Note: this is not a concern for DynamicDumpSharedSpaces,
>>> which makes a copy of the class metadata
>>> 3130 // instead of modifying them in place. The copy is
>>> inaccessible to the compiler.
>>> 3131 set_mode_flags(_int);
>>>
>>> We need to come back to revisit the above for the 'static' archive
>>> dumping at one point. There is a RFE filed for that, if I remember
>>> correctly. Could you please add a 'TODO' notes in the above comment.
>> Added TODO.
>>> A check should be done in arguments.cpp to make sure
>>> DynamicDumpSharedSpaces is not manipulated from the command-line
>>> directly. DynamicDumpSharedSpaces should not be enabled in the
>>> command-line without ArchiveClassesAtExit being specified.
>> Done.
>>> - src/hotspot/share/runtime/java.cpp
>>>
>>> 509
>>> 510 // FIXME: is this the right place?
>>> 511 if (DynamicDumpSharedSpaces) {
>>> 512 DynamicArchive::dump();
>>> 513 }
>>>
>>> Again, the above 'FIXME' is served as a cleanup reminder. Please get
>>> opinions from others on this change. If the calling place is okay,
>>> please remove the FIXME.
>> Removed the FIXME for now. Checked with David H. He indicated there's no
>> easy answer for this. Just need to do a lot of testing.
>>> - test
>>>
>>> Could you please add a test case for setting DynamicDumpSharedSpaces
>>> from command-line?
>> Here's an incremental webrev which contains a new test:
>>
>> http://cr.openjdk.java.net/~ccheung/8207812_dynamic_cds_archive/delta_01_02/
>>
>> thanks,
>> Calvin
>>> I only took a brief look of the test changes. Please ask Misha to
>>> review the test changes as well.
>>>
>>> Thanks and regards,
>>> Jiangli
> Thanks,
> Jiangli
More information about the hotspot-runtime-dev
mailing list