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

Jiangli Zhou jianglizhou at google.com
Mon Oct 28 04:13:50 UTC 2019


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. Also, we can do a quick pointer comparison of 'a_name' and
'b_name' first before adjusting the pointers.

---

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.

---

 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.

---

 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.

- 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.

---

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?

---

1362   DEBUG_ONLY(header()->set_mapped_base_address((char*)(uintptr_t)0xdeadbeef);)

Could you please explain the above?

---

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?

---

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.

---

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.

- 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?

 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?

- 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.

- 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?

- 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.

---

 193   static intx _mapping_delta; // FIXME rename

How about _relocation_delta?

- 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.

- 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.

- /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.

- 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.

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