RFR(L) 8231610 Relocate the CDS archive if it cannot be mapped to the requested address
Jiangli Zhou
jianglizhou at google.com
Mon Nov 4 02:34:31 UTC 2019
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.
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 }
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.
>
> > 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.
>
> > ---
> >
> > 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.
>
> > ---
> >
> > 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;
}
}
>
> >
> > ---
> >
> > 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.
>
> > ---
> >
> > 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())) {
> > ---
> >
> > 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?
> >
> > - 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.
>
> >
> > - 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.
Thanks,
Jiangli
>
> Thanks
> - Ioi
>
>
> > Thanks,
> > Jiangli
> >
> >
> >
> > 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