[PATCH] [lld][Driver] Parallelize reading initial object files.

[Shankar Kalpathi Easwaran]

  I think it would be nice to have an option to not use any threads, for debugging / testing purposes. It could also be an option to benchmark / experiment things with number of threads spawned inside the thread pool.


================
Comment at: lib/ReaderWriter/ELF/ELFTargetInfo.cpp:95-120
@@ -94,22 +94,28 @@
 
 error_code ELFTargetInfo::parseFile(std::unique_ptr<MemoryBuffer> &mb,
                           std::vector<std::unique_ptr<File>> &result) const {
+  std::unique_lock<std::mutex> lock(_readerMutex);
   if (!_elfReader)
     _elfReader = createReaderELF(*this);
+  lock.unlock();
   error_code ec = _elfReader->parseFile(mb, result);
   if (ec) {
     // Not an ELF file, check file extension to see if it might be yaml
     StringRef path = mb->getBufferIdentifier();
-    if ( path.endswith(".objtxt") ) {
+    if (path.endswith(".objtxt")) {
+      lock.lock();
       if (!_yamlReader)
           _yamlReader = createReaderYAML(*this);
+      lock.unlock();
       ec = _yamlReader->parseFile(mb, result);
     }
     if (ec) {
       // Not a yaml file, assume it is a linkerscript
+      lock.lock();
       if (!_linkerScriptReader)
         _linkerScriptReader.reset(new ReaderLinkerScript(*this));
+      lock.unlock();
       ec = _linkerScriptReader->parseFile(mb, result);
     }
   }
   return ec;
----------------
I think we could create the _elfReader/_yamlReader/_linkerScriptReader as soon as ELFTargetInfo is initialized. Because after the first time the _elfReader/_yamlReader/_linkerScriptReader is initialized, you dont need the locks. This implementation will lock them and make it slower too, because of the locks / unlocks being done multiple times.

================
Comment at: lib/Driver/Driver.cpp:47
@@ +46,3 @@
+  size_t index = 0;
+  std::atomic<bool> fail(false);
+  TaskGroup tg;
----------------
Why you want this to be atomic bool ? because you are only setting the value to be true and the check/return is done only after all the threads have got synced ? 

Also it would be essential to check if there was a failure reading one of the files before reading the next set of files.

For example:

Say thread concurrency = 2

TG created 2 threads and there were 4 files

Say there was an error reading file 1 (or) file 2. You could stop file 3/4 from being read.

================
Comment at: lib/Driver/Driver.cpp:62-67
@@ +61,8 @@
+
+      std::unique_ptr<MemoryBuffer> buff(MemoryBuffer::getMemBuffer(
+          buffer->getBuffer(), buffer->getBufferIdentifier()));
+      if (error_code ec = targetInfo.parseFile(buff, files[index])) {
+        diagnostics << "Failed to read file: " << input.getPath()
+                    << ": " << ec.message() << "\n";
+        fail = true;
+        return;
----------------
Isnt using index a problem with archive files.

--force-load-archive option / multiple archive files getting pulled in ?

================
Comment at: lib/ReaderWriter/ELF/File.h:314
@@ -310,1 +313,3 @@
 
+    // Iterate over sections by object file position. The pointers point into
+    // the memory mapped file, so it's valid to sort based on them.
----------------
did you mean iterate over sections by their positions in the object file ?

================
Comment at: lib/ReaderWriter/ELF/File.h:315-325
@@ -311,2 +314,13 @@
+    // Iterate over sections by object file position. The pointers point into
+    // the memory mapped file, so it's valid to sort based on them.
+    std::vector<typename SectionSymbolsT::value_type *> secSyms;
+    secSyms.reserve(sectionSymbols.size());
     for (auto &i : sectionSymbols) {
-      auto &symbols = i.second;
+      secSyms.push_back(&i);
+    }
+    std::sort(secSyms.begin(), secSyms.end(),
+              [](const typename SectionSymbolsT::value_type *a,
+                 const typename SectionSymbolsT::value_type *b) {
+       return a->first < b->first;
+    });
+
----------------
Was there an issue earlier ?


http://llvm-reviews.chandlerc.com/D664