Re: [PATCH] gold: add an option to mmap whole files (defaults to true on 64-bit linkers)

[Ian Lance Taylor <iant at google dot com>]

Mikolaj Zalewski <mikolajz@google.com> writes:

>   I have created a patch adding an option to mmap whole file in
> File_read::open and making it default on 64-bit linker builds. This
> seems to speed up large builds. The changes were very similar to the
> already existing support to open pseudo-files (for tests) with
> contents passed as an argument, so I've merged both features.

Thanks for pursuing this.

The name --mmap-whole-files makes sense but is not a good choice
because the existing -m option takes an argument and the linker has
historical confusion between single-dash and double-dash options.  I
changed it to --keep-files-mapped with an eye on the GNU ld
--keep-memory option (not currently implemented in gold).

I tweaked your patch to use a switch in File_read::View::~View.

Committed as follows.

Ian


2009-10-27  Mikolaj Zalewski  <mikolajz@google.com>

	* fileread.cc: (File_read::View::~View): Use the new
	data_ownership_ filed.
	(File_read::~File_read): Dispose the new whole_file_view_.
	(File_read::open): Mmap the whole file if needed.
	(File_read::open): Use whole_file_view_ instead of contents_.
	(File_read::find_view): Use whole_file_view_ if applicable.
	(File_read::do_read): Use whole_file_view_ instead of contents_.
	(File_read::make_view): Use whole_file_view_ instead of contents_,
	update File_read::View::View call.
	(File_read::find_or_make_view): Update File_read::View::View
	call.
	* fileread.h: (File_read::File_read): Initialize whole_file_view_,
	remove contents_
	(File_read::View::Data_ownership): New enum.
	(File_read::View::View): Replace bool mapped_ with Data_ownership
	argument.
	(File_read::View::mapped_): Remove (replaced by data_ownership_).
	(File_read::View::data_ownership_): New field.
	(File_read::contents_): Remove (replaced by whole_file_view_).
	(File_read::whole_file_view_): New field.
	* options.h (class General_options): Add --keep-files-mapped.


Index: fileread.cc
===================================================================
RCS file: /cvs/src/src/gold/fileread.cc,v
retrieving revision 1.55
diff -p -u -r1.55 fileread.cc
--- fileread.cc	10 Oct 2009 07:39:04 -0000	1.55
+++ fileread.cc	28 Oct 2009 00:39:43 -0000
@@ -57,14 +57,20 @@ namespace gold
 File_read::View::~View()
 {
   gold_assert(!this->is_locked());
-  if (!this->mapped_)
-    delete[] this->data_;
-  else
+  switch (this->data_ownership_)
     {
+    case DATA_ALLOCATED_ARRAY:
+      delete[] this->data_;
+      break;
+    case DATA_MMAPPED:
       if (::munmap(const_cast<unsigned char*>(this->data_), this->size_) != 0)
         gold_warning(_("munmap failed: %s"), strerror(errno));
-
       File_read::current_mapped_bytes -= this->size_;
+      break;
+    case DATA_NOT_OWNED:
+      break;
+    default:
+      gold_unreachable();
     }
 }
 
@@ -105,6 +111,8 @@ File_read::~File_read()
     }
   this->name_.clear();
   this->clear_views(true);
+  if (this->whole_file_view_)
+    delete this->whole_file_view_;
 }
 
 // Open the file.
@@ -132,6 +140,22 @@ File_read::open(const Task* task, const 
       gold_debug(DEBUG_FILES, "Attempt to open %s succeeded",
                  this->name_.c_str());
 
+      // Options may not yet be ready e.g. when reading a version
+      // script.  We then default to --no-keep-files-mapped.
+      if (parameters->options_valid()
+	  && parameters->options().keep_files_mapped())
+        {
+          const unsigned char* contents = static_cast<const unsigned char*>(
+              ::mmap(NULL, this->size_, PROT_READ, MAP_PRIVATE,
+                     this->descriptor_, 0));
+          if (contents == MAP_FAILED)
+            gold_fatal(_("%s: mmap failed: %s"), this->filename().c_str(),
+                       strerror(errno));
+          this->whole_file_view_ = new View(0, this->size_, contents, 0, false,
+                                            View::DATA_MMAPPED);
+          this->mapped_bytes_ += this->size_;
+        }
+
       this->token_.add_writer(task);
     }
 
@@ -149,7 +173,8 @@ File_read::open(const Task* task, const 
 	      && !this->is_descriptor_opened_
 	      && this->name_.empty());
   this->name_ = name;
-  this->contents_ = contents;
+  this->whole_file_view_ = new View(0, size, contents, 0, false,
+                                    View::DATA_NOT_OWNED);
   this->size_ = size;
   this->token_.add_writer(task);
   return true;
