[lldb] Make StatsDuration thread-safe
authorPavel Labath <pavel@labath.sk>
Mon, 17 Jan 2022 14:04:30 +0000 (15:04 +0100)
committerPavel Labath <pavel@labath.sk>
Wed, 19 Jan 2022 15:42:53 +0000 (16:42 +0100)
std::chrono::duration types are not thread-safe, and they cannot be
concurrently updated from multiple threads. Currently, we were doing
such a thing (only) in the DWARF indexing code
(DWARFUnit::ExtractDIEsRWLocked), but I think it can easily happen that
someone else tries to update another statistic like this without
bothering to check for thread safety.

This patch changes the StatsDuration type from a simple typedef into a
class in its own right. The class stores the duration internally as
std::atomic<uint64_t> (so it can be updated atomically), but presents it
to its users as the usual chrono type (duration<float>).

Differential Revision: https://reviews.llvm.org/D117474

lldb/include/lldb/Breakpoint/Breakpoint.h
lldb/include/lldb/Core/Module.h
lldb/include/lldb/Symbol/SymbolFile.h
lldb/include/lldb/Target/Statistics.h
lldb/source/Breakpoint/Breakpoint.cpp
lldb/source/Plugins/SymbolFile/DWARF/DWARFIndex.h
lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.cpp
lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.h
lldb/source/Target/Statistics.cpp

index 40435d5..113f8c4 100644 (file)
@@ -581,7 +581,7 @@ public:
   llvm::json::Value GetStatistics();
 
   /// Get the time it took to resolve all locations in this breakpoint.
-  StatsDuration GetResolveTime() const { return m_resolve_time; }
+  StatsDuration::Duration GetResolveTime() const { return m_resolve_time; }
 
 protected:
   friend class Target;
@@ -660,7 +660,7 @@ private:
 
   BreakpointName::Permissions m_permissions;
 
-  StatsDuration m_resolve_time{0.0};
+  StatsDuration m_resolve_time;
 
   void SendBreakpointChangedEvent(lldb::BreakpointEventType eventKind);
 
index d53481a..f6c3258 100644 (file)
@@ -1047,11 +1047,11 @@ protected:
   /// We store a symbol table parse time duration here because we might have
   /// an object file and a symbol file which both have symbol tables. The parse
   /// time for the symbol tables can be aggregated here.
-  StatsDuration m_symtab_parse_time{0.0};
+  StatsDuration m_symtab_parse_time;
   /// We store a symbol named index time duration here because we might have
   /// an object file and a symbol file which both have symbol tables. The parse
   /// time for the symbol tables can be aggregated here.
-  StatsDuration m_symtab_index_time{0.0};
+  StatsDuration m_symtab_index_time;
 
   /// Resolve a file or load virtual address.
   ///
index 288576b..6cdb2bf 100644 (file)
@@ -316,14 +316,14 @@ public:
   ///
   /// \returns 0.0 if no information has been parsed or if there is
   /// no computational cost to parsing the debug information.
-  virtual StatsDuration GetDebugInfoParseTime() { return StatsDuration(0.0); }
+  virtual StatsDuration::Duration GetDebugInfoParseTime() { return {}; }
 
   /// Return the time it took to index the debug information in the object
   /// file.
   ///
   /// \returns 0.0 if the file doesn't need to be indexed or if it
   /// hasn't been indexed yet, or a valid duration if it has.
-  virtual StatsDuration GetDebugInfoIndexTime() { return StatsDuration(0.0); }
+  virtual StatsDuration::Duration GetDebugInfoIndexTime() { return {}; }
 
   /// Accessors for the bool that indicates if the debug info index was loaded
   /// from, or saved to the module index cache.
index cf4fb83..dbf3554 100644 (file)
@@ -9,20 +9,39 @@
 #ifndef LLDB_TARGET_STATISTICS_H
 #define LLDB_TARGET_STATISTICS_H
 
-#include <chrono>
-#include <string>
-#include <vector>
-
 #include "lldb/Utility/Stream.h"
 #include "lldb/lldb-forward.h"
 #include "llvm/Support/JSON.h"
