Mmap and performance of gold

[Mikolaj Zalewski <mikolajz at google dot com>]

  Hi,
  I have seen a lot of CPU time spent inside mmap on my workstation.
To avoid blowing up the memory in 32-bit mode, gold tries to mmap only
the parts of the files that are needed, what generates lots of calls
to mmap small areas. Today, I've made a test by writing a quick patch
mmapping the whole file and not caring about alignment (it doesn't
make a difference on x86_64?) and made some measurements. Mmapping
once seems to speed up linking a 100MB, 64-bit C++ binary with gold
(values varied by some +/-0.1s between runs. I've taken the median of
three runs):

64-bit build -O2:
Mmapping all files:
real	0m3.640s
user	0m2.884s
sys	0m0.716s

Mmaping selected regions:
real	0m5.318s
user	0m2.984s
sys	0m2.312s

32-bit build -O2:
Mmapping all files:
real	0m3.787s
user	0m3.120s
sys	0m0.608s

Mmaping selected regions:
real	0m5.422s
user	0m3.124s
sys	0m2.240s

Does the attached patch make a similar difference on other systems, or
is there something strange with my kernel? Maybe we should mmap whole
files on 64-bit systems or add a command line options with a default
depending on whether the system is 64-bit. My current patch works on
architectures that don't care about alignment, but this could be
improved (the benchmark was done on a build using thin archives, so
probably the alignment change didn't affect the results in either
way).

MikoÅaj

diff --git a/gold/fileread.cc b/gold/fileread.cc
index bb10aa9..2c6cabd 100644
--- a/gold/fileread.cc
+++ b/gold/fileread.cc
@@ -50,13 +50,13 @@ File_read::View::~View()
   gold_assert(!this->is_locked());
   if (!this->mapped_)
     delete[] this->data_;
-  else
+/*  else
     {
       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_;
-    }
+    }*/
 }
 
 void
@@ -88,6 +88,11 @@ unsigned long long File_read::maximum_mapped_bytes;
 File_read::~File_read()
 {
   gold_assert(this->token_.is_writable());
+  if (contents_mmapped_) {
+    if (::munmap(const_cast<unsigned char*>(this->contents_), this->size_) != 0)
+      gold_warning(_("munmap failed: %s"), strerror(errno));
+    File_read::current_mapped_bytes -= this->size_;
+  }
   if (this->is_descriptor_opened_)
     {
       release_descriptor(this->descriptor_, true);
@@ -122,6 +127,15 @@ File_read::open(const Task* task, const std::string& name)
       this->size_ = s.st_size;
       gold_debug(DEBUG_FILES, "Attempt to open %s succeeded",
                  this->name_.c_str());
+      this->contents_ = static_cast<const unsigned char*>(
+          ::mmap(NULL, this->size_, PROT_READ, MAP_PRIVATE,
+                 this->descriptor_, 0));
+      if (this->contents_ == MAP_FAILED)
+        gold_fatal(_("%s: mmap failed: %s"),
+                   this->filename().c_str(),
+                   strerror(errno));
+      this->contents_mmapped_ = true;
+      this->mapped_bytes_ += this->size_;
 
       this->token_.add_writer(task);
     }
@@ -231,13 +245,13 @@ File_read::is_locked() const
 // not for the requested BYTESHIFT.
 
 inline File_read::View*
-File_read::find_view(off_t start, section_size_type size,
-		     unsigned int byteshift, File_read::View** vshifted) const
+File_read::find_view(off_t /*start*/, section_size_type /*size*/,
+		     unsigned int /*byteshift*/, File_read::View** vshifted) const
 {
   if (vshifted != NULL)
     *vshifted = NULL;
 
-  off_t page = File_read::page_offset(start);
+/*  off_t page = File_read::page_offset(start);
 
   unsigned int bszero = 0;
   Views::const_iterator p = this->views_.upper_bound(std::make_pair(page - 1,
@@ -260,7 +274,7 @@ File_read::find_view(off_t start, section_size_type size,
 	}
 
       ++p;
-    }
+    }*/
 
   return NULL;
 }
@@ -352,7 +366,7 @@ File_read::add_view(File_read::View* v)
 
 File_read::View*
 File_read::make_view(off_t start, section_size_type size,
-		     unsigned int byteshift, bool cache)
+		     unsigned int /*byteshift*/, bool cache)
 {
   gold_assert(size > 0);
 
@@ -366,7 +380,7 @@ File_read::make_view(off_t start, section_size_type size,
 		   static_cast<long long>(size),
 		   static_cast<long long>(start));
 
-  off_t poff = File_read::page_offset(start);
+/*  off_t poff = File_read::page_offset(start);
 
   section_size_type psize = File_read::pages(size + (start - poff));
 
@@ -404,7 +418,10 @@ File_read::make_view(off_t start, section_size_type size,
 
   this->add_view(v);
 
-  return v;
+  return v;*/
+  gold_assert(contents_ != NULL);
+  return new File_read::View(0, this->size_, this->contents_, 0,
+                             cache, true);
 }
 
 // Find a View or make a new one, shifted as required by the file
@@ -776,7 +793,7 @@ File_read::get_mtime()
 {
   struct stat file_stat;
   this->reopen_descriptor();
-  
+
   if (fstat(this->descriptor_, &file_stat) < 0)
     gold_fatal(_("%s: stat failed: %s"), this->name_.c_str(),
 	       strerror(errno));
diff --git a/gold/fileread.h b/gold/fileread.h
index 920a4da..e63ebb1 100644
--- a/gold/fileread.h
+++ b/gold/fileread.h
@@ -66,7 +66,7 @@ class File_read
   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)
+      contents_mmapped_(false), mapped_bytes_(0), released_(true)
   { }
 
   ~File_read();
@@ -207,7 +207,7 @@ class File_read
     this->reopen_descriptor();
     return this->descriptor_;
   }
-  
+
   // Return the file last modification time.  Calls gold_fatal if the stat
   // system call failed.
   Timespec
@@ -396,6 +396,8 @@ class File_read
   Saved_views saved_views_;
   // Specified file contents.  Used only for testing purposes.
   const unsigned char* contents_;
+  // Whether the CONTENTS_ is mapped and should be unmapped.
+  bool contents_mmapped_;
   // 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.