RFR: 8207812: Implement Dynamic CDS Archive
Jiangli Zhou
jianglizhou at google.com
Tue Apr 30 02:42:18 UTC 2019
Hi Calvin,
The updates look ok. Please make sure to test with a minimum build as well.
Best,
Jiangli
On Mon, Apr 29, 2019 at 10:13 AM Calvin Cheung <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> 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