8231257: Avoid calling FileMapInfo::write_region twice

Reviewed-by: ccheung

Author: iklam

src/hotspot/share/memory/dynamicArchive.cpp

@@ -471,6 +471,7 @@ class DynamicArchiveBuilder : ResourceObj {
   void set_symbols_permanent();
   void relocate_buffer_to_target();
   void write_archive(char* read_only_tables_start);
+  void write_regions(FileMapInfo* dynamic_info);
 
   void init_first_dump_space(address reserved_bottom) {
     address first_space_base = reserved_bottom;
@@ -911,9 +912,7 @@ void DynamicArchiveBuilder::relocate_buffer_to_target() {
  _header->relocate_shared_path_table(table);
 }
 
-static void write_archive_info(FileMapInfo* dynamic_info, DynamicArchiveHeader *header) {
-  dynamic_info->write_header();
-  dynamic_info->align_file_position();
+void DynamicArchiveBuilder::write_regions(FileMapInfo* dynamic_info) {
   dynamic_info->write_region(MetaspaceShared::rw,
                              MetaspaceShared::read_write_dump_space()->base(),
                              MetaspaceShared::read_write_dump_space()->used(),
@@ -937,19 +936,14 @@ void DynamicArchiveBuilder::write_archive(char* read_only_tables_start) {
   FileMapInfo* dynamic_info = FileMapInfo::dynamic_info();
   assert(dynamic_info != NULL, "Sanity");
 
-  // Populate the file offsets, region crcs, etc. No data is written out.
-  write_archive_info(dynamic_info, _header);
-
-  // the header will no longer change. Compute its crc.
-  dynamic_info->set_header_crc(dynamic_info->compute_header_crc());
-
   // Now write the archived data including the file offsets.
   const char* archive_name = Arguments::GetSharedDynamicArchivePath();
   dynamic_info->open_for_write(archive_name);
-  write_archive_info(dynamic_info, _header);
+  write_regions(dynamic_info);
+  dynamic_info->set_header_crc(dynamic_info->compute_header_crc());
+  dynamic_info->write_header();
   dynamic_info->close();
 
-
   address base = to_target(_alloc_bottom);
   address top  = address(current_dump_space()->top()) + _buffer_to_target_delta;
   size_t file_size = pointer_delta(top, base, sizeof(char));

src/hotspot/share/memory/filemap.cpp

@@ -1033,6 +1033,11 @@ bool FileMapInfo::init_from_file(int fd, bool is_static) {
   return true;
 }
 
+void FileMapInfo::seek_to_position(size_t pos) {
+  if (lseek(_fd, (long)pos, SEEK_SET) < 0) {
+    fail_stop("Unable to seek to position " SIZE_FORMAT, pos);
+  }
+}
 
 // Read the FileMapInfo information from the file.
 bool FileMapInfo::open_for_read(const char* path) {
@@ -1088,14 +1093,26 @@ void FileMapInfo::open_for_write(const char* path) {
               os::strerror(errno));
   }
   _fd = fd;
-  _file_offset = 0;
   _file_open = true;
+
+  // Seek past the header. We will write the header after all regions are written
+  // and their CRCs computed.
+  size_t header_bytes = header()->header_size();
+  if (header()->magic() == CDS_DYNAMIC_ARCHIVE_MAGIC) {
+    header_bytes += strlen(Arguments::GetSharedArchivePath()) + 1;
+  }
+
+  header_bytes = align_up(header_bytes, os::vm_allocation_granularity());
+  _file_offset = header_bytes;
+  seek_to_position(_file_offset);
 }
 
 
 // Write the header to the file, seek to the next allocation boundary.
 
 void FileMapInfo::write_header() {
+  _file_offset = 0;
+  seek_to_position(_file_offset);
   char* base_archive_name = NULL;
   if (header()->magic() == CDS_DYNAMIC_ARCHIVE_MAGIC) {
     base_archive_name = (char*)Arguments::GetSharedArchivePath();
@@ -1108,7 +1125,6 @@ void FileMapInfo::write_header() {
   if (base_archive_name != NULL) {
     write_bytes(base_archive_name, header()->base_archive_name_size());
   }
-  align_file_position();
 }
 
 void FileMapRegion::init(bool is_heap_region, char* base, size_t size, bool read_only,
@@ -1132,9 +1148,6 @@ void FileMapRegion::init(bool is_heap_region, char* base, size_t size, bool read
   _crc = crc;
 }
 
-// Dump region to file.
-// This is called twice for each region during archiving, once before
-// the archive file is open (_file_open is false) and once after.
 void FileMapInfo::write_region(int region, char* base, size_t size,
                                bool read_only, bool allow_exec) {
   assert(DumpSharedSpaces || DynamicDumpSharedSpaces, "Dump time only");
@@ -1146,14 +1159,10 @@ void FileMapInfo::write_region(int region, char* base, size_t size,
     target_base = DynamicArchive::buffer_to_target(base);
   }
 
-  if (_file_open) {
-    guarantee(si->file_offset() == _file_offset, "file offset mismatch.");
-    log_info(cds)("Shared file region %d: " SIZE_FORMAT_HEX_W(08)
-                  " bytes, addr " INTPTR_FORMAT " file offset " SIZE_FORMAT_HEX_W(08),
-                  region, size, p2i(target_base), _file_offset);
-  } else {
-    si->set_file_offset(_file_offset);
-  }
+  si->set_file_offset(_file_offset);
+  log_info(cds)("Shared file region %d: " SIZE_FORMAT_HEX_W(08)
+                " bytes, addr " INTPTR_FORMAT " file offset " SIZE_FORMAT_HEX_W(08),
+                region, size, p2i(target_base), _file_offset);
 
   int crc = ClassLoader::crc32(0, base, (jint)size);
   si->init(HeapShared::is_heap_region(region), target_base, size, read_only, allow_exec, crc);
@@ -1196,8 +1205,7 @@ void FileMapInfo::write_region(int region, char* base, size_t size,
 //             +-- gap
 size_t FileMapInfo::write_archive_heap_regions(GrowableArray<MemRegion> *heap_mem,
                                                GrowableArray<ArchiveHeapOopmapInfo> *oopmaps,
-                                               int first_region_id, int max_num_regions,
-                                               bool print_log) {
+                                               int first_region_id, int max_num_regions) {
   assert(max_num_regions <= 2, "Only support maximum 2 memory regions");
 
   int arr_len = heap_mem == NULL ? 0 : heap_mem->length();
@@ -1221,10 +1229,8 @@ size_t FileMapInfo::write_archive_heap_regions(GrowableArray<MemRegion> *heap_me
       total_size += size;
     }
 
-    if (print_log) {
-      log_info(cds)("Archive heap region %d " INTPTR_FORMAT " - " INTPTR_FORMAT " = " SIZE_FORMAT_W(8) " bytes",
-                    i, p2i(start), p2i(start + size), size);
-    }
+    log_info(cds)("Archive heap region %d " INTPTR_FORMAT " - " INTPTR_FORMAT " = " SIZE_FORMAT_W(8) " bytes",
+                  i, p2i(start), p2i(start + size), size);
     write_region(i, start, size, false, false);
     if (size > 0) {
       space_at(i)->init_oopmap(oopmaps->at(arr_idx)._oopmap,
@@ -1237,14 +1243,13 @@ size_t FileMapInfo::write_archive_heap_regions(GrowableArray<MemRegion> *heap_me
 // Dump bytes to file -- at the current file position.
 
 void FileMapInfo::write_bytes(const void* buffer, size_t nbytes) {
-  if (_file_open) {
-    size_t n = os::write(_fd, buffer, (unsigned int)nbytes);
-    if (n != nbytes) {
-      // If the shared archive is corrupted, close it and remove it.
-      close();
-      remove(_full_path);
-      fail_stop("Unable to write to shared archive file.");
-    }
+  assert(_file_open, "must be");
+  size_t n = os::write(_fd, buffer, (unsigned int)nbytes);
+  if (n != nbytes) {
+    // If the shared archive is corrupted, close it and remove it.
+    close();
+    remove(_full_path);
+    fail_stop("Unable to write to shared archive file.");
   }
   _file_offset += nbytes;
 }
@@ -1257,20 +1262,17 @@ bool FileMapInfo::is_file_position_aligned() const {
 // Align file position to an allocation unit boundary.
 
 void FileMapInfo::align_file_position() {
+  assert(_file_open, "must be");
   size_t new_file_offset = align_up(_file_offset,
-                                         os::vm_allocation_granularity());
+                                    os::vm_allocation_granularity());
   if (new_file_offset != _file_offset) {
     _file_offset = new_file_offset;
-    if (_file_open) {
-      // Seek one byte back from the target and write a byte to insure
-      // that the written file is the correct length.
-      _file_offset -= 1;
-      if (lseek(_fd, (long)_file_offset, SEEK_SET) < 0) {
-        fail_stop("Unable to seek.");
-      }
-      char zero = 0;
-      write_bytes(&zero, 1);
-    }
+    // Seek one byte back from the target and write a byte to insure
+    // that the written file is the correct length.
+    _file_offset -= 1;
+    seek_to_position(_file_offset);
+    char zero = 0;
+    write_bytes(&zero, 1);
   }
 }
 

src/hotspot/share/memory/filemap.hpp

@@ -412,8 +412,7 @@ class FileMapInfo : public CHeapObj<mtInternal> {
                      bool read_only, bool allow_exec);
   size_t write_archive_heap_regions(GrowableArray<MemRegion> *heap_mem,
                                     GrowableArray<ArchiveHeapOopmapInfo> *oopmaps,
-                                    int first_region_id, int max_num_regions,
-                                    bool print_log);
+                                    int first_region_id, int max_num_regions);
   void  write_bytes(const void* buffer, size_t count);
   void  write_bytes_aligned(const void* buffer, size_t count);
   size_t  read_bytes(void* buffer, size_t count);
@@ -479,6 +478,7 @@ class FileMapInfo : public CHeapObj<mtInternal> {
   char* region_addr(int idx);
 
  private:
+  void  seek_to_position(size_t pos);
   char* skip_first_path_entry(const char* path) NOT_CDS_RETURN_(NULL);
   int   num_paths(const char* path) NOT_CDS_RETURN_(0);
   GrowableArray<const char*>* create_path_array(const char* path) NOT_CDS_RETURN_(NULL);

src/hotspot/share/memory/metaspaceShared.cpp

@@ -1137,7 +1137,9 @@ class VM_PopulateDumpSharedSpace: public VM_Operation {
 
   VMOp_Type type() const { return VMOp_PopulateDumpSharedSpace; }
   void doit();   // outline because gdb sucks
-  static void write_region(FileMapInfo* mapinfo, int region, DumpRegion* space, bool read_only,  bool allow_exec);
+  static void write_region(FileMapInfo* mapinfo, int region_idx, DumpRegion* dump_region, bool read_only,  bool allow_exec) {
+    mapinfo->write_region(region_idx, dump_region->base(), dump_region->used(), read_only, allow_exec);
+  }
   bool allow_nested_vm_operations() const { return true; }
 }; // class VM_PopulateDumpSharedSpace
 
@@ -1406,11 +1408,6 @@ DumpAllocStats* ArchiveCompactor::_alloc_stats;
 SortedSymbolClosure* ArchiveCompactor::_ssc;
 ArchiveCompactor::RelocationTable* ArchiveCompactor::_new_loc_table;
 
-void VM_PopulateDumpSharedSpace::write_region(FileMapInfo* mapinfo, int region_idx,
-                                              DumpRegion* dump_region, bool read_only,  bool allow_exec) {
-  mapinfo->write_region(region_idx, dump_region->base(), dump_region->used(), read_only, allow_exec);
-}
-
 void VM_PopulateDumpSharedSpace::dump_symbols() {
   tty->print_cr("Dumping symbol table ...");
 
@@ -1546,44 +1543,30 @@ void VM_PopulateDumpSharedSpace::doit() {
   mapinfo->set_read_only_tables_start(read_only_tables_start);
   mapinfo->set_misc_data_patching_start(vtbl_list);
   mapinfo->set_i2i_entry_code_buffers(MetaspaceShared::i2i_entry_code_buffers(),
-                                          MetaspaceShared::i2i_entry_code_buffers_size());
+                                      MetaspaceShared::i2i_entry_code_buffers_size());
   mapinfo->set_core_spaces_size(core_spaces_size);
+  mapinfo->open_for_write();
 
-  for (int pass=1; pass<=2; pass++) {
-    bool print_archive_log = (pass==1);
-    if (pass == 1) {
-      // The first pass doesn't actually write the data to disk. All it
-      // does is to update the fields in the mapinfo->_header.
-    } else {
-      // After the first pass, the contents of mapinfo->_header are finalized,
-      // so we can compute the header's CRC, and write the contents of the header
-      // and the regions into disk.
-      mapinfo->open_for_write();
-      mapinfo->set_header_crc(mapinfo->compute_header_crc());
-    }
-    mapinfo->write_header();
-
-    // NOTE: md contains the trampoline code for method entries, which are patched at run time,
-    // so it needs to be read/write.
-    write_region(mapinfo, MetaspaceShared::mc, &_mc_region, /*read_only=*/false,/*allow_exec=*/true);
-    write_region(mapinfo, MetaspaceShared::rw, &_rw_region, /*read_only=*/false,/*allow_exec=*/false);
-    write_region(mapinfo, MetaspaceShared::ro, &_ro_region, /*read_only=*/true, /*allow_exec=*/false);
-    write_region(mapinfo, MetaspaceShared::md, &_md_region, /*read_only=*/false,/*allow_exec=*/false);
+  // NOTE: md contains the trampoline code for method entries, which are patched at run time,
+  // so it needs to be read/write.
+  write_region(mapinfo, MetaspaceShared::mc, &_mc_region, /*read_only=*/false,/*allow_exec=*/true);
+  write_region(mapinfo, MetaspaceShared::rw, &_rw_region, /*read_only=*/false,/*allow_exec=*/false);
+  write_region(mapinfo, MetaspaceShared::ro, &_ro_region, /*read_only=*/true, /*allow_exec=*/false);
+  write_region(mapinfo, MetaspaceShared::md, &_md_region, /*read_only=*/false,/*allow_exec=*/false);
 
-    _total_closed_archive_region_size = mapinfo->write_archive_heap_regions(
+  _total_closed_archive_region_size = mapinfo->write_archive_heap_regions(
                                         _closed_archive_heap_regions,
                                         _closed_archive_heap_oopmaps,
                                         MetaspaceShared::first_closed_archive_heap_region,
-                                        MetaspaceShared::max_closed_archive_heap_region,
-                                        print_archive_log);
-    _total_open_archive_region_size = mapinfo->write_archive_heap_regions(
+                                        MetaspaceShared::max_closed_archive_heap_region);
+  _total_open_archive_region_size = mapinfo->write_archive_heap_regions(
                                         _open_archive_heap_regions,
                                         _open_archive_heap_oopmaps,
                                         MetaspaceShared::first_open_archive_heap_region,
-                                        MetaspaceShared::max_open_archive_heap_region,
-                                        print_archive_log);
-  }
+                                        MetaspaceShared::max_open_archive_heap_region);
 
+  mapinfo->set_header_crc(mapinfo->compute_header_crc());
+  mapinfo->write_header();
   mapinfo->close();
 
   // Restore the vtable in case we invoke any virtual methods.