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