[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-runtime-dev
Subject:    Re: RFR: 8207812: Implement Dynamic CDS Archive
From:       Calvin Cheung <calvin.cheung () oracle ! com>
Date:       2019-04-30 17:46:42
Message-ID: 5CC88A02.1010703 () oracle ! com
[Download RAW message or body]

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@oracle.com 
> > <mailto:calvin.cheung@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@oracle.com <mailto:calvin.cheung@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
> 


[prev in list] [next in list] [prev in thread] [next in thread]