8297313: Refactor APIs for calculating address of CDS archive heap regions

Reviewed-by: matsaave, ccheung

src/hotspot/share/cds/archiveHeapLoader.cpp

@@ -42,6 +42,8 @@
 bool ArchiveHeapLoader::_closed_regions_mapped = false;
 bool ArchiveHeapLoader::_open_regions_mapped = false;
 bool ArchiveHeapLoader::_is_loaded = false;
+
+bool    ArchiveHeapLoader::_narrow_oop_base_initialized = false;
 address ArchiveHeapLoader::_narrow_oop_base;
 int     ArchiveHeapLoader::_narrow_oop_shift;
 
@@ -59,10 +61,26 @@ intx ArchiveHeapLoader::_runtime_offset_2 = 0;
 intx ArchiveHeapLoader::_runtime_offset_3 = 0;
 bool ArchiveHeapLoader::_loading_failed = false;
 
-// Support for mapped heap (!UseCompressedOops only)
+// Support for mapped heap.
-ptrdiff_t ArchiveHeapLoader::_runtime_delta = 0;
+bool      ArchiveHeapLoader::_mapped_heap_relocation_initialized = false;
+ptrdiff_t ArchiveHeapLoader::_mapped_heap_delta = 0;
+
+// Every mapped region is offset by _mapped_heap_delta from its requested address.
+// See FileMapInfo::heap_region_requested_address().
+void ArchiveHeapLoader::init_mapped_heap_relocation(ptrdiff_t delta, int dumptime_oop_shift) {
+  assert(!_mapped_heap_relocation_initialized, "only once");
+  if (!UseCompressedOops) {
+    assert(dumptime_oop_shift == 0, "sanity");
+  }
+  assert(can_map(), "sanity");
+  init_narrow_oop_decoding(CompressedOops::base() + delta, dumptime_oop_shift);
+  _mapped_heap_delta = delta;
+  _mapped_heap_relocation_initialized = true;
+}
 
 void ArchiveHeapLoader::init_narrow_oop_decoding(address base, int shift) {
+  assert(!_narrow_oop_base_initialized, "only once");
+  _narrow_oop_base_initialized = true;
   _narrow_oop_base = base;
   _narrow_oop_shift = shift;
 }
@@ -112,7 +130,7 @@ class PatchUncompressedEmbeddedPointers: public BitMapClosure {
     oop* p = _start + offset;
     intptr_t dumptime_oop = (intptr_t)((void*)*p);
     assert(dumptime_oop != 0, "null oops should have been filtered out at dump time");
-    intptr_t runtime_oop = dumptime_oop + ArchiveHeapLoader::runtime_delta();
+    intptr_t runtime_oop = dumptime_oop + ArchiveHeapLoader::mapped_heap_delta();
     RawAccess<IS_NOT_NULL>::oop_store(p, cast_to_oop(runtime_oop));
     return true;
   }
@@ -192,6 +210,10 @@ void ArchiveHeapLoader::init_loaded_heap_relocation(LoadedArchiveHeapRegion* loa
 }
 
 bool ArchiveHeapLoader::can_load() {
+  if (!UseCompressedOops) {
+    // Pointer relocation for uncompressed oops is unimplemented.
+    return false;
+  }
   return Universe::heap()->can_load_archived_objects();
 }
 
@@ -227,13 +249,13 @@ class PatchLoadedRegionPointers: public BitMapClosure {
   }
 
   bool do_bit(size_t offset) {
+    assert(UseCompressedOops, "PatchLoadedRegionPointers for uncompressed oops is unimplemented");
     narrowOop* p = _start + offset;
     narrowOop v = *p;
     assert(!CompressedOops::is_null(v), "null oops should have been filtered out at dump time");
     uintptr_t o = cast_from_oop<uintptr_t>(ArchiveHeapLoader::decode_from_archive(v));
     assert(_base_0 <= o && o < _top, "must be");
 
-
     // We usually have only 2 regions for the default archive. Use template to avoid unnecessary comparisons.
     if (NUM_LOADED_REGIONS > 3 && o >= _base_3) {
       o += _offset_3;
@@ -264,7 +286,7 @@ int ArchiveHeapLoader::init_loaded_regions(FileMapInfo* mapinfo, LoadedArchiveHe
       LoadedArchiveHeapRegion* ri = &loaded_regions[num_loaded_regions++];
       ri->_region_index = i;
       ri->_region_size = r->used();
-      ri->_dumptime_base = (uintptr_t)mapinfo->start_address_as_decoded_from_archive(r);
+      ri->_dumptime_base = (uintptr_t)mapinfo->heap_region_dumptime_address(r);
     }
   }
 
@@ -346,6 +368,7 @@ bool ArchiveHeapLoader::load_regions(FileMapInfo* mapinfo, LoadedArchiveHeapRegi
 }
 
 bool ArchiveHeapLoader::load_heap_regions(FileMapInfo* mapinfo) {
+  assert(UseCompressedOops, "loaded heap for !UseCompressedOops is unimplemented");
   init_narrow_oop_decoding(mapinfo->narrow_oop_base(), mapinfo->narrow_oop_shift());
 
   LoadedArchiveHeapRegion loaded_regions[MetaspaceShared::max_num_heap_regions];
@@ -386,7 +409,8 @@ class VerifyLoadedHeapEmbeddedPointers: public BasicOopIterateClosure {
     }
   }
   virtual void do_oop(oop* p) {
-    ShouldNotReachHere();
+    // Uncompressed oops are not supported by loaded heaps.
+    Unimplemented();
   }
 };
 