+#include <atomic>
+#include <chrono>
+#include <string>
+#include <vector>
 
 namespace lldb_private {
 
 using StatsClock = std::chrono::high_resolution_clock;
-using StatsDuration = std::chrono::duration<double>;
 using StatsTimepoint = std::chrono::time_point<StatsClock>;
 
+class StatsDuration {
+public:
+  using Duration = std::chrono::duration<double>;
+
+  Duration get() const {
+    return Duration(InternalDuration(value.load(std::memory_order_relaxed)));
+  }
+  operator Duration() const { return get(); }
+
+  StatsDuration &operator+=(Duration dur) {
+    value.fetch_add(std::chrono::duration_cast<InternalDuration>(dur).count(),
+                    std::memory_order_relaxed);
+    return *this;
+  }
+
+private:
+  using InternalDuration = std::chrono::duration<uint64_t, std::micro>;
+  std::atomic<uint64_t> value{0};
+};
+
 /// A class that measures elapsed time in an exception safe way.
 ///
 /// This is a RAII class is designed to help gather timing statistics within
@@ -54,7 +73,7 @@ public:
     m_start_time = StatsClock::now();
   }
   ~ElapsedTime() {
-    StatsDuration elapsed = StatsClock::now() - m_start_time;
+    StatsClock::duration elapsed = StatsClock::now() - m_start_time;
     m_elapsed_time += elapsed;
   }
 };
@@ -104,7 +123,7 @@ public:
   StatsSuccessFail &GetFrameVariableStats() { return m_frame_var; }
 
 protected:
-  StatsDuration m_create_time{0.0};
+  StatsDuration m_create_time;
   llvm::Optional<StatsTimepoint> m_launch_or_attach_time;
   llvm::Optional<StatsTimepoint> m_first_private_stop_time;
   llvm::Optional<StatsTimepoint> m_first_public_stop_time;
