RFR(L) 8231610 Relocate the CDS archive if it cannot be mapped to the requested address

Ioi Lam ioi.lam at oracle.com
Fri Nov 8 00:22:14 UTC 2019


Hi Coleen,

Thanks for the review. Here's an webrev that has incorporated your 
suggestions:

http://cr.openjdk.java.net/~iklam/jdk14/8231610-relocate-cds-archive.v06-delta/

Please see comments in-line

On 11/7/19 2:46 PM, coleen.phillimore at oracle.com wrote:
> Hi, I've done a more high level code review of this and it looks good!
>
> http://cr.openjdk.java.net/~iklam/jdk14/8231610-relocate-cds-archive.v05/src/hotspot/share/memory/archiveUtils.hpp.html 
>
>
> I think these classes require comments on what they do and why. The 
> comments you sent me offline look good.

I added more comments for ArchivePtrMarker::_compacted per your offline 
request.

>
> Also .hpp files shouldn't include .inline.hpp files, like 
> bitMap.inline.hpp.  Hopefully it's just a case of moving do_bit() into 
> the cpp file.

I moved the do_bit() function into archiveUtils.inline.hpp, since is 
used by 3 .cpp files, and performance is important.

>
> I wonder if the exception list of classes to exclude should be a 
> function in javaClasses.hpp/cpp where the explanation would make more 
> sense?  ie bool 
> JavaClasses::has_injected_native_pointers(InstanceKlass* k);

I moved the checking code to javaClasses.cpp. Since we do (partially) 
support java.lang.Class, which has injected native pointers, I named the 
function as JavaClasses::is_supported_for_archiving instead. I also 
massaged the comments a little for clarification.

>
> Is there already an RFE to move the DumpSharedSpaces output from 
> tty->print() to log_info() ?

I created https://bugs.openjdk.java.net/browse/JDK-8233826 (Change CDS 
dumping tty->print_cr() to unified logging).

Thanks
- Ioi

