RFR: 8207812: Implement Dynamic CDS Archive
Calvin Cheung
calvin.cheung at oracle.com
Tue Apr 30 17:46:42 UTC 2019
Karen,
Thanks for taking another look.
I'll take care of the typo you mentioned below.
thanks,
Calvin
On 4/30/19, 9:00 AM, Karen Kinnear wrote:
> Code changes look good to go for me.
> Many thanks for the updates and additional testing.
>
> thanks,
> Karen
>
> p.s. minor note: dynamicArchive.hpp line 57 “an copy” -> “a copy"
>
>> On Apr 29, 2019, at 1:13 PM, Calvin Cheung <calvin.cheung at oracle.com
>> <mailto:calvin.cheung at oracle.com>> wrote:
>>
>> 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 <mailto: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
>> <http://cr.openjdk.java.net/%7Eccheung/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/
>> <http://cr.openjdk.java.net/%7Eccheung/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/
>>>> <http://cr.openjdk.java.net/%7Eccheung/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