Use a non-recursive mutex in GsymCreator.

[simon.giesecke]

Summary

There doesn't seem to be a need to support recursive locking,
and a recursive mutex is unnecessarily inefficient.

[simon.giesecke]

GSYM conversion still spends a significant time for locking in GsymCreator::insertString even with only 8 threads, so lock contention seems to be generally an issue here. I was wondering if we could avoid locking the mutex here, but using thread-local tables until finalisation does not seem to be feasible since any thread might insert the same string IIUC and we want to have unique IDs at this point. A lock-free map might be an alternative here, though?

[grandinj]

In D102486#2759417, @simon.giesecke wrote:

GSYM conversion still spends a significant time for locking in GsymCreator::insertString even with only 8 threads, so lock contention seems to be generally an issue

Could perhaps start by moving the CachedHashStringRef construction outside the lock?

[clayborg]

In D102486#2760645, @grandinj wrote:

In D102486#2759417, @simon.giesecke wrote:

GSYM conversion still spends a significant time for locking in GsymCreator::insertString even with only 8 threads, so lock contention seems to be generally an issue

Could perhaps start by moving the CachedHashStringRef construction outside the lock?

The "Copy" argument is usually set to "false" so it won't end up copying the string. We only have to back the string up if the DWARF information had no mangled name for an entry and we and up calculating a fully qualified name by traversing the DWARF. This is rare though.

[clayborg]

In D102486#2759417, @simon.giesecke wrote:

GSYM conversion still spends a significant time for locking in GsymCreator::insertString even with only 8 threads, so lock contention seems to be generally an issue here. I was wondering if we could avoid locking the mutex here, but using thread-local tables until finalisation does not seem to be feasible since any thread might insert the same string IIUC and we want to have unique IDs at this point. A lock-free map might be an alternative here, though?

I am open to ideas to speed things up, but many things rely on strings being correctly uniqued: strings for function names and inlined function names, files in the file tables (which consist of two string: directory + basename).

[simon.giesecke]

In D102486#2760645, @grandinj wrote:

In D102486#2759417, @simon.giesecke wrote:

GSYM conversion still spends a significant time for locking in GsymCreator::insertString even with only 8 threads, so lock contention seems to be generally an issue

Could perhaps start by moving the CachedHashStringRef construction outside the lock?

Yes, makes sense. I have missed that opportunity somehow in D102484

[simon.giesecke]

In D102486#2760836, @clayborg wrote:

In D102486#2759417, @simon.giesecke wrote:

GSYM conversion still spends a significant time for locking in GsymCreator::insertString even with only 8 threads, so lock contention seems to be generally an issue here. I was wondering if we could avoid locking the mutex here, but using thread-local tables until finalisation does not seem to be feasible since any thread might insert the same string IIUC and we want to have unique IDs at this point. A lock-free map might be an alternative here, though?

I am open to ideas to speed things up, but many things rely on strings being correctly uniqued: strings for function names and inlined function names, files in the file tables (which consist of two string: directory + basename).

Sure, that's a property that must be maintained.

[simon.giesecke]

In D102486#2760834, @clayborg wrote:

In D102486#2760645, @grandinj wrote:

In D102486#2759417, @simon.giesecke wrote:

GSYM conversion still spends a significant time for locking in GsymCreator::insertString even with only 8 threads, so lock contention seems to be generally an issue

Could perhaps start by moving the CachedHashStringRef construction outside the lock?

The "Copy" argument is usually set to "false" so it won't end up copying the string. We only have to back the string up if the DWARF information had no mangled name for an entry and we and up calculating a fully qualified name by traversing the DWARF. This is rare though.

The "Copy" case might be rare, but the hash needs to be calculated regardless of whether "Copy" is true.

[simon.giesecke]

@clayborg Can you land the four patches again for me? Thanks!

[simon.giesecke]

In D102486#2762564, @simon.giesecke wrote:

@clayborg Can you land the four patches again for me? Thanks!

Or should I rather request commit access? I saw the other patch you landed now mentions you both as the author and the reviewer, which seems a bit odd.

[clayborg]

I would go ahead and request commit access, but at least there is the Phabricator link in the commit that shows that you authored the diff. This is the main reason you want to get commit access, so you everyone can easily see who did the patches.

[simon.giesecke]

Update using arc

