Add total_space_rs, total reserved space to release_reserved_spaces and reserve_address_space_for_archives, made changes to check failed output on test.

Yumin Qi · Yumin Qi · commit f79583061ced · 2020-12-07T21:37:36.000-08:00
diff --git a/src/hotspot/share/memory/metaspaceShared.cpp b/src/hotspot/share/memory/metaspaceShared.cpp
@@ -1364,10 +1364,13 @@ MapArchiveResult MetaspaceShared::map_archives(FileMapInfo* static_mapinfo, File
     assert(static_mapinfo->mapping_end_offset() == dynamic_mapinfo->mapping_base_offset(), "no gap");
   }
 
-  ReservedSpace archive_space_rs, class_space_rs;
+  ReservedSpace total_space_rs, archive_space_rs, class_space_rs;
   MapArchiveResult result = MAP_ARCHIVE_OTHER_FAILURE;
-  char* mapped_base_address = reserve_address_space_for_archives(static_mapinfo, dynamic_mapinfo,
-                                                                 use_requested_addr, archive_space_rs,
+  char* mapped_base_address = reserve_address_space_for_archives(static_mapinfo,
+                                                                 dynamic_mapinfo,
+                                                                 use_requested_addr,
+                                                                 total_space_rs,
+                                                                 archive_space_rs,
                                                                  class_space_rs);
   if (mapped_base_address == NULL) {
     result = MAP_ARCHIVE_MMAP_FAILURE;
@@ -1417,6 +1420,7 @@ MapArchiveResult MetaspaceShared::map_archives(FileMapInfo* static_mapinfo, File
       //  this with use_requested_addr, since we're going to patch all the
       //  pointers anyway so there's no benefit to mmap.
       if (use_requested_addr) {
+        assert(!total_space_rs.is_reserved(), "Should not be reserved for Windows");
         log_info(cds)("Windows mmap workaround: releasing archive space.");
         archive_space_rs.release();
       }
@@ -1485,7 +1489,8 @@ MapArchiveResult MetaspaceShared::map_archives(FileMapInfo* static_mapinfo, File
   } else {
     unmap_archive(static_mapinfo);
     unmap_archive(dynamic_mapinfo);
-    release_reserved_spaces(archive_space_rs, class_space_rs);
+    log_debug(cds)("Released shared space " INTPTR_FORMAT, p2i(total_space_rs.base()));
+    release_reserved_spaces(total_space_rs, archive_space_rs, class_space_rs);
   }
 
   return result;
@@ -1534,6 +1539,9 @@ MapArchiveResult MetaspaceShared::map_archives(FileMapInfo* static_mapinfo, File
 // Return:
 //
 // - On success:
+//    - total_space_rs will be reserved as whole for archive_space_rs and
+//      class_space_rs if UseCompressedClassPointers is true (not Windows).
+//      On Windows, total_space_rs is not reserved.
 //    - archive_space_rs will be reserved and large enough to host static and
 //      if needed dynamic archive: [Base, A).
 //      archive_space_rs.base and size will be aligned to CDS reserve
@@ -1548,6 +1556,7 @@ MapArchiveResult MetaspaceShared::map_archives(FileMapInfo* static_mapinfo, File
 char* MetaspaceShared::reserve_address_space_for_archives(FileMapInfo* static_mapinfo,
                                                           FileMapInfo* dynamic_mapinfo,
                                                           bool use_archive_base_addr,
+                                                          ReservedSpace& total_space_rs,
                                                           ReservedSpace& archive_space_rs,
                                                           ReservedSpace& class_space_rs) {
 
@@ -1628,38 +1637,36 @@ char* MetaspaceShared::reserve_address_space_for_archives(FileMapInfo* static_ma
                                        false /* large */, (char*)ccs_base);
     }
     if (!archive_space_rs.is_reserved() || !class_space_rs.is_reserved()) {
-      release_reserved_spaces(archive_space_rs, class_space_rs);
+      release_reserved_spaces(total_space_rs, archive_space_rs, class_space_rs);
       return NULL;
     }
   } else {
-    ReservedSpace total_rs;
-    if (base_address != NULL) {
-      // Reserve at the given archive base address, or not at all.
-      total_rs = ReservedSpace(total_range_size, archive_space_alignment,
-                               false /* bool large */, (char*) base_address);
+    if (use_archive_base_addr && base_address != nullptr) {
+      total_space_rs = ReservedSpace(total_range_size, archive_space_alignment,
+                                     false /* bool large */, (char*) base_address);
     } else {
       // Reserve at any address, but leave it up to the platform to choose a good one.
-      total_rs = Metaspace::reserve_address_space_for_compressed_classes(total_range_size);
+      total_space_rs = Metaspace::reserve_address_space_for_compressed_classes(total_range_size);
     }
 
-    if (!total_rs.is_reserved()) {
+    if (!total_space_rs.is_reserved()) {
       return NULL;
     }
 
     // Paranoid checks:
-    assert(base_address == NULL || (address)total_rs.base() == base_address,
-           "Sanity (" PTR_FORMAT " vs " PTR_FORMAT ")", p2i(base_address), p2i(total_rs.base()));
-    assert(is_aligned(total_rs.base(), archive_space_alignment), "Sanity");
-    assert(total_rs.size() == total_range_size, "Sanity");
-    assert(CompressedKlassPointers::is_valid_base((address)total_rs.base()), "Sanity");
+    assert(base_address == NULL || (address)total_space_rs.base() == base_address,
+           "Sanity (" PTR_FORMAT " vs " PTR_FORMAT ")", p2i(base_address), p2i(total_space_rs.base()));
+    assert(is_aligned(total_space_rs.base(), archive_space_alignment), "Sanity");
+    assert(total_space_rs.size() == total_range_size, "Sanity");
+    assert(CompressedKlassPointers::is_valid_base((address)total_space_rs.base()), "Sanity");
 
     // Now split up the space into ccs and cds archive. For simplicity, just leave
-    //  the gap reserved at the end of the archive space.
-    archive_space_rs = total_rs.first_part(ccs_begin_offset,
-                                           (size_t)os::vm_allocation_granularity(),
-                                           /*split=*/false);
-    class_space_rs = total_rs.last_part(ccs_begin_offset);
-    MemTracker::record_virtual_memory_split_reserved(total_rs.base(), total_rs.size(),
+    //  the gap reserved at the end of the archive space. Do not do real splitting.
+    archive_space_rs = total_space_rs.first_part(ccs_begin_offset,
+                                                 (size_t)os::vm_allocation_granularity(),
+                                                 /*split=*/false);
+    class_space_rs = total_space_rs.last_part(ccs_begin_offset);
+    MemTracker::record_virtual_memory_split_reserved(total_space_rs.base(), total_space_rs.size(),
                                                      ccs_begin_offset);
   }
   assert(is_aligned(archive_space_rs.base(), archive_space_alignment), "Sanity");
@@ -1680,15 +1687,21 @@ char* MetaspaceShared::reserve_address_space_for_archives(FileMapInfo* static_ma
 
 }
 
-void MetaspaceShared::release_reserved_spaces(ReservedSpace& archive_space_rs,
+void MetaspaceShared::release_reserved_spaces(ReservedSpace& total_space_rs,
+                                              ReservedSpace& archive_space_rs,
                                               ReservedSpace& class_space_rs) {
-  if (archive_space_rs.is_reserved()) {
-    log_debug(cds)("Released shared space (archive) " INTPTR_FORMAT, p2i(archive_space_rs.base()));
-    archive_space_rs.release();
-  }
-  if (class_space_rs.is_reserved()) {
-    log_debug(cds)("Released shared space (classes) " INTPTR_FORMAT, p2i(class_space_rs.base()));
-    class_space_rs.release();
+  if (total_space_rs.is_reserved()) {
+    log_debug(cds)("Released shared space (archive + class) " INTPTR_FORMAT, p2i(total_space_rs.base()));
+    total_space_rs.release();
+  } else {
+    if (archive_space_rs.is_reserved()) {
+      log_debug(cds)("Released shared space (archive) " INTPTR_FORMAT, p2i(archive_space_rs.base()));
+      archive_space_rs.release();
+    }
+    if (class_space_rs.is_reserved()) {
+      log_debug(cds)("Released shared space (classes) " INTPTR_FORMAT, p2i(class_space_rs.base()));
+      class_space_rs.release();
+    }
   }
 }
 
diff --git a/src/hotspot/share/memory/metaspaceShared.hpp b/src/hotspot/share/memory/metaspaceShared.hpp
@@ -288,10 +288,12 @@ class MetaspaceShared : AllStatic {
   static char* reserve_address_space_for_archives(FileMapInfo* static_mapinfo,
                                                   FileMapInfo* dynamic_mapinfo,
                                                   bool use_archive_base_addr,
+                                                  ReservedSpace& total_space_rs,
                                                   ReservedSpace& archive_space_rs,
                                                   ReservedSpace& class_space_rs);
-  static void release_reserved_spaces(ReservedSpace& archive_space_rs,
-                                      ReservedSpace& class_space_rs);
+ static void release_reserved_spaces(ReservedSpace& total_space_rs,
+                                     ReservedSpace& archive_space_rs,
+                                     ReservedSpace& class_space_rs);
   static MapArchiveResult map_archive(FileMapInfo* mapinfo, char* mapped_base_address, ReservedSpace rs);
   static void unmap_archive(FileMapInfo* mapinfo);
 };
diff --git a/src/hotspot/share/runtime/os.cpp b/src/hotspot/share/runtime/os.cpp
@@ -1723,6 +1723,9 @@ bool os::release_memory(char* addr, size_t bytes) {
   } else {
     res = pd_release_memory(addr, bytes);
   }
+  if (!res) {
+    log_info(os)("os::release_memory(" PTR_FORMAT ", " SIZE_FORMAT ") failed", p2i(addr), bytes);
+  }
   return res;
 }
 
diff --git a/test/hotspot/jtreg/runtime/cds/SharedBaseAddress.java b/test/hotspot/jtreg/runtime/cds/SharedBaseAddress.java
@@ -50,6 +50,9 @@ public class SharedBaseAddress {
         "0",                  // always let OS pick the base address at runtime (ASLR for CDS archive)
     };
 
+    // failed pattern
+    private static String failedPattern ="os::release_memory(.*,\\s.*)\\sfailed";
+
     public static void main(String[] args) throws Exception {
 
         for (String testEntry : testTable) {
@@ -68,7 +71,8 @@ public static void main(String[] args) throws Exception {
             OutputAnalyzer out = CDSTestUtils.runWithArchiveAndCheck(opts);
             if (testEntry.equals("0")) {
               out.shouldContain("Archive(s) were created with -XX:SharedBaseAddress=0. Always map at os-selected address.")
-                 .shouldContain("Try to map archive(s) at an alternative address");
+                 .shouldContain("Try to map archive(s) at an alternative address")
+                 .shouldNotMatch(failedPattern);
             }
         }
     }

Original file line number	Diff line number	Diff line change
`@@ -1723,6 +1723,9 @@ bool os::release_memory(char* addr, size_t bytes) {`
`1723`	`1723`	`} else {`
`1724`	`1724`	`res = pd_release_memory(addr, bytes);`
`1725`	`1725`	`}`
	`1726`	`+ if (!res) {`
	`1727`	`+ log_info(os)("os::release_memory(" PTR_FORMAT ", " SIZE_FORMAT ") failed", p2i(addr), bytes);`
	`1728`	`+ }`
`1726`	`1729`	`return res;`
`1727`	`1730`	`}`
`1728`	`1731`
Original file line number	Diff line number	Diff line change
`@@ -50,6 +50,9 @@ public class SharedBaseAddress {`
`50`	`50`	`"0", // always let OS pick the base address at runtime (ASLR for CDS archive)`
`51`	`51`	`};`
`52`	`52`
	`53`	`+ // failed pattern`
	`54`	`+ private static String failedPattern ="os::release_memory(.,\\s.)\\sfailed";`
	`55`	`+`
`53`	`56`	`public static void main(String[] args) throws Exception {`
`54`	`57`
`55`	`58`	`for (String testEntry : testTable) {`
`@@ -68,7 +71,8 @@ public static void main(String[] args) throws Exception {`
`68`	`71`	`OutputAnalyzer out = CDSTestUtils.runWithArchiveAndCheck(opts);`
`69`	`72`	`if (testEntry.equals("0")) {`
`70`	`73`	`out.shouldContain("Archive(s) were created with -XX:SharedBaseAddress=0. Always map at os-selected address.")`
`71`		`- .shouldContain("Try to map archive(s) at an alternative address");`
	`74`	`+ .shouldContain("Try to map archive(s) at an alternative address")`
	`75`	`+ .shouldNotMatch(failedPattern);`
`72`	`76`	`}`
`73`	`77`	`}`
`74`	`78`	`}`