>
> Thanks,
> Coleen
>
> On 11/6/19 4:17 PM, Ioi Lam wrote:
>> Hi Jiangli,
>>
>> I've uploaded the webrev after integrating your comments:
>>
>> http://cr.openjdk.java.net/~iklam/jdk14/8231610-relocate-cds-archive.v05/ 
>>
>> http://cr.openjdk.java.net/~iklam/jdk14/8231610-relocate-cds-archive.v05-delta/ 
>>
>>
>> Please see more replies below:
>>
>>
>> On 11/4/19 5:52 PM, Jiangli Zhou wrote:
>>> On Sun, Nov 3, 2019 at 10:27 PM Ioi Lam <ioi.lam at oracle.com 
>>> <mailto:ioi.lam at oracle.com>> wrote:
>>>
>>>     Hi Jiangli,
>>>
>>>     Thank you so much for spending time reviewing this RFE!
>>>
>>>     On 11/3/19 6:34 PM, Jiangli Zhou wrote:
>>>     > Hi Ioi,
>>>     >
>>>     > Sorry for the delay again. Will try to put this on the top of my
>>>     list
>>>     > next week and reduce the turn-around time. The updates look 
>>> good in
>>>     > general.
>>>     >
>>>     > We might want to have a better strategy when choosing metadata
>>>     > relocation address (when relocation is needed). Some
>>>     > applications/benchmarks may be more sensitive to cache 
>>> locality and
>>>     > memory/data layout. There was a bug,
>>>     > https://bugs.openjdk.java.net/browse/JDK-8213713 that caused 
>>> 1G gap
>>>     > between Java heap data and metadata before JDK 12. The gap 
>>> seemed to
>>>     > cause a small but noticeable runtime effect in one case that I 
>>> came
>>>     > across.
>>>
>>>     I guess you're saying we should try to relocate the archive into
>>>     somewhere under 32GB?
>>>
>>>
>>> I don't yet have sufficient data that determins if mapping at low 
>>> 32G produces better runtime performance. I experimented with that, 
>>> but didn't see noticeable difference when comparing to mapping at 
>>> the current default address. It doesn't hurt, I think. So it may be 
>>> a better choice than relocating to a random address in high 32G 
>>> space (when Java heap is in low 32G address space).
>>
>> Maybe we should reconsider this when we have more concrete data for 
>> the benefits of moving the compressed class space to under 32G.
>>
>> Please note that in metaspace.cpp, when CDS is disabled and  the VM 
>> fails to allocate the class space at the requested address 
>> (0x7c000000 for 16GB heap), it also just allocates from a random 
>> address (without trying to to search under 32GB):
>>
>> http://hg.openjdk.java.net/jdk/jdk/annotate/e767fa6a1d45/src/hotspot/share/memory/metaspace.cpp#l1128 
>>
>>
>> This code has been there since 2013 and we have not seen any issues.
>>
>>
>>
>>
>>>
>>>     Could you elaborate more about the performance issue, especially
>>>     about
>>>     cache locality? I looked at JDK-8213713 but it didn't mention about
>>>     performance.
>>>
>>>
>>> When enabling CDS we noticed a small runtime overhead in JDK 11 
>>> recently with a benchmark. After I backported JDK-8213713 to 11, it 
>>> seemed to reduce the runtime overhead that the benchmark was 
>>> experiencing.
>>>
>>>
>>>     Also, by default, we have non-zero narrow_klass_base and
>>>     narrow_klass_shift = 3, and archive relocation doesn't change that:
>>>
>>>     $ java -Xlog:cds=debug -version
>>>     ... narrow_klass_base = 0x0000000800000000, narrow_klass_shift = 3
>>>     $ java -Xlog:cds=debug -XX:SharedBaseAddress=0 -version
>>>     ... narrow_klass_base = 0x00007f1e8b499000, narrow_klass_shift = 3
>>>
>>>     We always use narrow_klass_shift due to this:
>>>
>>>        // CDS uses LogKlassAlignmentInBytes for narrow_klass_shift. See
>>>        //
>>> MetaspaceShared::initialize_dumptime_shared_and_meta_spaces() for
>>>        // how dump time narrow_klass_shift is set. Although, CDS can 
>>> work
>>>        // with zero-shift mode also, to be consistent with AOT it uses
>>>        // LogKlassAlignmentInBytes for klass shift so archived java
>>>     heap objects
>>>        // can be used at same time as AOT code.
>>>        if (!UseSharedSpaces
>>>            && (uint64_t)(higher_address - lower_base) <=
>>>     UnscaledClassSpaceMax) {
>>>          CompressedKlassPointers::set_shift(0);
>>>        } else {
>>> CompressedKlassPointers::set_shift(LogKlassAlignmentInBytes);
>>>        }
>>>
>>>
>>> Right. If we relocate to low 32G space, it needs to make sure that 
>>> the range containing the mapped class data and class space must be 
>>> encodable.
>>>
>>>
>>>     > Here are some additional comments (minor).
>>>     >
>>>     > Could you please fix the long lines in the following?
>>>     >
>>>     > 1237 void
>>> java_lang_Class::update_archived_primitive_mirror_native_pointers(oop
>>>     > archived_mirror) {
>>>     > 1238   if (MetaspaceShared::relocation_delta() != 0) {
>>>     > 1239  assert(archived_mirror->metadata_field(_klass_offset) ==
>>>     > NULL, "must be for primitive class");
>>>     > 1240
>>>     > 1241     Klass* ak =
>>>     > ((Klass*)archived_mirror->metadata_field(_array_klass_offset));
>>>     > 1242     if (ak != NULL) {
>>>     > 1243  archived_mirror->metadata_field_put(_array_klass_offset,
>>>     > (Klass*)(address(ak) + MetaspaceShared::relocation_delta()));
>>>     > 1244     }
>>>     > 1245   }
>>>     > 1246 }
>>>     >
>>>     > src/hotspot/share/memory/dynamicArchive.cpp
>>>     >
>>>     >   889   Thread* THREAD = Thread::current();
>>>     >   890   Method::sort_methods(ik->methods(), /*set_idnums=*/true,
>>>     > dynamic_dump_method_comparator);
>>>     >   891   if (ik->default_methods() != NULL) {
>>>     >   892  Method::sort_methods(ik->default_methods(),
>>>     > /*set_idnums=*/false, dynamic_dump_method_comparator);
>>>     >   893   }
>>>     >
>>>
>>>     OK will do.
>>>
>>>     > Please see inlined comments below.
>>>     >
>>>     > On Mon, Oct 28, 2019 at 9:05 PM Ioi Lam <ioi.lam at oracle.com
>>>     <mailto:ioi.lam at oracle.com>> wrote:
>>>     >> Hi Jiangli,
>>>     >>
>>>     >> Thanks for the review. I've updated the patch according to your
>>>     comments:
>>>     >>
>>>     >>
>>> http://cr.openjdk.java.net/~iklam/jdk14/8231610-relocate-cds-archive.v04/ 
>>>
>>>     >>
>>> http://cr.openjdk.java.net/~iklam/jdk14/8231610-relocate-cds-archive.v04.delta/ 
>>>
>>>     >>
>>>     >> (the delta is on top of 8231610-relocate-cds-archive.v03.delta
>>>     in my
>>>     >> reply to Calvin's comments).
>>>     >>
>>>     >> On 10/27/19 9:13 PM, Jiangli Zhou wrote:
>>>     >>> Hi Ioi,
>>>     >>>
>>>     >>> Sorry for the delay. Here are my remaining comments.
>>>     >>>
>>>     >>> - src/hotspot/share/memory/dynamicArchive.cpp
>>>     >>>
>>>     >>> 128   static intx _method_comparator_name_delta;
>>>     >>>
>>>     >>> The name of the above variable is confusing. It's the value of
>>>     >>> _buffer_to_target_delta. It's better to _buffer_to_target_delta
>>>     >>> directly.
>>>     >> _buffer_to_target_delta is a non-static field, but
>>>     >> dynamic_dump_method_comparator() must be a static function so
>>>     it can't
>>>     >> use the non-static field easily.
>>>     >
>>>     > It sounds like an issue. _buffer_to_target_delta was made as a
>>>     > non-static mostly because we might support more than one dynamic
>>>     > archives in the future. However, today's usages bake in an
>>>     assumption
>>>     > that _buffer_to_target_delta is a singleton value. It is 
>>> cleaner to
>>>     > either make _buffer_to_target_delta as a static variable for 
>>> now, or
>>>     > adding an access API in DynamicArchiveBuilder to allow other 
>>> code to
>>>     > properly and correctly use the value.
>>>
>>>     OK, I'll move it to a static variable.
>>>
>>>     >
>>>     >>> Also, we can do a quick pointer comparison of 'a_name' and
>>>     >>> 'b_name' first before adjusting the pointers.
>>>     >> I added this:
>>>     >>
>>>     >>       if (a_name == b_name) {
>>>     >>         return 0;
>>>     >>       }
>>>     >>
>>>     >>> ---
>>>     >>>
>>>     >>> 934 void DynamicArchiveBuilder::relocate_buffer_to_target() {
>>>     >>> ...
>>>     >>>    944
>>>     >>>    945  ArchivePtrMarker::compact(relocatable_base,
>>>     relocatable_end);
>>>     >>> ...
>>>     >>>
>>>     >>>    974     SharedDataRelocator patcher((address*)patch_base,
>>>     >>> (address*)patch_end, valid_old_base, valid_old_end,
>>>     >>>    975  valid_new_base, valid_new_end, addr_delta);
>>>     >>>    976  ArchivePtrMarker::ptrmap()->iterate(&patcher);
>>>     >>>
>>>     >>> Could we reduce the number of data re-iterations to help 
>>> archive
>>>     >>> dumping performance. The ArchivePtrMarker::compact operation
>>>     can be
>>>     >>> combined with the patching iteration.
>>>     ArchivePtrMarker::compact API
>>>     >>> can be removed.
>>>     >> That's a good idea. I implemented it using a template parameter
>>>     so that
>>>     >> we can have max performance when relocating the archive at run
>>>     time.
>>>     >>
>>>     >> I added comments to explain why the relocation is done here. The
>>>     >> relocation is pretty rare (only when the base archive was not
>>>     mapped at
>>>     >> the default location).
>>>     >>
>>>     >>> ---
>>>     >>>
>>>     >>>    967     address valid_new_base =
>>>     >>> (address)Arguments::default_SharedBaseAddress();
>>>     >>>    968     address valid_new_end  = valid_new_base +
>>>     base_plus_top_size;
>>>     >>>
>>>     >>> The debugging only code can be included under #ifdef ASSERT.
>>>     >> These values are actually also used in debug logging so they
>>>     can't be
>>>     >> ifdef'ed out.
>>>     >>
>>>     >> Also, the c++ compiler is pretty good with eliding code 
>>> that's no
>>>     >> actually used. If I comment out all the logging code in
>>>     >> DynamicArchiveBuilder::relocate_buffer_to_target() and
>>>     >> SharedDataRelocator, gcc elides all the unused fields and their
>>>     >> assignments. So no code is generated for this, etc.
>>>     >>
>>>     >>       address valid_new_base =
>>>     >> (address)Arguments::default_SharedBaseAddress();
>>>     >>
>>>     >> Since #ifdef ASSERT makes the code harder to read, I think we
>>>     should use
>>>     >> it only when really necessary.
>>>     > It seems cleaner to get rid of these debugging only variables, by
>>>     > using 'relocatable_base' and
>>>     > '(address)Arguments::default_SharedBaseAddress()' in the logging
>>>     code.
>>>
>>>     SharedDataRelocator is used under 3 different situations. These six
>>>     variables (patch_base, patch_end, valid_old_base, valid_old_end,
>>>     valid_new_base, valid_new_end) describes what is being patched,
>>>     and what
>>>     the expectations are, for each situation. The code will be hard to
>>>     understand without them.
>>>
>>>     Please note there's also logging code in the SharedDataRelocator
>>>     constructor that prints out these values.
>>>
>>>     I think I'll just remove the 'debug only' comment to avoid 
>>> confusion.
>>>
>>>
>>> Ok.
>>>
>>>
>>>     >
>>>     >>> ---
>>>     >>>
>>>     >>>    993
>>>  dynamic_info->write_bitmap_region(ArchivePtrMarker::ptrmap());
>>>     >>>
>>>     >>> We could combine the archived heap data bitmap into the new
>>>     region as
>>>     >>> well? It can be handled as a separate RFE.
>>>     >> I've filed https://bugs.openjdk.java.net/browse/JDK-8233093
>>>     >>
>>>     >>> - src/hotspot/share/memory/filemap.cpp
>>>     >>>
>>>     >>> 1038     if (is_static()) {
>>>     >>> 1039       if (errno == ENOENT) {
>>>     >>> 1040         // Not locating the shared archive is ok.
>>>     >>> 1041         fail_continue("Specified shared archive not found
>>>     (%s).",
>>>     >>> _full_path);
>>>     >>> 1042       } else {
>>>     >>> 1043         fail_continue("Failed to open shared archive file
>>>     (%s).",
>>>     >>> 1044  os::strerror(errno));
>>>     >>> 1045       }
>>>     >>> 1046     } else {
>>>     >>> 1047       log_warning(cds, dynamic)("specified dynamic archive
>>>     >>> doesn't exist: %s", _full_path);
>>>     >>> 1048     }
>>>     >>>
>>>     >>> If the top layer is explicitly specified by the user, a
>>>     warning does
>>>     >>> not seem to be a proper behavior if the VM fails to open the
>>>     archive
>>>     >>> file.
>>>     >>>
>>>     >>> If might be better to handle the relocation unrelated code in
>>>     separate
>>>     >>> changeset and track with a separate RFE.
>>>     >> This code was moved from
>>>     >>
>>>     >>
>>> http://hg.openjdk.java.net/jdk/jdk/file/d3382812b788/src/hotspot/share/memory/dynamicArchive.cpp#l1070 
>>>
>>>     >>
>>>     >> so I am not changing the behavior. If you want, we can file an
>>>     REF to
>>>     >> change the behavior.
>>>     > Ok. A new RFE sounds like the right thing to re-evaluable the 
>>> usage
>>>     > issue here. Thanks.
>>>
>>>     I created https://bugs.openjdk.java.net/browse/JDK-8233446
>>>
>>>     >>> ---
>>>     >>>
>>>     >>> 1148 void FileMapInfo::write_region(int region, char* base,
>>>     size_t size,
>>>     >>> 1149                                bool read_only, bool
>>>     allow_exec) {
>>>     >>> ...
>>>     >>> 1154
>>>     >>> 1155   if (region == MetaspaceShared::bm) {
>>>     >>> 1156     target_base = NULL;
>>>     >>> 1157   } else if (DynamicDumpSharedSpaces) {
>>>     >>>
>>>     >>> It's not too clear to me how the bitmap (bm) region is handled
>>>     for the
>>>     >>> base layer and top layer. Could you please explain?
>>>     >> The bm region for both layers are mapped at an address picked
>>>     by the OS:
>>>     >>
>>>     >> char* FileMapInfo::map_relocation_bitmap(size_t& bitmap_size) {
>>>     >>     FileMapRegion* si = space_at(MetaspaceShared::bm);
>>>     >>     bitmap_size = si->used_aligned();
>>>     >>     bool read_only = true, allow_exec = false;
>>>     >>     char* requested_addr = NULL; // allow OS to pick any 
>>> location
>>>     >>     char* bitmap_base = os::map_memory(_fd, _full_path,
>>>     si->file_offset(),
>>>     >> requested_addr, bitmap_size,
>>>     >> read_only, allow_exec);
>>>     >>
>>>     > Ok, after staring at the code for a few seconds I saw that's
>>>     intended.
>>>     > If the current region is 'bm', then the 'target_base' is NULL
>>>     > regardless if it's static or dynamic archive. Otherwise, the
>>>     > 'target_base' is handled differently for the static and dynamic
>>>     case.
>>>     > The following would be cleaner and has better reliability.
>>>     >
>>>     >     char* target_base = NULL;
>>>     >
>>>     >     // The target_base is NULL for 'bm' region.
>>>     >     if (!region == MetaspaceShared::bm) {
>>>     >       if (DynamicDumpSharedSpaces) {
>>>     >         assert(!HeapShared::is_heap_region(region), "dynamic 
>>> archive
>>>     > doesn't support heap regions");
>>>     >         target_base = DynamicArchive::buffer_to_target(base);
>>>     >       } else {
>>>     >         target_base = base;
>>>     >       }
>>>     >    }
>>>
>>>     How about this?
>>>
>>>        char* target_base;
>>>        if (region == MetaspaceShared::bm) {
>>>          target_base = NULL; // always NULL for bm region.
>>>        } else {
>>>          if (DynamicDumpSharedSpaces) {
>>>              assert(!HeapShared::is_heap_region(region), "dynamic 
>>> archive
>>>     doesn't support heap regions");
>>>              target_base = DynamicArchive::buffer_to_target(base);
>>>          } else {
>>>              target_base = base;
>>>          }
>>>        }
>>>
>>>
>>> No objection If you prefer the extra 'else' block.
>>>
>>>
>>>     >
>>>     >>> ---
>>>     >>>
>>>     >>> 1362
>>>  DEBUG_ONLY(header()->set_mapped_base_address((char*)(uintptr_t)0xdeadbeef);) 
>>>
>>>     >>>
>>>     >>> Could you please explain the above?
>>>     >> I added the comments
>>>     >>
>>>     >>     // Make sure we don't attempt to use
>>>     header()->mapped_base_address()
>>>     >> unless
>>>     >>     // it's been successfully mapped.
>>>     >>
>>> DEBUG_ONLY(header()->set_mapped_base_address((char*)(uintptr_t)0xdeadbeef);) 
>>>
>>>     >>
>>>     >>> ---
>>>     >>>
>>>     >>> 1359   FileMapRegion* last_region = NULL;
>>>     >>>
>>>     >>> 1371     if (last_region != NULL) {
>>>     >>> 1372       // Ensure that the OS won't be able to allocate new
>>>     memory
>>>     >>> spaces between any mapped
>>>     >>> 1373       // regions, or else it would mess up the simple
>>>     comparision
>>>     >>> in MetaspaceObj::is_shared().
>>>     >>> 1374       assert(si->mapped_base() == 
>>> last_region->mapped_end(),
>>>     >>> "must have no gaps");
>>>     >>>
>>>     >>> 1379     last_region = si;
>>>     >>>
>>>     >>> Can you please place 'last_region' related code under #ifdef
>>>     ASSERT?
>>>     >> I think that will make the code more cluttered. The compiler 
>>> will
>>>     >> optimize out that away.
>>>     > It's cleaner to define debugging only variable for debugging only
>>>     > builds. You can wrapper it and related usage with DEBUG_ONLY.
>>>
>>>     OK, will do.
>>>
>>>     >
>>>     >>> ---
>>>     >>>
>>>     >>> 1478 char* FileMapInfo::map_relocation_bitmap(size_t&
>>>     bitmap_size) {
>>>     >>> 1479   FileMapRegion* si = space_at(MetaspaceShared::bm);
>>>     >>> 1480   bitmap_size = si->used_aligned();
>>>     >>> 1481   bool read_only = true, allow_exec = false;
>>>     >>> 1482   char* requested_addr = NULL; // allow OS to pick any
>>>     location
>>>     >>> 1483   char* bitmap_base = os::map_memory(_fd, _full_path,
>>>     si->file_offset(),
>>>     >>> 1484 requested_addr, bitmap_size,
>>>     >>> read_only, allow_exec);
>>>     >>>
>>>     >>> We need to handle mapping failure here.
>>>     >> It's handled here:
>>>     >>
>>>     >> bool FileMapInfo::relocate_pointers(intx addr_delta) {
>>>     >>     log_debug(cds, reloc)("runtime archive relocation start");
>>>     >>     size_t bitmap_size;
>>>     >>     char* bitmap_base = map_relocation_bitmap(bitmap_size);
>>>     >>     if (bitmap_base != NULL) {
>>>     >>     ...
>>>     >>     } else {
>>>     >>       log_error(cds)("failed to map relocation bitmap");
>>>     >>       return false;
>>>     >>     }
>>>     >>
>>>     > 'bitmap_base' is used immediately after map_memory(). So the 
>>> check
>>>     > needs to be done immediately after map_memory(), but not in the
>>>     caller
>>>     > of map_relocation_bitmap().
>>>     >
>>>     > 1490   char* bitmap_base = os::map_memory(_fd, _full_path,
>>>     si->file_offset(),
>>>     > 1491 requested_addr, bitmap_size,
>>>     > read_only, allow_exec);
>>>     > 1492
>>>     > 1493   if (VerifySharedSpaces && bitmap_base != NULL &&
>>>     > !region_crc_check(bitmap_base, bitmap_size, si->crc())) {
>>>
>>>     OK, I'll fix that.
>>>
>>>     >
>>>     >
>>>     >>> ---
>>>     >>>
>>>     >>> 1513     // debug only -- the current value of the pointers 
>>> to be
>>>     >>> patched must be within this
>>>     >>> 1514     // range (i.e., must be between the requesed base
>>>     address,
>>>     >>> and the of the current archive).
>>>     >>> 1515     // Note: top archive may point to objects in the base
>>>     >>> archive, but not the other way around.
>>>     >>> 1516     address valid_old_base =
>>>     (address)header()->requested_base_address();
>>>     >>> 1517     address valid_old_end  = valid_old_base +
>>>     mapping_end_offset();
>>>     >>>
>>>     >>> Please place all FileMapInfo::relocate_pointers debugging only
>>>     code
>>>     >>> under #ifdef ASSERT.
>>>     >> Ditto about ifdef ASSERT
>>>     >>
>>>     >>> - src/hotspot/share/memory/heapShared.cpp
>>>     >>>
>>>     >>>    441 void
>>>     HeapShared::initialize_from_archived_subgraph(Klass* k) {
>>>     >>>    442   if (!open_archive_heap_region_mapped() ||
>>>     !MetaspaceObj::is_shared(k)) {
>>>     >>>    443     return; // nothing to do
>>>     >>>    444   }
>>>     >>>
>>>     >>> When do we call HeapShared::initialize_from_archived_subgraph
>>>     for a
>>>     >>> klass that's not shared?
>>>     >> I've removed the !MetaspaceObj::is_shared(k). I probably added
>>>     that for
>>>     >> debugging purposes only.
>>>     >>
>>>     >>>    616   DEBUG_ONLY({
>>>     >>>    617       Klass* klass = orig_obj->klass();
>>>     >>>    618       assert(klass != 
>>> SystemDictionary::Module_klass() &&
>>>     >>>    619              klass !=
>>>     SystemDictionary::ResolvedMethodName_klass() &&
>>>     >>>    620              klass !=
>>>     SystemDictionary::MemberName_klass() &&
>>>     >>>    621              klass != 
>>> SystemDictionary::Context_klass() &&
>>>     >>>    622              klass !=
>>>     SystemDictionary::ClassLoader_klass(), "we
>>>     >>> can only relocate metaspace object pointers inside 
>>> java_lang_Class
>>>     >>> instances");
>>>     >>>    623     });
>>>     >>>
>>>     >>> Let's leave the above for a separate RFE. I think assert is not
>>>     >>> sufficient for the check. Also, why ResolvedMethodName, 
>>> Module and
>>>     >>> MemberName cannot be part of the graph?
>>>     >>>
>>>     >>>
>>>     >> I added the following comment:
>>>     >>
>>>     >>     DEBUG_ONLY({
>>>     >>         // The following are classes in
>>>     share/classfile/javaClasses.cpp
>>>     >> that have injected native pointers
>>>     >>         // to metaspace objects. To support these classes, we
>>>     need to add
>>>     >> relocation code similar to
>>>     >>         // 
>>> java_lang_Class::update_archived_mirror_native_pointers.
>>>     >>         Klass* klass = orig_obj->klass();
>>>     >>         assert(klass != SystemDictionary::Module_klass() &&
>>>     >>                klass !=
>>>     SystemDictionary::ResolvedMethodName_klass() &&
>>>     >>
>>>     > It's too restrictive to exclude those objects from the archived
>>>     object
>>>     > graph because metadata relocation, since metadata relocation is
>>>     rare.
>>>     > The trade-off doesn't seem to buy us much.
>>>     >
>>>     > Do you plan to add the needed relocation code?
>>>
>>>     I looked more into this. Actually we cannot handle these 5 
>>> classes at
>>>     all, even without archive relocation:
>>>
>>>     [1] #define MODULE_INJECTED_FIELDS(macro) \
>>>        macro(java_lang_Module, module_entry, intptr_signature, false)
>>>
>>>     ->  module_entry is malloc'ed
>>>
>>>     [2] #define RESOLVEDMETHOD_INJECTED_FIELDS(macro) \
>>>        macro(java_lang_invoke_ResolvedMethodName, vmholder,
>>>     object_signature, false) \
>>>        macro(java_lang_invoke_ResolvedMethodName, vmtarget,
>>>     intptr_signature, false)
>>>
>>>     -> these fields are related to method handles and lambda forms, 
>>> etc.
>>>     They can't be easily be archived without implementing lambda form
>>>     archiving. (I did a prototype; it's very complex and fragile).
>>>
>>>     [3] #define CALLSITECONTEXT_INJECTED_FIELDS(macro) \
>>> macro(java_lang_invoke_MethodHandleNatives_CallSiteContext,
>>>     vmdependencies, intptr_signature, false) \
>>> macro(java_lang_invoke_MethodHandleNatives_CallSiteContext,
>>>     last_cleanup, long_signature, false)
>>>
>>>     -> vmdependencies is malloc'ed.
>>>
>>>     [4] #define
>>> MEMBERNAME_INJECTED_FIELDS(macro) \
>>>        macro(java_lang_invoke_MemberName, vmindex, intptr_signature,
>>>     false)
>>>
>>>     -> this one is probably OK. Despite being declared as
>>>     'intptr_signature', it seems to be used just as an integer. 
>>> However,
>>>     MemberNames are typically used with [2] and [3]. So let's just
>>>     forbid it
>>>     to be safe.
>>>
>>>     [2] [3] [4] are not used directly by regular Java code and are
>>>     unlikely
>>>     to be referenced (directly or indirectly) by static fields (except
>>>     for
>>>     the static fields in the classes in java.lang.invoke, which we
>>>     probably
>>>     won't support for heap archiving due to the problem I described for
>>>     [2]). Objects of these types are typically referenced via constant
>>>     pool
>>>     entries.
>>>
>>>     [5] #define CLASSLOADER_INJECTED_FIELDS(macro) \
>>>        macro(java_lang_ClassLoader, loader_data, intptr_signature, 
>>> false)
>>>
>>>     -> loader_data is malloc'ed.
>>>
>>>     So, I will change the DEBUG_ONLY into a product-mode check, and 
>>> quit
>>>     dumping if these objects are found in the object subgraph.
>>>
>>>
>>> Sounds good. Can you please also add a comment with explanation.
>>>
>>> For  ClassLoader and Module, it worth considering caching the 
>>> additional native data some time in the future. Lois had suggested 
>>> the Module part a while ago.
>>
>> I think we can do that if/when we archive Modules directly into the 
>> shared heap.
>>
>>
>>
>>>
>>>
>>>
>>>
>>>
>>>     Maybe we should backport the check to older versions as well?
>>>
>>>
>>> We should discuss with Andrew Haley for backports to JDK 11 update 
>>> releases. Since the current OpenJDK 11 only applies Java heap 
>>> archiving to a restricted set of JDK library code, I think it is 
>>> safe without the new check.
>>>
>>> For non-LTS releases, it might not be worthwhile as they may not be 
>>> widely used?
>>
>> I agree. FYI, we (Oracle) have no plan for backporting more types of 
>> heap object archiving, so the decision would be up to whoever that 
>> decides to do so.
>>
>> Thanks
>> - Ioi
>>
>>
>>>
>>> Thanks,
>>> Jiangli
>>>
>>>
>>>     >
>>>     >>> - src/hotspot/share/memory/metaspace.cpp
>>>     >>>
>>>     >>> 1036   metaspace_rs = 
>>> ReservedSpace(compressed_class_space_size(),
>>>     >>> 1037   _reserve_alignment,
>>>     >>> 1038   large_pages,
>>>     >>> 1039   requested_addr);
>>>     >>>
>>>     >>> Please fix indentation.
>>>     >> Fixed.
>>>     >>
>>>     >>> - src/hotspot/share/memory/metaspaceClosure.hpp
>>>     >>>
>>>     >>>     78   enum SpecialRef {
>>>     >>>     79     _method_entry_ref
>>>     >>>     80   };
>>>     >>>
>>>     >>> Are there other pointers that are not references to
>>>     MetaspaceObj? If
>>>     >>> _method_entry_ref is the only type, it's probably not worth
>>>     defining
>>>     >>> SpecialRef?
>>>     >> There may be more types in the future, so I want to have a
>>>     stable API
>>>     >> that can be easily expanded without touching all the code that
>>>     uses it.
>>>     >>
>>>     >>
>>>     >>> - src/hotspot/share/memory/metaspaceShared.hpp
>>>     >>>
>>>     >>>     42 enum MapArchiveResult {
>>>     >>>     43   MAP_ARCHIVE_SUCCESS,
>>>     >>>     44   MAP_ARCHIVE_MMAP_FAILURE,
>>>     >>>     45   MAP_ARCHIVE_OTHER_FAILURE
>>>     >>>     46 };
>>>     >>>
>>>     >>> If we want to define different failure types, it's probably 
>>> worth
>>>     >>> using separate types for relocation failure and validation
>>>     failure.
>>>     >> For now, I just need to distinguish between MMAP_FAILURE (where
>>>     I should
>>>     >> attempt to remap at an alternative address) and OTHER_FAILURE
>>>     (where the
>>>     >> CDS archive loading will fail -- due to validation error,
>>>     insufficient
>>>     >> memory, etc -- without attempting to remap.)
>>>     >>
>>>     >>> ---
>>>     >>>
>>>     >>>    193   static intx _mapping_delta; // FIXME rename
>>>     >>>
>>>     >>> How about _relocation_delta?
>>>     >> Changed as suggested.
>>>     >>
>>>     >>> - src/hotspot/share/oops/instanceKlass
>>>     >>>
>>>     >>> 1573 bool InstanceKlass::_disable_method_binary_search = false;
>>>     >>>
>>>     >>> The use of _disable_method_binary_search is not necessary. You
>>>     can use
>>>     >>> DynamicDumpSharedSpaces for the purpose. That would make things
>>>     >>> cleaner.
>>>     >> If we always disable the binary search when
>>>     DynamicDumpSharedSpaces is
>>>     >> true, it will slow down normal execution of the Java program 
>>> when
>>>     >> -XX:ArchiveClassesAtExit has been specified, but the program
>>>     hasn't exited.
>>>     > Could you please add some comments to 
>>> _disable_method_binary_search
>>>     > with the above explanation? Thanks.
>>>
>>>     OK
>>>     >
>>>     >>> - test/hotspot/jtreg/runtime/cds/SpaceUtilizationCheck.java
>>>     >>>
>>>     >>>     76                     if (name.equals("s0") ||
>>>     name.equals("s1")) {
>>>     >>>     77                       // String regions are listed at
>>>     the end and
>>>     >>> they may not be fully occupied.
>>>     >>>     78                       break;
>>>     >>>     79                     } else if (name.equals("bm")) {
>>>     >>>     80                       // Bitmap space does not have a
>>>     requested address.
>>>     >>>     81                       break;
>>>     >>>
>>>     >>> It's not part of your change, but could you please fix line 76
>>>     - 78
>>>     >>> since it is trivial. It seems the lines can be removed.
>>>     >> Removed.
>>>     >>
>>>     >>> - /src/hotspot/share/memory/archiveUtils.hpp
>>>     >>> The file name does not match with the macro '#ifndef
>>>     >>> SHARE_MEMORY_SHAREDDATARELOCATOR_HPP'. Could you please rename
>>>     >>> archiveUtils.* ? archiveRelocator.hpp and 
>>> archiveRelocator.cpp are
>>>     >>> more descriptive.
>>>     >> I named the file archiveUtils.hpp so we can move other misc
>>>     stuff used
>>>     >> by dumping into this file (e.g., DumpRegion, WriteClosure from
>>>     >> metaspaceShared.hpp), since theses are not used by the majority
>>>     of the
>>>     >> files that use metaspaceShared.hpp.
>>>     >>
>>>     >> I fixed the ifdef.
>>>     >>
>>>     >>> - src/hotspot/share/memory/archiveUtils.cpp
>>>     >>>
>>>     >>>     36 void ArchivePtrMarker::initialize(CHeapBitMap* ptrmap,
>>>     address*
>>>     >>> ptr_base, address* ptr_end) {
>>>     >>>     37   assert(_ptrmap == NULL, "initialize only once");
>>>     >>>     38   _ptr_base = ptr_base;
>>>     >>>     39   _ptr_end = ptr_end;
>>>     >>>     40   _compacted = false;
>>>     >>>     41   _ptrmap = ptrmap;
>>>     >>>     42   _ptrmap->initialize(12 * M / sizeof(intptr_t)); //
>>>     default
>>>     >>> archive is about 12MB.
>>>     >>>     43 }
>>>     >>>
>>>     >>> Could we do a better estimate here? We could guesstimate the 
>>> size
>>>     >>> based on the current used class space and metaspace size. It's
>>>     okay if
>>>     >>> a larger bitmap used, since it can be reduced after all
>>>     marking are
>>>     >>> done.
>>>     >> The bitmap is automatically expanded when necessary in
>>>     >> ArchivePtrMarker::mark_pointer(). It's only about 1/32 or 1/64
>>>     of the
>>>     >> total archive size, so even if we do expand, the cost will be
>>>     trivial.
>>>     > The initial value is based on the default CDS archive. When 
>>> dealing
>>>     > with a really large archive, it would have to re-grow many times.
>>>     > Also, using a hard-coded value is less desirable.
>>>
>>>     OK, I changed it to the following
>>>
>>>        // Use this as initial guesstimate. We should need less space
>>>     in the
>>>        // archive, but if we're wrong the bitmap will be expanded
>>>     automatically.
>>>        size_t estimated_archive_size = 
>>> MetaspaceGC::capacity_until_GC();
>>>        // But set it smaller in debug builds so we always test the
>>>     expansion
>>>     code.
>>>        // (Default archive is about 12MB).
>>>        DEBUG_ONLY(estimated_archive_size = 6 * M);
>>>
>>>        // We need one bit per pointer in the archive.
>>>        _ptrmap->initialize(estimated_archive_size / sizeof(intptr_t));
>>>
>>>
>>>     Thanks!
>>>     - Ioi
>>>
>>>     >
>>>     >>>
>>>     >>>
>>>     >>> On Wed, Oct 16, 2019 at 4:58 PM Jiangli Zhou
>>>     <jianglizhou at google.com <mailto:jianglizhou at google.com>> wrote:
>>>     >>>> Hi Ioi,
>>>     >>>>
>>>     >>>> This is another great step for CDS usability improvement.
>>>     Thank you!
>>>     >>>>
>>>     >>>> I have a high level question (or request): could we consider
>>>     >>>> separating the relocation work for 'direct' class metadata
>>>     from other
>>>     >>>> types of metadata (such as the shared system dictionary,
>>>     symbol table,
>>>     >>>> etc)? Initially we only relocate the tables and other
>>>     archived global
>>>     >>>> data. When each archived class is being loaded, we can
>>>     relocate all
>>>     >>>> the pointers within the current class. We could find the
>>>     segment (for
>>>     >>>> the current class) in the bitmap and update the pointers
>>>     within the
>>>     >>>> segment. That way we can reduce initial startup costs and
>>>     also avoid
>>>     >>>> relocating class data that's not used at runtime. In some
>>>     real world
>>>     >>>> large systems, an archive may contain extremely large 
>>> number of
>>>     >>>> classes.
>>>     >>>>
>>>     >>>> Following are partial review comments so we can move things
>>>     forward.
>>>     >>>> Still going through the rest of the changes.
>>>     >>>>
>>>     >>>> - src/hotspot/share/classfile/javaClasses.cpp
>>>     >>>>
>>>     >>>> 1218 void
>>> java_lang_Class::update_archived_mirror_native_pointers(oop
>>>     >>>> archived_mirror) {
>>>     >>>> 1219   Klass* k =
>>> ((Klass*)archived_mirror->metadata_field(_klass_offset));
>>>     >>>> 1220   if (k != NULL) { // k is NULL for the primitive
>>>     classes such as
>>>     >>>> java.lang.Byte::TYPE <<<<<<<<<<<
>>>     >>>> 1221  archived_mirror->metadata_field_put(_klass_offset,
>>>     >>>> (Klass*)(address(k) + MetaspaceShared::mapping_delta()));
>>>     >>>> 1222   }
>>>     >>>> 1223 ...
>>>     >>>>
>>>     >>>> Primitive type mirrors are handled separately. Could you
>>>     please verify
>>>     >>>> if this call path happens for primitive type mirror?
>>>     >>>>
>>>     >>>> To answer my question above, looks like you added the
>>>     following, which
>>>     >>>> is to be used for primitive type mirrors. That seems to be
>>>     the reason
>>>     >>>> why update_archived_mirror_native_pointers is trying to also
>>>     cover
>>>     >>>> primitive type. It better to have a separate API for
>>>     primitive type
>>>     >>>> mirror, which is cleaner. And, we also can replace the above
>>>     check at
>>>     >>>> line 1220 to be an assert for regular mirrors.
>>>     >>>>
>>>     >>>> +void ReadClosure::do_mirror_oop(oop *p) {
>>>     >>>> +  do_oop(p);
>>>     >>>> +  oop mirror = *p;
>>>     >>>> +  if (mirror != NULL) {
>>>     >>>> +
>>> java_lang_Class::update_archived_mirror_native_pointers(mirror);
>>>     >>>> +  }
>>>     >>>> +}
>>>     >>>> +
>>>     >>>>
>>>     >>>> How about renaming update_archived_mirror_native_pointers to
>>>     >>>> update_archived_mirror_klass_pointers.
>>>     >>>>
>>>     >>>> It would be good to pass the current klass as an argument. 
>>> We can
>>>     >>>> verify the relocated pointer matches with the current klass
>>>     pointer.
>>>     >>>>
>>>     >>>> We should also check if relocation is necessary before
>>>     spending cycles
>>>     >>>> to obtain the klass pointer from the mirror.
>>>     >>>>
>>>     >>>> 1252  update_archived_mirror_native_pointers(m);
>>>     >>>> 1253
>>>     >>>> 1254   // mirror is archived, restore
>>>     >>>> 1255  assert(HeapShared::is_archived_object(m), "must be 
>>> archived
>>>     >>>> mirror object");
>>>     >>>> 1256   Handle mirror(THREAD, m);
>>>     >>>>
>>>     >>>> Could we move the line at 1252 after the assert at line 1255?
>>>     >>>>
>>>     >>>> - src/hotspot/share/include/cds.h
>>>     >>>>
>>>     >>>>     47   int     _mapped_from_file;  // Is this region mapped
>>>     from a file?
>>>     >>>>     48                               // If false, this 
>>> region was
>>>     >>>> initialized using os::read().
>>>     >>>>
>>>     >>>> Is the new field truly needed? It seems we could use
>>>     _mapped_base to
>>>     >>>> determine if a region is mapped or not?
>>>     >>>>
>>>     >>>> - src/hotspot/share/memory/dynamicArchive.cpp
>>>     >>>>
>>>     >>>> Could you please remove the debugging print code in
>>>     >>>> dynamic_dump_method_comparator? Or convert those to logging
>>>     output if
>>>     >>>> they are helpful.
>>>     >>>>
>>>     >>>> Will send out the rest of the review comments later.
>>>     >>>>
>>>     >>>> Best,
>>>     >>>>
>>>     >>>> Jiangli
>>>     >>>>
>>>     >>>>
>>>     >>>>
>>>     >>>>
>>>     >>>> On Thu, Oct 10, 2019 at 6:00 PM Ioi Lam <ioi.lam at oracle.com
>>>     <mailto:ioi.lam at oracle.com>> wrote:
>>>     >>>>> Bug:
>>>     >>>>> https://bugs.openjdk.java.net/browse/JDK-8231610
>>>     >>>>>
>>>     >>>>> Webrev:
>>>     >>>>>
>>> http://cr.openjdk.java.net/~iklam/jdk14/8231610-relocate-cds-archive.v01/ 
>>>
>>>     >>>>>
>>>     >>>>> Design:
>>>     >>>>>
>>> http://cr.openjdk.java.net/~iklam/jdk14/design/8231610-relocate-cds-archive.txt 
>>>
>>>     >>>>>
>>>     >>>>>
>>>     >>>>> Overview:
>>>     >>>>>
>>>     >>>>> The CDS archive is mmaped to a fixed address range 
>>> (starting at
>>>     >>>>> SharedBaseAddress, usually 0x800000000). Previously, if this
>>>     >>>>> requested address range is not available (usually due to 
>>> Address
>>>     >>>>> Space Layout Randomization (ASLR) [2]), the JVM will give 
>>> up and
>>>     >>>>> will load classes dynamically using class files.
>>>     >>>>>
>>>     >>>>> [a] This causes slow down in JVM start-up.
>>>     >>>>> [b] Handling of mapping failures causes unnecessary
>>>     complication in
>>>     >>>>>        the CDS tests.
>>>     >>>>>
>>>     >>>>> Here are some preliminary benchmarking results (using
>>>     default CDS archive,
>>>     >>>>> running helloworld):
>>>     >>>>>
>>>     >>>>> (a) 47.1ms (CDS enabled, mapped at requested addr)
>>>     >>>>> (b) 53.8ms (CDS enabled, mapped at alternate addr)
>>>     >>>>> (c) 86.2ms (CDS disabled)
>>>     >>>>>
>>>     >>>>> The small degradation in (b) is caused by the relocation of
>>>     >>>>> absolute pointers embedded in the CDS archive. However, it is
>>>     >>>>> still a big improvement over case (c)
>>>     >>>>>
>>>     >>>>> Please see the design doc (link above) for details.
>>>     >>>>>
>>>     >>>>> Thanks
>>>     >>>>> - Ioi
>>>     >>>>>
>>>
>>
>



More information about the hotspot-runtime-dev mailing list