RFR: 8207812: Implement Dynamic CDS Archive
Calvin Cheung
calvin.cheung at oracle.com
Wed Apr 24 00:08:38 UTC 2019
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.
>
> - 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().
>
> - 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.
>
> - 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
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.
>
> 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.
>
> 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.
>
> 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
More information about the hotspot-runtime-dev
mailing list