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

 /// 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;

llvm/lib/DebugInfo/GSYM/GsymCreator.cpp

   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));

 }
 
 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");

 }
 
 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;

 
   // 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

 }
 
 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::hasFunctionInfoForAddress(uint64_t Addr) const {
-  std::lock_guard<std::recursive_mutex> Guard(Mutex);
+  std::lock_guard<std::mutex> Guard(Mutex);
   return Ranges.contains(Addr);
 }