RFR(L) 8231610 Relocate the CDS archive if it cannot be mapped to the requested address
Jiangli Zhou
jianglizhou at google.com
Fri Nov 8 02:11:16 UTC 2019
I looked both 05.full and 06.delta webrevs. They look good.
I still feel a bit uneasy about the potential runtime impact when data
does get relocated. Long running apps/services may be shy away from
enabling archive at runtime, if there is a detectable overhead even
though it may only occur rarely. As relocation is enabled by default
and users cannot turn it off, disabling with -Xshare:off entirely
would become the only choice. Could you please create a new RFE
(possibly with higher priority) to investigate the potential effect,
or provide an option for users to opt-in relocation with the
command-line switch?
Regards,
Jiangli
On Thu, Nov 7, 2019 at 4:22 PM Ioi Lam <ioi.lam at oracle.com> wrote:
>
> 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