llvm/include/llvm/DebugInfo/GSYM/GsymCreator.h

 ///
 /// The function info data is the payload that contains information about the
 /// address that is being looked up. It contains all of the encoded
 /// FunctionInfo objects. Each encoded FunctionInfo's data is pointed to by an
 /// entry in the Function Info Offsets Table. For details on the exact encoding
 /// of FunctionInfo objects, see "llvm/DebugInfo/GSYM/FunctionInfo.h".
 class GsymCreator {
   // Private member variables require Mutex protections
-  mutable std::recursive_mutex Mutex;
+  mutable std::mutex Mutex;
   std::vector<FunctionInfo> Funcs;
   StringTableBuilder StrTab;
   StringSet<> StringStorage;
   DenseMap<llvm::gsym::FileEntry, uint32_t> FileEntryToIndex;
   std::vector<llvm::gsym::FileEntry> Files;
   std::vector<uint8_t> UUID;
   Optional<AddressRanges> ValidTextRanges;
   AddressRanges Ranges;

llvm/lib/DebugInfo/GSYM/GsymCreator.cpp

uint32_t GsymCreator::insertFile(StringRef Path, llvm::sys::path::Style Style) {
   // We must insert the strings first, then call the FileEntry constructor.
   // If we inline the insertString() function call into the constructor, the
   // call order is undefined due to parameter lists not having any ordering
   // requirements.
   const uint32_t Dir = insertString(directory);
   const uint32_t Base = insertString(filename);
   FileEntry FE(Dir, Base);
 
-  std::lock_guard<std::recursive_mutex> Guard(Mutex);
+  std::lock_guard<std::mutex> Guard(Mutex);
   const auto NextIndex = Files.size();
   // Find FE in hash map and insert if not present.
   auto R = FileEntryToIndex.insert(std::make_pair(FE, NextIndex));
   if (R.second)
     Files.emplace_back(FE);
   return R.first->second;
 }
 
 llvm::Error GsymCreator::save(StringRef Path,
                               llvm::support::endianness ByteOrder) const {
   std::error_code EC;
   raw_fd_ostream OutStrm(Path, EC);
   if (EC)
     return llvm::errorCodeToError(EC);
   FileWriter O(OutStrm, ByteOrder);
   return encode(O);
 }
 
 llvm::Error GsymCreator::encode(FileWriter &O) const {
-  std::lock_guard<std::recursive_mutex> Guard(Mutex);
+  std::lock_guard<std::mutex> Guard(Mutex);
   if (Funcs.empty())
     return createStringError(std::errc::invalid_argument,
                              "no functions to encode");
   if (!Finalized)
     return createStringError(std::errc::invalid_argument,
                              "GsymCreator wasn't finalized prior to encoding");
 
   if (Funcs.size() > UINT32_MAX)
if (FirstIt != LastIt)
           PrevIt = FirstIt;
           *FirstIt++ = std::move(*CurrIt);
         }
   }
   return FirstIt;
 }
 
 llvm::Error GsymCreator::finalize(llvm::raw_ostream &OS) {
-  std::lock_guard<std::recursive_mutex> Guard(Mutex);
+  std::lock_guard<std::mutex> Guard(Mutex);
   if (Finalized)
     return createStringError(std::errc::invalid_argument, "already finalized");
   Finalized = true;
 
   // Sort function infos so we can emit sorted functions.
   llvm::sort(Funcs);
 
   // Don't let the string table indexes change by finalizing in order.

 }
 
 uint32_t GsymCreator::insertString(StringRef S, bool Copy) {
   if (S.empty())
     return 0;
 
   // The hash can be calculated outside the lock.
   CachedHashStringRef CHStr(S);
-  std::lock_guard<std::recursive_mutex> Guard(Mutex);
+  std::lock_guard<std::mutex> Guard(Mutex);
   if (Copy) {
     // We need to provide backing storage for the string if requested
     // since StringTableBuilder stores references to strings. Any string
     // that comes from a section in an object file doesn't need to be
     // copied, but any string created by code will need to be copied.
     // This allows GsymCreator to be really fast when parsing DWARF and
     // other object files as most strings don't need to be copied.
     if (!StrTab.contains(CHStr))
       CHStr = CachedHashStringRef{StringStorage.insert(S).first->getKey(),
                                   CHStr.hash()};
   }
   return StrTab.add(CHStr);
 }
 
 void GsymCreator::addFunctionInfo(FunctionInfo &&FI) {
-  std::lock_guard<std::recursive_mutex> Guard(Mutex);
+  std::lock_guard<std::mutex> Guard(Mutex);
   Ranges.insert(FI.Range);
   Funcs.emplace_back(std::move(FI));
 }
 
 void GsymCreator::forEachFunctionInfo(
     std::function<bool(FunctionInfo &)> const &Callback) {
-  std::lock_guard<std::recursive_mutex> Guard(Mutex);
+  std::lock_guard<std::mutex> Guard(Mutex);
   for (auto &FI : Funcs) {
     if (!Callback(FI))
       break;
   }
 }
 
 void GsymCreator::forEachFunctionInfo(
     std::function<bool(const FunctionInfo &)> const &Callback) const {
-  std::lock_guard<std::recursive_mutex> Guard(Mutex);
+  std::lock_guard<std::mutex> Guard(Mutex);
   for (const auto &FI : Funcs) {
     if (!Callback(FI))
       break;
   }
 }
 
 size_t GsymCreator::getNumFunctionInfos() const {
-  std::lock_guard<std::recursive_mutex> Guard(Mutex);
+  std::lock_guard<std::mutex> Guard(Mutex);
   return Funcs.size();
 }
 
 bool GsymCreator::IsValidTextAddress(uint64_t Addr) const {
   if (ValidTextRanges)
     return ValidTextRanges->contains(Addr);
   return true; // No valid text ranges has been set, so accept all ranges.
 }
 
 bool GsymCreator::hasFunctionInfoForAddress(uint64_t Addr) const {
-  std::lock_guard<std::recursive_mutex> Guard(Mutex);
+  std::lock_guard<std::mutex> Guard(Mutex);
   return Ranges.contains(Addr);
 }