RFR: 8207812: Implement Dynamic CDS Archive
Jiangli Zhou
jianglizhou at google.com
Mon Apr 22 21:07:18 UTC 2019
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.
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.
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.
- 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.
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.
- 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.
- 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.
- 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?
- 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.
- src/hotspot/share/classfile/systemDictionaryShared.cpp
1279 ResourceMark rm;
You can use 'ResourceMark rm(THREAD)'.
- 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?
- 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.
Can you please rename '_runtime_dynamic_info' so it's more
descriptive? Maybe use '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:
* 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
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.
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.
- 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.
- 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?
- 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.
- 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)?
- 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.
2729 // -Xshare:auto || -Xshare:dynamicDump
As you've renamed the command-line argument for dynamic dumping
support, the comment needs to be 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.
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.
- 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.
- test
Could you please add a test case for setting DynamicDumpSharedSpaces
from command-line?
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