index d6acf65..eeb8eac 100644 (file)
@@ -1094,7 +1094,7 @@ Breakpoint::BreakpointEventData::GetBreakpointLocationAtIndexFromEvent(
 json::Value Breakpoint::GetStatistics() {
   json::Object bp;
   bp.try_emplace("id", GetID());
-  bp.try_emplace("resolveTime", m_resolve_time.count());
+  bp.try_emplace("resolveTime", m_resolve_time.get().count());
   bp.try_emplace("numLocations", (int64_t)GetNumLocations());
   bp.try_emplace("numResolvedLocations", (int64_t)GetNumResolvedLocations());
   bp.try_emplace("internal", IsInternal());
index 1d3d70d..c4995e7 100644 (file)
@@ -64,11 +64,11 @@ public:
 
   virtual void Dump(Stream &s) = 0;
 
-  StatsDuration GetIndexTime() { return m_index_time; }
+  StatsDuration::Duration GetIndexTime() { return m_index_time; }
 
 protected:
   Module &m_module;
-  StatsDuration m_index_time{0.0};
+  StatsDuration m_index_time;
 
   /// Helper function implementing common logic for processing function dies. If
   /// the function given by "ref" matches search criteria given by
index ca701c6..02d1a6a 100644 (file)
@@ -4086,8 +4086,8 @@ LanguageType SymbolFileDWARF::GetLanguageFamily(DWARFUnit &unit) {
   return LanguageTypeFromDWARF(lang);
 }
 
-StatsDuration SymbolFileDWARF::GetDebugInfoIndexTime() {
+StatsDuration::Duration SymbolFileDWARF::GetDebugInfoIndexTime() {
   if (m_index)
     return m_index->GetIndexTime();
-  return StatsDuration(0.0);
+  return {};
 }
index e81ce28..f84a786 100644 (file)
@@ -319,10 +319,10 @@ public:
   /// Same as GetLanguage() but reports all C++ versions as C++ (no version).
   static lldb::LanguageType GetLanguageFamily(DWARFUnit &unit);
 
-  lldb_private::StatsDuration GetDebugInfoParseTime() override {
+  lldb_private::StatsDuration::Duration GetDebugInfoParseTime() override {
     return m_parse_time;
   }
-  lldb_private::StatsDuration GetDebugInfoIndexTime() override;
+  lldb_private::StatsDuration::Duration GetDebugInfoIndexTime() override;
 
   lldb_private::StatsDuration &GetDebugInfoParseTimeRef() {
     return m_parse_time;
@@ -559,7 +559,7 @@ protected:
   /// Try to filter out this debug info by comparing it to the lowest code
   /// address in the module.
   lldb::addr_t m_first_code_address = LLDB_INVALID_ADDRESS;
-  lldb_private::StatsDuration m_parse_time{0.0};
+  lldb_private::StatsDuration m_parse_time;
 };
 
 #endif // LLDB_SOURCE_PLUGINS_SYMBOLFILE_DWARF_SYMBOLFILEDWARF_H
index 2491f6a..6ee189e 100644 (file)
@@ -1447,8 +1447,8 @@ uint64_t SymbolFileDWARFDebugMap::GetDebugInfoSize() {
   return debug_info_size;
 }
 
-StatsDuration SymbolFileDWARFDebugMap::GetDebugInfoParseTime() {
-  StatsDuration elapsed(0.0);
+StatsDuration::Duration SymbolFileDWARFDebugMap::GetDebugInfoParseTime() {
+  StatsDuration::Duration elapsed(0.0);
   ForEachSymbolFile([&](SymbolFileDWARF *oso_dwarf) -> bool {
     ObjectFile *oso_objfile = oso_dwarf->GetObjectFile();
     if (oso_objfile) {
@@ -1464,8 +1464,8 @@ StatsDuration SymbolFileDWARFDebugMap::GetDebugInfoParseTime() {
   return elapsed;
 }
 
-StatsDuration SymbolFileDWARFDebugMap::GetDebugInfoIndexTime() {
-  StatsDuration elapsed(0.0);
+StatsDuration::Duration SymbolFileDWARFDebugMap::GetDebugInfoIndexTime() {
+  StatsDuration::Duration elapsed(0.0);
   ForEachSymbolFile([&](SymbolFileDWARF *oso_dwarf) -> bool {
     ObjectFile *oso_objfile = oso_dwarf->GetObjectFile();
     if (oso_objfile) {
index 74f3244..2a6232a 100644 (file)
@@ -143,8 +143,8 @@ public:
   llvm::StringRef GetPluginName() override { return GetPluginNameStatic(); }
 
   uint64_t GetDebugInfoSize() override;
-  lldb_private::StatsDuration GetDebugInfoParseTime() override;
-  lldb_private::StatsDuration GetDebugInfoIndexTime() override;
+  lldb_private::StatsDuration::Duration GetDebugInfoParseTime() override;
+  lldb_private::StatsDuration::Duration GetDebugInfoIndexTime() override;
 
 protected:
   enum { kHaveInitializedOSOs = (1 << 0), kNumFlags };
index d50343f..8d1e982 100644 (file)
@@ -34,7 +34,8 @@ json::Value StatsSuccessFail::ToJSON() const {
 }
 
 static double elapsed(const StatsTimepoint &start, const StatsTimepoint &end) {
-  StatsDuration elapsed = end.time_since_epoch() - start.time_since_epoch();
+  StatsDuration::Duration elapsed =
+      end.time_since_epoch() - start.time_since_epoch();
   return elapsed.count();
 }
 
@@ -86,7 +87,8 @@ json::Value TargetStats::ToJSON(Target &target) {
         elapsed(*m_launch_or_attach_time, *m_first_public_stop_time);
     target_metrics_json.try_emplace("firstStopTime", elapsed_time);
   }
-  target_metrics_json.try_emplace("targetCreateTime", m_create_time.count());
+  target_metrics_json.try_emplace("targetCreateTime",
+                                  m_create_time.get().count());
 
   json::Array breakpoints_array;
   double totalBreakpointResolveTime = 0.0;
@@ -177,8 +179,8 @@ llvm::json::Value DebuggerStats::ReportStatistics(Debugger &debugger,
     }
     module_stat.uuid = module->GetUUID().GetAsString();
     module_stat.triple = module->GetArchitecture().GetTriple().str();
-    module_stat.symtab_parse_time = module->GetSymtabParseTime().count();
-    module_stat.symtab_index_time = module->GetSymtabIndexTime().count();
+    module_stat.symtab_parse_time = module->GetSymtabParseTime().get().count();
+    module_stat.symtab_index_time = module->GetSymtabIndexTime().get().count();
     Symtab *symtab = module->GetSymtab();
     if (symtab) {
       module_stat.symtab_loaded_from_cache = symtab->GetWasLoadedFromCache();