src/hotspot/share/cds/archiveHeapLoader.hpp

@@ -73,9 +73,10 @@ public:
     return is_loaded() || is_mapped();
   }
 
-  static ptrdiff_t runtime_delta() {
+  static ptrdiff_t mapped_heap_delta() {
-    assert(!UseCompressedOops, "must be");
+    CDS_JAVA_HEAP_ONLY(assert(!is_loaded(), "must be"));
-    CDS_JAVA_HEAP_ONLY(return _runtime_delta;)
+    CDS_JAVA_HEAP_ONLY(assert(_mapped_heap_relocation_initialized, "must be"));
+    CDS_JAVA_HEAP_ONLY(return _mapped_heap_delta;)
     NOT_CDS_JAVA_HEAP_RETURN_(0L);
   }
 
@@ -102,14 +103,13 @@ public:
   // function instead.
   inline static oop decode_from_archive(narrowOop v) NOT_CDS_JAVA_HEAP_RETURN_(NULL);
 
-  static void init_narrow_oop_decoding(address base, int shift) NOT_CDS_JAVA_HEAP_RETURN;
-
   static void patch_embedded_pointers(MemRegion region, address oopmap,
                                       size_t oopmap_in_bits) NOT_CDS_JAVA_HEAP_RETURN;
 
   static void fixup_regions() NOT_CDS_JAVA_HEAP_RETURN;
 
 #if INCLUDE_CDS_JAVA_HEAP
+  static void init_mapped_heap_relocation(ptrdiff_t delta, int dumptime_oop_shift);
 private:
   static bool _closed_regions_mapped;
   static bool _open_regions_mapped;
@@ -132,12 +132,16 @@ private:
   static bool _loading_failed;
 
   // UseCompressedOops only: Used by decode_from_archive
+  static bool    _narrow_oop_base_initialized;
   static address _narrow_oop_base;
   static int     _narrow_oop_shift;
 
-  // !UseCompressedOops only: used to relocate pointers to the archived objects
+  // is_mapped() only: the mapped address of each region is offset by this amount from
-  static ptrdiff_t _runtime_delta;
+  // their requested address.
+  static ptrdiff_t _mapped_heap_delta;
+  static bool      _mapped_heap_relocation_initialized;
 
+  static void init_narrow_oop_decoding(address base, int shift);
   static int init_loaded_regions(FileMapInfo* mapinfo, LoadedArchiveHeapRegion* loaded_regions,
                                  MemRegion& archive_space);
   static void sort_loaded_regions(LoadedArchiveHeapRegion* loaded_regions, int num_loaded_regions,
@@ -161,11 +165,6 @@ public:
   static void assert_in_loaded_heap(uintptr_t o) {
     assert(is_in_loaded_heap(o), "must be");
   }
-
-  static void set_runtime_delta(ptrdiff_t delta) {
-    assert(!UseCompressedOops, "must be");
-    _runtime_delta = delta;
-  }
 #endif // INCLUDE_CDS_JAVA_HEAP
 
 };

src/hotspot/share/cds/archiveHeapLoader.inline.hpp

@@ -34,6 +34,7 @@
 
 inline oop ArchiveHeapLoader::decode_from_archive(narrowOop v) {
   assert(!CompressedOops::is_null(v), "narrow oop value can never be zero");
+  assert(_narrow_oop_base_initialized, "relocation information must have been initialized");
   uintptr_t p = ((uintptr_t)_narrow_oop_base) + ((uintptr_t)v << _narrow_oop_shift);
   if (p >= _dumptime_base_0) {
     assert(p < _dumptime_top, "must be");

src/hotspot/share/cds/archiveUtils.cpp

@@ -328,7 +328,8 @@ void ReadClosure::do_oop(oop *p) {
     if (dumptime_oop == 0 || !ArchiveHeapLoader::is_fully_available()) {
       *p = NULL;
     } else {
-      intptr_t runtime_oop = dumptime_oop + ArchiveHeapLoader::runtime_delta();
+      assert(!ArchiveHeapLoader::is_loaded(), "ArchiveHeapLoader::can_load() is not supported for uncompessed oops");
+      intptr_t runtime_oop = dumptime_oop + ArchiveHeapLoader::mapped_heap_delta();
       *p = cast_to_oop(runtime_oop);
     }
   }

src/hotspot/share/cds/filemap.cpp

@@ -1629,13 +1629,13 @@ void FileMapInfo::write_region(int region, char* base, size_t size,
     assert(!DynamicDumpSharedSpaces, "must be");
     requested_base = base;
     if (UseCompressedOops) {
-      mapping_offset = (size_t)CompressedOops::encode_not_null(cast_to_oop(base));
+      mapping_offset = (size_t)((address)base - CompressedOops::base());
+      assert((mapping_offset >> CompressedOops::shift()) << CompressedOops::shift() == mapping_offset, "must be");
     } else {
 #if INCLUDE_G1GC
       mapping_offset = requested_base - (char*)G1CollectedHeap::heap()->reserved().start();
 #endif
     }
-    assert(mapping_offset == (size_t)(uint32_t)mapping_offset, "must be 32-bit only");
   } else {
     char* requested_SharedBaseAddress = (char*)MetaspaceShared::requested_base_address();
     requested_base = ArchiveBuilder::current()->to_requested(base);
@@ -2060,16 +2060,6 @@ size_t FileMapInfo::read_bytes(void* buffer, size_t count) {
   return count;
 }
 
-address FileMapInfo::decode_start_address(FileMapRegion* spc, bool with_current_oop_encoding_mode) {
-  size_t offset = spc->mapping_offset();
-  narrowOop n = CompressedOops::narrow_oop_cast(offset);
-  if (with_current_oop_encoding_mode) {
-    return cast_from_oop<address>(CompressedOops::decode_raw_not_null(n));
-  } else {
-    return cast_from_oop<address>(ArchiveHeapLoader::decode_from_archive(n));
-  }
-}
-
 static MemRegion *closed_heap_regions = NULL;
 static MemRegion *open_heap_regions = NULL;
 static int num_closed_heap_regions = 0;
@@ -2084,7 +2074,7 @@ bool FileMapInfo::has_heap_regions() {
 // current oop encoding mode. This range may be different than the one seen at
 // dump time due to encoding mode differences. The result is used in determining
 // if/how these regions should be relocated at run time.
-MemRegion FileMapInfo::get_heap_regions_range_with_current_oop_encoding_mode() {
+MemRegion FileMapInfo::get_heap_regions_requested_range() {
   address start = (address) max_uintx;
   address end   = NULL;
 
@@ -2094,8 +2084,10 @@ MemRegion FileMapInfo::get_heap_regions_range_with_current_oop_encoding_mode() {
     FileMapRegion* r = region_at(i);
     size_t size = r->used();
     if (size > 0) {
-      address s = start_address_as_decoded_with_current_oop_encoding_mode(r);
+      address s = heap_region_requested_address(r);
       address e = s + size;
+      log_info(cds)("Heap region %s = " INTPTR_FORMAT " - " INTPTR_FORMAT " = "  SIZE_FORMAT_W(8) " bytes",
+                    region_name(i), p2i(s), p2i(e), size);
       if (start > s) {
         start = s;
       }
@@ -2105,6 +2097,10 @@ MemRegion FileMapInfo::get_heap_regions_range_with_current_oop_encoding_mode() {
     }
   }
   assert(end != NULL, "must have at least one used heap region");
+
+  start = align_down(start, HeapRegion::GrainBytes);
+  end = align_up(end, HeapRegion::GrainBytes);
+
   return MemRegion((HeapWord*)start, (HeapWord*)end);
 }
 
@@ -2116,10 +2112,15 @@ void FileMapInfo::map_or_load_heap_regions() {
       success = map_heap_regions();
     } else if (ArchiveHeapLoader::can_load()) {
       success = ArchiveHeapLoader::load_heap_regions(this);
+    } else {
+      if (!UseCompressedOops && !ArchiveHeapLoader::can_map()) {
+        // TODO - remove implicit knowledge of G1
+        log_info(cds)("Cannot use CDS heap data. UseG1GC is required for -XX:-UseCompressedOops");
       } else {
         log_info(cds)("Cannot use CDS heap data. UseEpsilonGC, UseG1GC, UseSerialGC or UseParallelGC are required.");
       }
     }
+  }
 
   if (!success) {
     MetaspaceShared::disable_full_module_graph();
@@ -2172,27 +2173,55 @@ bool FileMapInfo::can_use_heap_regions() {
   return true;
 }
 
-// The address where the bottom of this shared heap region should be mapped
+// The actual address of this region during dump time.
-// at runtime
+address FileMapInfo::heap_region_dumptime_address(FileMapRegion* r) {
-address FileMapInfo::heap_region_runtime_start_address(FileMapRegion* spc) {
   assert(UseSharedSpaces, "runtime only");
-  spc->assert_is_heap_region();
+  r->assert_is_heap_region();
+  assert(is_aligned(r->mapping_offset(), sizeof(HeapWord)), "must be");
   if (UseCompressedOops) {
-    return start_address_as_decoded_from_archive(spc);
+    return /*dumptime*/ narrow_oop_base() + r->mapping_offset();
   } else {
-    assert(is_aligned(spc->mapping_offset(), sizeof(HeapWord)), "must be");
+    return heap_region_requested_address(r);
-    return header()->heap_begin() + spc->mapping_offset() + ArchiveHeapLoader::runtime_delta();
   }
 }
 
-void FileMapInfo::set_shared_heap_runtime_delta(ptrdiff_t delta) {
+// The address where this region can be mapped into the runtime heap without
+// patching any of the pointers that are embedded in this region.
+address FileMapInfo::heap_region_requested_address(FileMapRegion* r) {
+  assert(UseSharedSpaces, "runtime only");
+  r->assert_is_heap_region();
+  assert(is_aligned(r->mapping_offset(), sizeof(HeapWord)), "must be");
+  assert(ArchiveHeapLoader::can_map(), "cannot be used by ArchiveHeapLoader::can_load() mode");
   if (UseCompressedOops) {
-    ArchiveHeapLoader::init_narrow_oop_decoding(narrow_oop_base() + delta, narrow_oop_shift());
+    // We can avoid relocation if each region's offset from the runtime CompressedOops::base()
+    // is the same as its offset from the CompressedOops::base() during dumptime.
+    // Note that CompressedOops::base() may be different between dumptime and runtime.
+    //
+    // Example:
+    // Dumptime base = 0x1000 and shift is 0. We have a region at address 0x2000. There's a
+    // narrowOop P stored in this region that points to an object at address 0x2200.
+    // P's encoded value is 0x1200.
+    //
+    // Runtime base = 0x4000 and shift is also 0. If we map this region at 0x5000, then
+    // the value P can remain 0x1200. The decoded address = (0x4000 + (0x1200 << 0)) = 0x5200,
+    // which is the runtime location of the referenced object.
+    return /*runtime*/ CompressedOops::base() + r->mapping_offset();
   } else {
-    ArchiveHeapLoader::set_runtime_delta(delta);
+    // We can avoid relocation if each region is mapped into the exact same address
+    // where it was at dump time.
+    return /*dumptime*/header()->heap_begin() + r->mapping_offset();
   }
 }
 
+// The address where this shared heap region is actually mapped at runtime. This function
+// can be called only after we have determined the value for ArchiveHeapLoader::mapped_heap_delta().
+address FileMapInfo::heap_region_mapped_address(FileMapRegion* r) {
+  assert(UseSharedSpaces, "runtime only");
+  r->assert_is_heap_region();
+  assert(ArchiveHeapLoader::can_map(), "cannot be used by ArchiveHeapLoader::can_load() mode");
+  return heap_region_requested_address(r) + ArchiveHeapLoader::mapped_heap_delta();
+}
+
 //
 // Map the closed and open archive heap objects to the runtime java heap.
 //
@@ -2208,71 +2237,46 @@ void FileMapInfo::set_shared_heap_runtime_delta(ptrdiff_t delta) {
 // regions may be added. GC may mark and update references in the mapped
 // open archive objects.
 void FileMapInfo::map_heap_regions_impl() {
-  if (narrow_oop_mode() != CompressedOops::mode() ||
+  // G1 -- always map at the very top of the heap to avoid fragmentation.
-      narrow_oop_base() != CompressedOops::base() ||
+  assert(UseG1GC, "the following code assumes G1");
-      narrow_oop_shift() != CompressedOops::shift()) {
+  _heap_pointers_need_patching = false;
+
+  MemRegion heap_range = G1CollectedHeap::heap()->reserved();
+  MemRegion archive_range = get_heap_regions_requested_range();
+
+  address heap_end = (address)heap_range.end();
+  address archive_end = (address)archive_range.end();
+
+  assert(is_aligned(heap_end, HeapRegion::GrainBytes), "must be");
+  assert(is_aligned(archive_end, HeapRegion::GrainBytes), "must be");
+
+  if (UseCompressedOops &&
+      (narrow_oop_mode() != CompressedOops::mode() ||
+       narrow_oop_shift() != CompressedOops::shift())) {
     log_info(cds)("CDS heap data needs to be relocated because the archive was created with an incompatible oop encoding mode.");
     _heap_pointers_need_patching = true;
-  } else {
+  } else if (!heap_range.contains(archive_range)) {
-    if (UseCompressedOops) {
-      MemRegion range = get_heap_regions_range_with_current_oop_encoding_mode();
-      if (!CompressedOops::is_in(range)) {
     log_info(cds)("CDS heap data needs to be relocated because");
-        log_info(cds)("the desired range " PTR_FORMAT " - "  PTR_FORMAT, p2i(range.start()), p2i(range.end()));
+    log_info(cds)("the desired range " PTR_FORMAT " - "  PTR_FORMAT, p2i(archive_range.start()), p2i(archive_range.end()));
-        log_info(cds)("is outside of the heap " PTR_FORMAT " - "  PTR_FORMAT, p2i(CompressedOops::begin()), p2i(CompressedOops::end()));
+    log_info(cds)("is outside of the heap " PTR_FORMAT " - "  PTR_FORMAT, p2i(heap_range.start()), p2i(heap_range.end()));
     _heap_pointers_need_patching = true;
-      } else if (header()->heap_end() != CompressedOops::end()) {
+  } else {
+    assert(heap_end >= archive_end, "must be");
+    if (heap_end != archive_end) {
       log_info(cds)("CDS heap data needs to be relocated to the end of the runtime heap to reduce fragmentation");
       _heap_pointers_need_patching = true;
     }
-    } else {
-      MemRegion range((HeapWord*)header()->heap_begin(), (HeapWord*)header()->heap_end());
-      if (!G1CollectedHeap::heap()->reserved().contains(range)) {
-        log_info(cds)("CDS heap data needs to be relocated because");
-        log_info(cds)("the desired range " PTR_FORMAT " - "  PTR_FORMAT, p2i(range.start()), p2i(range.end()));
-        log_info(cds)("is outside of the heap " PTR_FORMAT " - "  PTR_FORMAT,
-            p2i((address)G1CollectedHeap::heap()->reserved().start()), p2i((address)G1CollectedHeap::heap()->reserved().end()));
-        _heap_pointers_need_patching = true;
-      } else if (header()->heap_end() != (address)G1CollectedHeap::heap()->reserved().end()) {
-        log_info(cds)("CDS heap data needs to be relocated to the end of the runtime heap to reduce fragmentation");
-        _heap_pointers_need_patching = true;
-      }
-    }
   }
 
   ptrdiff_t delta = 0;
   if (_heap_pointers_need_patching) {
-    //   dumptime heap end  ------------v
+    delta = heap_end - archive_end;
-    //   [      |archived heap regions| ]         run time heap end -----v
-    //                                       [   |archived heap regions| ]
-    //          ^
-    //          D                                ^
-    //                                           R
-    //                                  |<-----delta-------------------->|
-    //
-    // At dump time, the archived heap regions were near the top of the heap.
-    // At run time, if the heap ends at a different address, we need to
-    // move them near to top of the run time heap. This can be done by
-    // the simple math of adding the delta as shown above.
-    //
-    // Also: D = bottom of a heap region at dump time
-    //       R = bottom of a heap region at run time
-    //
-    // FileMapRegion* spc = ...;
-    //   address D = header()->heap_begin() + spc->mapping_offset();
-    //   address R = D + delta;
-    address dumptime_heap_end = header()->heap_end();
-    address runtime_heap_end = UseCompressedOops ? CompressedOops::end() :
-                                                   (address)G1CollectedHeap::heap()->reserved().end();
-    delta = runtime_heap_end - dumptime_heap_end;
   }
 
   log_info(cds)("CDS heap data relocation delta = " INTX_FORMAT " bytes", delta);
 
-  set_shared_heap_runtime_delta(delta);
-
   FileMapRegion* r = region_at(MetaspaceShared::first_closed_heap_region);
-  address relocated_closed_heap_region_bottom = heap_region_runtime_start_address(r);
+  address relocated_closed_heap_region_bottom = heap_region_requested_address(r) + delta;
 
   if (!is_aligned(relocated_closed_heap_region_bottom, HeapRegion::GrainBytes)) {
     // Align the bottom of the closed archive heap regions at G1 region boundary.
@@ -2284,10 +2288,12 @@ void FileMapInfo::map_heap_regions_impl() {
     log_info(cds)("CDS heap data needs to be relocated lower by a further " SIZE_FORMAT
                   " bytes to " INTX_FORMAT " to be aligned with HeapRegion::GrainBytes",
                   align, delta);
-    set_shared_heap_runtime_delta(delta);
-    relocated_closed_heap_region_bottom = heap_region_runtime_start_address(r);
     _heap_pointers_need_patching = true;
   }
+
+  ArchiveHeapLoader::init_mapped_heap_relocation(delta, narrow_oop_shift());
+  relocated_closed_heap_region_bottom = heap_region_mapped_address(r);
+
   assert(is_aligned(relocated_closed_heap_region_bottom, HeapRegion::GrainBytes),
          "must be");
 
@@ -2353,7 +2359,7 @@ bool FileMapInfo::map_heap_regions(int first, int max,  bool is_open_archive,
     r = region_at(i);
     size_t size = r->used();
     if (size > 0) {
-      HeapWord* start = (HeapWord*)heap_region_runtime_start_address(r);
+      HeapWord* start = (HeapWord*)heap_region_mapped_address(r);
       regions[num_regions] = MemRegion(start, size / HeapWordSize);
       num_regions ++;
       log_info(cds)("Trying to map heap data: region[%d] at " INTPTR_FORMAT ", size = " SIZE_FORMAT_W(8) " bytes",
@@ -2418,7 +2424,6 @@ void FileMapInfo::patch_heap_embedded_pointers() {
     return;
   }
 
-  log_info(cds)("patching heap embedded pointers");
   patch_heap_embedded_pointers(closed_heap_regions,
                                num_closed_heap_regions,
                                MetaspaceShared::first_closed_heap_region);
@@ -2433,7 +2438,24 @@ void FileMapInfo::patch_heap_embedded_pointers(MemRegion* regions, int num_regio
   char* bitmap_base = map_bitmap_region();
   assert(bitmap_base != NULL, "must have already been mapped");
   for (int i=0; i<num_regions; i++) {
-    FileMapRegion* r = region_at(i + first_region_idx);
+    int region_idx = i + first_region_idx;
+    FileMapRegion* r = region_at(region_idx);
+    if (UseCompressedOops) {
+      // These are the encoded values for the bottom of this region at dump-time vs run-time:
+      narrowOop dt_encoded_bottom = CompressedOops::narrow_oop_cast(r->mapping_offset() >> narrow_oop_shift());
+      narrowOop rt_encoded_bottom = CompressedOops::encode_not_null(cast_to_oop(regions[i].start()));
+      log_info(cds)("patching heap embedded pointers for %s: narrowOop 0x%8x -> 0x%8x",
+                    region_name(region_idx), (uint)dt_encoded_bottom, (uint)rt_encoded_bottom);
+      // TODO JDK-8269736: if we have the same narrow_oop_shift between dumptime and runtime,
+      // Each embedded pointer P can be updated by:
+      //     P += (rt_encoded_bottom - dt_encoded_bottom)
+      //
+      // TODO:
+      // if (dt_encoded_bottom == rt_encoded_bottom && narrow_oop_shift() == CompressedOops::shift()) {
+      //   //nothing to do
+      //   return;
+      // }
+    }
     ArchiveHeapLoader::patch_embedded_pointers(
       regions[i],
       (address)(region_at(MetaspaceShared::bm)->mapped_base()) + r->oopmap_offset(),
@@ -2599,14 +2621,9 @@ bool FileMapInfo::initialize() {
 }
 
 char* FileMapInfo::region_addr(int idx) {
+  assert(UseSharedSpaces, "must be");
   FileMapRegion* r = region_at(idx);
-  if (HeapShared::is_heap_region(idx)) {
-    assert(DumpSharedSpaces, "The following doesn't work at runtime");
-    return r->used() > 0 ?
-          (char*)start_address_as_decoded_with_current_oop_encoding_mode(r) : NULL;
-  } else {
   return r->mapped_base();
-  }
 }
 
 // The 2 core spaces are RW->RO
@@ -2726,17 +2743,6 @@ bool FileMapInfo::validate_header() {
   }
 }
 
-// Check if a given address is within one of the shared regions
-bool FileMapInfo::is_in_shared_region(const void* p, int idx) {
-  assert(idx == MetaspaceShared::ro ||
-         idx == MetaspaceShared::rw, "invalid region index");
-  char* base = region_addr(idx);
-  if (p >= base && p < base + region_at(idx)->used()) {
-    return true;
-  }
-  return false;
-}
-
 // Unmap mapped regions of shared space.
 void FileMapInfo::stop_sharing_and_unmap(const char* msg) {
   MetaspaceShared::set_shared_metaspace_range(NULL, NULL, NULL);

src/hotspot/share/cds/filemap.hpp

@@ -468,7 +468,7 @@ public:
   void  patch_heap_embedded_pointers(MemRegion* regions, int num_regions,
                                      int first_region_idx) NOT_CDS_JAVA_HEAP_RETURN;
   bool  has_heap_regions()  NOT_CDS_JAVA_HEAP_RETURN_(false);
-  MemRegion get_heap_regions_range_with_current_oop_encoding_mode() NOT_CDS_JAVA_HEAP_RETURN_(MemRegion());
+  MemRegion get_heap_regions_requested_range() NOT_CDS_JAVA_HEAP_RETURN_(MemRegion());
   bool  read_region(int i, char* base, size_t size, bool do_commit);
   char* map_bitmap_region();
   void  unmap_region(int i);
@@ -490,7 +490,6 @@ public:
     CDS_ONLY(return _memory_mapping_failed;)
     NOT_CDS(return false;)
   }
-  bool is_in_shared_region(const void* p, int idx) NOT_CDS_RETURN_(false);
 
   // Stop CDS sharing and unmap CDS regions.
   static void stop_sharing_and_unmap(const char* msg);
@@ -583,25 +582,16 @@ public:
   bool  can_use_heap_regions();
   bool  load_heap_regions() NOT_CDS_JAVA_HEAP_RETURN_(false);
   bool  map_heap_regions() NOT_CDS_JAVA_HEAP_RETURN_(false);
-  address heap_region_runtime_start_address(FileMapRegion* spc) NOT_CDS_JAVA_HEAP_RETURN_(NULL);
-  void set_shared_heap_runtime_delta(ptrdiff_t delta) NOT_CDS_JAVA_HEAP_RETURN;
   void  map_heap_regions_impl() NOT_CDS_JAVA_HEAP_RETURN;
   MapArchiveResult map_region(int i, intx addr_delta, char* mapped_base_address, ReservedSpace rs);
   bool  relocate_pointers_in_core_regions(intx addr_delta);
   static size_t set_bitmaps_offset(GrowableArray<ArchiveHeapBitmapInfo> *bitmaps, size_t curr_size);
   static size_t write_bitmaps(GrowableArray<ArchiveHeapBitmapInfo> *bitmaps, size_t curr_offset, char* buffer);
 
-  address decode_start_address(FileMapRegion* spc, bool with_current_oop_encoding_mode);
-
-  // The starting address of spc, as calculated with CompressedOop::decode_non_null()
-  address start_address_as_decoded_with_current_oop_encoding_mode(FileMapRegion* spc) {
-    return decode_start_address(spc, true);
-  }
 public:
-  // The starting address of spc, as calculated with HeapShared::decode_from_archive()
+  address heap_region_dumptime_address(FileMapRegion* r) NOT_CDS_JAVA_HEAP_RETURN_(NULL);
-  address start_address_as_decoded_from_archive(FileMapRegion* spc) {
+  address heap_region_requested_address(FileMapRegion* r) NOT_CDS_JAVA_HEAP_RETURN_(NULL);
-    return decode_start_address(spc, false);
+  address heap_region_mapped_address(FileMapRegion* r) NOT_CDS_JAVA_HEAP_RETURN_(NULL);
-  }
 
 private:
 

src/hotspot/share/cds/metaspaceShared.cpp

@@ -956,11 +956,6 @@ void MetaspaceShared::set_shared_metaspace_range(void* base, void *static_top, v
   MetaspaceObj::set_shared_metaspace_range(base, top);
 }
 
-// Return true if given address is in the misc data region
-bool MetaspaceShared::is_in_shared_region(const void* p, int idx) {
-  return UseSharedSpaces && FileMapInfo::current_info()->is_in_shared_region(p, idx);
-}
-
 bool MetaspaceShared::is_shared_dynamic(void* p) {
   if ((p < MetaspaceObj::shared_metaspace_top()) &&
       (p >= _shared_metaspace_static_top)) {

src/hotspot/share/cds/metaspaceShared.hpp

@@ -114,9 +114,6 @@ public:
 
   static void set_shared_metaspace_range(void* base, void *static_top, void* top) NOT_CDS_RETURN;
 
-  // Return true if given address is in the shared region corresponding to the idx
-  static bool is_in_shared_region(const void* p, int idx) NOT_CDS_RETURN_(false);
-
   static bool is_shared_dynamic(void* p) NOT_CDS_RETURN_(false);
 
   static void serialize(SerializeClosure* sc) NOT_CDS_RETURN;

src/hotspot/share/classfile/stringTable.cpp

@@ -72,16 +72,18 @@ const double CLEAN_DEAD_HIGH_WATER_MARK = 0.5;
 
 #if INCLUDE_CDS_JAVA_HEAP
 inline oop read_string_from_compact_hashtable(address base_address, u4 offset) {
+  assert(ArchiveHeapLoader::are_archived_strings_available(), "sanity");
   if (UseCompressedOops) {
     assert(sizeof(narrowOop) == sizeof(offset), "must be");
     narrowOop v = CompressedOops::narrow_oop_cast(offset);
     return ArchiveHeapLoader::decode_from_archive(v);
   } else {
+    assert(!ArchiveHeapLoader::is_loaded(), "Pointer relocation for uncompressed oops is unimplemented");
     intptr_t dumptime_oop = (uintptr_t)offset;
     assert(dumptime_oop != 0, "null strings cannot be interned");
     intptr_t runtime_oop = dumptime_oop +
                            (intptr_t)FileMapInfo::current_info()->header()->heap_begin() +
-                           (intptr_t)ArchiveHeapLoader::runtime_delta();
+                           (intptr_t)ArchiveHeapLoader::mapped_heap_delta();
     return (oop)cast_to_oop(runtime_oop);
   }
 }

src/hotspot/share/include/cds.h

@@ -50,9 +50,17 @@ typedef struct CDSFileMapRegion {
   int     _mapped_from_file;  // Is this region mapped from a file?
                               // If false, this region was initialized using ::read().
   size_t  _file_offset;       // Data for this region starts at this offset in the archive file.
-  size_t  _mapping_offset;    // This region should be mapped at this offset from the base address
+  size_t  _mapping_offset;    // This encodes the requested address for this region to be mapped at runtime.
-                              // - for non-heap regions, the base address is SharedBaseAddress
+                              // However, the JVM may choose to map at an alternative location (e.g., for ASLR,
-                              // - for heap regions, the base address is the compressed oop encoding base
+                              // or to adapt to the available ranges in the Java heap range).
+                              // - For an RO/RW region, the requested address is:
+                              //     FileMapHeader::requested_base_address() + _mapping_offset
+                              // - For a heap region, the requested address is:
+                              //     +UseCompressedOops: /*runtime*/ CompressedOops::base() + _mapping_offset
+                              //     -UseCompressedOops: FileMapHeader::heap_begin() + _mapping_offset
+                              //     See FileMapInfo::heap_region_requested_address().
+                              // - For bitmap regions, the _mapping_offset is always zero. The runtime address
+                              //   is picked by the OS.
   size_t  _used;              // Number of bytes actually used by this region (excluding padding bytes added
                               // for alignment purposed.
   size_t  _oopmap_offset;     // Bitmap for relocating oop fields in archived heap objects.

test/hotspot/jtreg/runtime/cds/appcds/TestSerialGCWithCDS.java

@@ -37,16 +37,42 @@
  * @run driver TestSerialGCWithCDS
  */
 
+// Below is exactly the same as above, except:
+// - requires vm.bits == "64"
+// - extra argument "false"
+
+/*
+ * @test Loading CDS archived heap objects into SerialGC
+ * @bug 8234679
+ * @requires vm.cds
+ * @requires vm.gc.Serial
+ * @requires vm.gc.G1
+ * @requires vm.bits == "64"
+ *
+ * @comment don't run this test if any -XX::+Use???GC options are specified, since they will
+ *          interfere with the test.
+ * @requires vm.gc == null
+ *
+ * @library /test/lib /test/hotspot/jtreg/runtime/cds/appcds
+ * @compile test-classes/Hello.java
+ * @run driver TestSerialGCWithCDS false
+ */
+
 import jdk.test.lib.Platform;
 import jdk.test.lib.process.OutputAnalyzer;
 
 public class TestSerialGCWithCDS {
     public final static String HELLO = "Hello World";
     static String helloJar;
+    static boolean useCompressedOops = true;
 
     public static void main(String... args) throws Exception {
         helloJar = JarBuilder.build("hello", "Hello");
 
+        if (args.length > 0 && args[0].equals("false")) {
+            useCompressedOops = false;
+        }
+
         // Check if we can use SerialGC during dump time, or run time, or both.
         test(false, true);
         test(true,  false);
@@ -54,7 +80,9 @@ public class TestSerialGCWithCDS {
 
         // We usually have 2 heap regions. To increase test coverage, we can have 3 heap regions
         // by using "-Xmx256m -XX:ObjectAlignmentInBytes=64"
-        if (Platform.is64bit()) test(false, true, true);
+        if (Platform.is64bit()) {
+            test(false, true, /*useSmallRegions=*/true);
+        }
     }
 
     final static String G1 = "-XX:+UseG1GC";
@@ -65,10 +93,17 @@ public class TestSerialGCWithCDS {
     }
 
     static void test(boolean dumpWithSerial, boolean execWithSerial, boolean useSmallRegions) throws Exception {
+        String DUMMY = "-showversion"; // A harmless option that doesn't doesn't do anything except for printing out the version
         String dumpGC = dumpWithSerial ? Serial : G1;
         String execGC = execWithSerial ? Serial : G1;
-        String small1 = useSmallRegions ? "-Xmx256m" : "-showversion";
+        String small1 = useSmallRegions ? "-Xmx256m" : DUMMY;
-        String small2 = useSmallRegions ? "-XX:ObjectAlignmentInBytes=64" : "-showversion";
+        String small2 = useSmallRegions ? "-XX:ObjectAlignmentInBytes=64" : DUMMY;
+        String coops;
+        if (Platform.is64bit()) {
+            coops = useCompressedOops ? "-XX:+UseCompressedOops" : "-XX:-UseCompressedOops";
+        } else {
+            coops = DUMMY;
+        }
         OutputAnalyzer out;
 
         System.out.println("0. Dump with " + dumpGC);
@@ -77,6 +112,7 @@ public class TestSerialGCWithCDS {
                               dumpGC,
                               small1,
                               small2,
+                              coops,
                               "-Xlog:cds");
         out.shouldContain("Dumping shared data to file:");
         out.shouldHaveExitValue(0);
@@ -86,27 +122,27 @@ public class TestSerialGCWithCDS {
                               execGC,
                               small1,
                               small2,
+                              coops,
                               "-Xlog:cds",
                               "Hello");
-        out.shouldContain(HELLO);
+        checkExecOutput(dumpWithSerial, execWithSerial, out);
-        out.shouldHaveExitValue(0);
 
         System.out.println("2. Exec with " + execGC + " and test ArchiveRelocationMode");
         out = TestCommon.exec(helloJar,
                               execGC,
                               small1,
                               small2,
+                              coops,
                               "-Xlog:cds,cds+heap",
                               "-XX:ArchiveRelocationMode=1", // always relocate shared metadata
                               "Hello");
-        out.shouldContain(HELLO);
         if (out.getOutput().contains("Trying to map heap") || out.getOutput().contains("Loaded heap")) {
             // The native data in the RO/RW regions have been relocated. If the CDS heap is
             // mapped/loaded, we must patch all the native pointers. (CDS heap is
             // not supported on all platforms)
             out.shouldContain("Patching native pointers in heap region");
         }
-        out.shouldHaveExitValue(0);
+        checkExecOutput(dumpWithSerial, execWithSerial, out);
 
         int n = 2;
         if (dumpWithSerial == false && execWithSerial == true) {
@@ -126,10 +162,11 @@ public class TestSerialGCWithCDS {
                                       small1,
                                       small2,
                                       xmx,
+                                      coops,
                                       "-Xlog:cds",
                                       "Hello");
                 if (out.getExitValue() == 0) {
-                    out.shouldContain(HELLO);
+                    checkExecOutput(dumpWithSerial, execWithSerial, out);
                 } else {
                     String output = out.getStdout() + out.getStderr();
                     String exp1 = "Too small maximum heap";
@@ -142,4 +179,19 @@ public class TestSerialGCWithCDS {
             }
         }
     }
+
+    static void checkExecOutput(boolean dumpWithSerial, boolean execWithSerial, OutputAnalyzer out) {
+        String errMsg = "Cannot use CDS heap data. UseG1GC is required for -XX:-UseCompressedOops";
+        if (Platform.is64bit() &&
+            !Platform.isWindows() && // archive heap not supported on Windows.
+            !dumpWithSerial && // Dumped with G1, so we have an archived heap
+            execWithSerial && // Running with serial
+            !useCompressedOops) { // ArchiveHeapLoader::can_load() always returns false when COOP is disabled
+            out.shouldContain(errMsg);
+        }
+        if (!execWithSerial) {
+            // We should never see this message with G1
+            out.shouldNotContain(errMsg);
+        }
+    }
 }