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

[Michael Spencer]

================
Comment at: lib/Driver/Driver.cpp:47
@@ +46,3 @@
+  size_t index = 0;
+  std::atomic<bool> fail(false);
+  TaskGroup tg;
----------------
Shankar Kalpathi Easwaran wrote:
> 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.
It has to be atomic because it could be written from multiple threads.

I don't see why it's essential to early exit the other tasks. It is indeed possible though. Just check the value of fail.

================
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;
----------------
Shankar Kalpathi Easwaran wrote:
> Isnt using index a problem with archive files.
> 
> --force-load-archive option / multiple archive files getting pulled in ?
files is a std::vector<std::vector<std::unique_ptr<File>>>. Each input argument gets a std::vector.

================
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;
----------------
Shankar Kalpathi Easwaran wrote:
> 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.
k

================
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.
----------------
Shankar Kalpathi Easwaran wrote:
> did you mean iterate over sections by their positions in the object file ?
Yes, that's what that says.

================
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;
+    });
+
----------------
Shankar Kalpathi Easwaran wrote:
> Was there an issue earlier ?
I changed sectionSymbols to a DenseMap instead of a std::map. This is required because DenseMap is unordered.


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