@@ -246,6 +271,12 @@ File_read::find_view(off_t start, sectio
   if (vshifted != NULL)
     *vshifted = NULL;
 
+  // If we have the whole file mmapped, and the alignment is right,
+  // we can return it.
+  if (this->whole_file_view_)
+    if (byteshift == -1U || byteshift == 0)
+      return this->whole_file_view_;
+
   off_t page = File_read::page_offset(start);
 
   unsigned int bszero = 0;
@@ -281,12 +312,12 @@ void
 File_read::do_read(off_t start, section_size_type size, void* p)
 {
   ssize_t bytes;
-  if (this->contents_ != NULL)
+  if (this->whole_file_view_ != NULL)
     {
       bytes = this->size_ - start;
       if (static_cast<section_size_type>(bytes) >= size)
 	{
-	  memcpy(p, this->contents_ + start, size);
+	  memcpy(p, this->whole_file_view_->data() + start, size);
 	  return;
 	}
     }
@@ -386,12 +417,13 @@ File_read::make_view(off_t start, sectio
     }
 
   File_read::View* v;
-  if (this->contents_ != NULL || byteshift != 0)
+  if (this->whole_file_view_ != NULL || byteshift != 0)
     {
       unsigned char* p = new unsigned char[psize + byteshift];
       memset(p, 0, byteshift);
       this->do_read(poff, psize, p + byteshift);
-      v = new File_read::View(poff, psize, p, byteshift, cache, false);
+      v = new File_read::View(poff, psize, p, byteshift, cache,
+                              View::DATA_ALLOCATED_ARRAY);
     }
   else
     {
@@ -408,7 +440,8 @@ File_read::make_view(off_t start, sectio
       this->mapped_bytes_ += psize;
 
       const unsigned char* pbytes = static_cast<const unsigned char*>(p);
-      v = new File_read::View(poff, psize, pbytes, 0, cache, true);
+      v = new File_read::View(poff, psize, pbytes, 0, cache,
+                              View::DATA_MMAPPED);
     }
 
   this->add_view(v);
@@ -463,9 +496,9 @@ File_read::find_or_make_view(off_t offse
       memset(pbytes, 0, byteshift);
       memcpy(pbytes + byteshift, v->data() + v->byteshift(), v->size());
 
-      File_read::View* shifted_view = new File_read::View(v->start(), v->size(),
-							  pbytes, byteshift,
-							  cache, false);
+      File_read::View* shifted_view =
+          new File_read::View(v->start(), v->size(), pbytes, byteshift,
+			      cache, View::DATA_ALLOCATED_ARRAY);
 
       this->add_view(shifted_view);
       return shifted_view;
Index: fileread.h
===================================================================
RCS file: /cvs/src/src/gold/fileread.h,v
retrieving revision 1.37
diff -p -u -r1.37 fileread.h
--- fileread.h	9 Oct 2009 23:20:44 -0000	1.37
+++ fileread.h	28 Oct 2009 00:39:44 -0000
@@ -65,8 +65,8 @@ class File_read
  public:
   File_read()
     : name_(), descriptor_(-1), is_descriptor_opened_(false), object_count_(0),
-      size_(0), token_(false), views_(), saved_views_(), contents_(NULL),
-      mapped_bytes_(0), released_(true)
+      size_(0), token_(false), views_(), saved_views_(), mapped_bytes_(0),
+      released_(true), whole_file_view_(NULL)
   { }
 
   ~File_read();
@@ -234,10 +234,23 @@ class File_read
   class View
   {
    public:
+    // Specifies how to dispose the data on destruction of the view.
+    enum Data_ownership
+    {
+      // Data owned by File object - nothing done in destructor.
+      DATA_NOT_OWNED,
+      // Data alocated with new[] and owned by this object - should
+      // use delete[].
+      DATA_ALLOCATED_ARRAY,
+      // Data mmapped and owned by this object - should munmap.
+      DATA_MMAPPED
+    };
+
     View(off_t start, section_size_type size, const unsigned char* data,
-	 unsigned int byteshift, bool cache, bool mapped)
+	 unsigned int byteshift, bool cache, Data_ownership data_ownership)
       : start_(start), size_(size), data_(data), lock_count_(0),
-	byteshift_(byteshift), cache_(cache), mapped_(mapped), accessed_(true)
+	byteshift_(byteshift), cache_(cache), data_ownership_(data_ownership),
+	accessed_(true)
     { }
 
     ~View();
@@ -311,7 +324,7 @@ class File_read
     bool cache_;
     // Whether the view is mapped into memory.  If not, data_ points
     // to memory allocated using new[].
-    bool mapped_;
+    Data_ownership data_ownership_;
     // Whether the view has been accessed recently.
     bool accessed_;
   };
@@ -400,14 +413,18 @@ class File_read
   // List of views which were locked but had to be removed from views_
   // because they were not large enough.
   Saved_views saved_views_;
-  // Specified file contents.  Used only for testing purposes.
-  const unsigned char* contents_;
   // Total amount of space mapped into memory.  This is only changed
   // while the file is locked.  When we unlock the file, we transfer
   // the total to total_mapped_bytes, and reset this to zero.
   size_t mapped_bytes_;
   // Whether the file was released.
   bool released_;
+  // A view containing the whole file.  May be NULL if we mmap only
+  // the relevant parts of the file.  Not NULL if:
+  // - Flag --mmap_whole_files is set (default on 64-bit hosts).
+  // - The contents was specified in the constructor.  Used only for
+  //   testing purposes).
+  View* whole_file_view_;
 };
 
 // A view of file data that persists even when the file is unlocked.
Index: options.h
===================================================================
RCS file: /cvs/src/src/gold/options.h,v
retrieving revision 1.114
diff -p -u -r1.114 options.h
--- options.h	14 Oct 2009 05:25:02 -0000	1.114
+++ options.h	28 Oct 2009 00:39:44 -0000
@@ -723,6 +723,12 @@ class General_options
   DEFINE_special(just_symbols, options::TWO_DASHES, '\0',
                  N_("Read only symbol values from FILE"), N_("FILE"));
 
+  DEFINE_bool(keep_files_mapped, options::TWO_DASHES, '\0',
+	      sizeof(void*) >= 8,
+              N_("Map whole files to memory (default on 64-bit hosts)"),
+              N_("Map relevant file parts to memory (default on 32-bit "
+                 "hosts)"));
+
   DEFINE_special(library, options::TWO_DASHES, 'l',
                  N_("Search for library LIBNAME"), N_("LIBNAME"));