RFR(L) 8231610 Relocate the CDS archive if it cannot be mapped to the requested address
Jiangli Zhou
jianglizhou at google.com
Tue Nov 5 01:52:08 UTC 2019
On Sun, Nov 3, 2019 at 10:27 PM Ioi Lam <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).
>
> 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> 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.
> 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?
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>
> 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> 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