[lldb] Refactor statistics (NFC)

[JDevlieghere]

Summary

Refactor LLDB's current statistics into a single class named Analytics which consists of different metrics.

[aprantl]

Mechanically, this sgtm.

[clayborg]

please don't commit this. I am in the process of doing very similar stuff in https://reviews.llvm.org/D110804

[clayborg]

I will have the new version that implements many of these changes submitted by tonight

[labath]

Some general thoughts:

• instead of throwing patches around in might be good to have a discussion thread to determine all the requirements and figure out the general direction
• it would be better to have a series of smaller patches (like this one) instead one big patch implementing everything

lldb/include/lldb/Utility/Analytics.h

43

This assumes all statistics can be represented as integers. Here's an idea how that could be done with a flexible type representation https://godbolt.org/z/j6f3bhP8P.

lldb/source/Utility/Analytics.cpp

40–41

You could get rid of these if you made MaxID a synonym for the highest valued entry.

[aprantl]

In D110893#3035753, @labath wrote:

Some general thoughts:

• instead of throwing patches around in might be good to have a discussion thread to determine all the requirements and figure out the general direction

Got to agree with Pavel here :-)

[Sorry, I missed the context in https://reviews.llvm.org/D110804, I should read through my reviews in chronological order.]

lldb/include/lldb/Target/Target.h

 #include "lldb/Core/UserSettingsController.h"
 #include "lldb/Expression/Expression.h"
 #include "lldb/Host/ProcessLaunchInfo.h"
 #include "lldb/Symbol/TypeSystem.h"
 #include "lldb/Target/ExecutionContextScope.h"
 #include "lldb/Target/PathMappingList.h"
 #include "lldb/Target/SectionLoadHistory.h"
 #include "lldb/Target/ThreadSpec.h"
+#include "lldb/Utility/Analytics.h"
 #include "lldb/Utility/ArchSpec.h"
 #include "lldb/Utility/Broadcaster.h"
 #include "lldb/Utility/LLDBAssert.h"
 #include "lldb/Utility/Timeout.h"
 #include "lldb/lldb-public.h"
 
 namespace lldb_private {
 
protected:
   /// Stores the frame recognizers of this target.
   lldb::StackFrameRecognizerManagerUP m_frame_recognizer_manager_up;
 
   static void ImageSearchPathsChanged(const PathMappingList &path_list,
                                       void *baton);
 
   // Utilities for `statistics` command.
 private:
-  std::vector<uint32_t> m_stats_storage;
-  bool m_collecting_stats = false;
+  Analytics m_analytics;
 
 public:
-  void SetCollectingStats(bool v) { m_collecting_stats = v; }
+  void SetCollectingStats(bool v) { m_analytics.SetEnabled(v); }
 
-  bool GetCollectingStats() { return m_collecting_stats; }
+  bool GetCollectingStats() { return m_analytics.IsEnabled(); }
 
-  void IncrementStats(lldb_private::StatisticKind key) {
-    if (!GetCollectingStats())
-      return;
-    lldbassert(key < lldb_private::StatisticKind::StatisticMax &&
-               "invalid statistics!");
-    m_stats_storage[key] += 1;
-  }
-
-  std::vector<uint32_t> GetStatistics() { return m_stats_storage; }
+  Analytics &GetAnalytics() { return m_analytics; }
 
 private:
   /// Construct with optional file and arch.
   ///
   /// This member is private. Clients must use
   /// TargetList::CreateTarget(const FileSpec*, const ArchSpec*)
   /// so all targets can be tracked from the central target list.
   ///

lldb/include/lldb/Utility/Analytics.h

+//===-- Analytics.h ---------------------------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLDB_UTILITY_ANALYTICS_H
+#define LLDB_UTILITY_ANALYTICS_H
+
+#include "lldb/Utility/StructuredData.h"
+
+#include <vector>
+
+namespace lldb_private {
+
+class Stream;
+
+/// A kind of metric to collect.
+enum class Metric : unsigned {
+#define LLDB_METRIC_DEF(EnumName, Desc) EnumName,
+#include "lldb/Utility/Metrics.def"
+  MaxID
+};
+
+/// Keep track of statistics. Data collection is off by default.
+class Analytics {
+public:
+  Analytics();
+
+  void SetEnabled(bool v) { m_collecting_analytics = v; }
+
+  bool IsEnabled() const { return m_collecting_analytics; }
+
+  void Record(Metric stat);
+
+  void Print(Stream &stream) const;
+
+  std::unique_ptr<StructuredData::Dictionary> GetAsStructuredData() const;
+
+private:
+  std::vector<unsigned> m_counters;
+  bool m_collecting_analytics = false;
+};
+
+} // namespace lldb_private
+
+#endif // LLDB_UTILITY_ANALYTICS_H

lldb/include/lldb/Utility/Metrics.def

+//===-- Statistics.def ------------------------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLDB_METRIC_DEF
+#error "Must define LLDB_METRIC_DEF(EnumName, Desc)."
+#endif
+
+LLDB_METRIC_DEF(ExpressionSuccess, "Number of expr evaluation successes")
+LLDB_METRIC_DEF(ExpressionFailure, "Number of expr evaluation failures")
+LLDB_METRIC_DEF(FrameVarSuccess, "Number of frame var successes")
+LLDB_METRIC_DEF(FrameVarFailure, "Number of frame var failures")
+
+#undef LLDB_METRIC_DEF

lldb/include/lldb/lldb-private-enumerations.h

enum class CompilerContextKind : uint16_t {
 
   Any = 1 << 15,
   /// Match 0..n nested modules.
   AnyModule = Any | Module,
   /// Match any type.
   AnyType = Any | Class | Struct | Union | Enum | Typedef
 };
 
-// Enumerations that can be used to specify the kind of metric we're looking at
-// when collecting stats.
-enum StatisticKind {
-  ExpressionSuccessful = 0,
-  ExpressionFailure = 1,
-  FrameVarSuccess = 2,
-  FrameVarFailure = 3,
-  StatisticMax = 4
-};
-
-
-inline std::string GetStatDescription(lldb_private::StatisticKind K) {
-   switch (K) {
-   case StatisticKind::ExpressionSuccessful:
-     return "Number of expr evaluation successes";
-   case StatisticKind::ExpressionFailure:
-     return "Number of expr evaluation failures";
-   case StatisticKind::FrameVarSuccess:
-     return "Number of frame var successes";
-   case StatisticKind::FrameVarFailure:
-     return "Number of frame var failures";
-   case StatisticKind::StatisticMax:
-     return "";
-   }
-   llvm_unreachable("Statistic not registered!");
-}
-
 } // namespace lldb_private
 
 namespace llvm {
 template <> struct format_provider<lldb_private::Vote> {
   static void format(const lldb_private::Vote &V, llvm::raw_ostream &Stream,
                      StringRef Style) {
     switch (V) {
     case lldb_private::eVoteNo:

lldb/include/lldb/module.modulemap

 
 
 module lldb_Utility {
   requires cplusplus
 
   umbrella "Utility"
   module * { export * }
 
+  textual header "Utility/Statistics.def"
+
   module lldb_defines { header "lldb-defines.h" export * }
   module lldb_enumerations { header "lldb-enumerations.h" export * }
   module lldb_forward { header "lldb-forward.h" export * }
   module lldb_private_enumerations { header "lldb-private-enumerations.h" export * }
   module lldb_private_defines { header "lldb-private-defines.h" export * }
   module lldb_private_forward { header "lldb-private-forward.h" export * }
   module lldb_private { header "lldb-private.h" export * }
   module lldb_private_interfaces { header "lldb-private-interfaces.h" export * }
   module lldb_private_types { header "lldb-private-types.h" export * }
   module lldb_public { header "lldb-public.h" export * }
   module lldb_types { header "lldb-types.h" export * }
   module lldb_versioning { header "lldb-versioning.h" export * }
 }

lldb/source/API/SBTarget.cpp

 SBStructuredData SBTarget::GetStatistics() {
   LLDB_RECORD_METHOD_NO_ARGS(lldb::SBStructuredData, SBTarget, GetStatistics);
 
   SBStructuredData data;
   TargetSP target_sp(GetSP());
   if (!target_sp)
     return LLDB_RECORD_RESULT(data);
 
-  auto stats_up = std::make_unique<StructuredData::Dictionary>();
-  int i = 0;
-  for (auto &Entry : target_sp->GetStatistics()) {
-    std::string Desc = lldb_private::GetStatDescription(
-        static_cast<lldb_private::StatisticKind>(i));
-    stats_up->AddIntegerItem(Desc, Entry);
-    i += 1;
-  }
-
-  data.m_impl_up->SetObjectSP(std::move(stats_up));
+  data.m_impl_up->SetObjectSP(target_sp->GetAnalytics().GetAsStructuredData());
   return LLDB_RECORD_RESULT(data);
 }
 
 void SBTarget::SetCollectingStats(bool v) {
   LLDB_RECORD_METHOD(void, SBTarget, SetCollectingStats, (bool), v);
 
   TargetSP target_sp(GetSP());
   if (!target_sp)

lldb/source/Commands/CommandObjectExpression.cpp

if (args.HasArgs()) {
     // No expression following options
     else if (expr.empty()) {
       GetMultilineExpression();
       return result.Succeeded();
     }
   }
 
   Target &target = GetSelectedOrDummyTarget();
+  Analytics &analytics = target.GetAnalytics();
   if (EvaluateExpression(expr, result.GetOutputStream(),
                          result.GetErrorStream(), result)) {
 
     if (!m_fixed_expression.empty() && target.GetEnableNotifyAboutFixIts()) {
       CommandHistory &history = m_interpreter.GetCommandHistory();
       // FIXME: Can we figure out what the user actually typed (e.g. some alias
       // for expr???)
       // If we can it would be nice to show that.
       std::string fixed_command("expression ");
       if (args.HasArgs()) {
         // Add in any options that might have been in the original command:
         fixed_command.append(std::string(args.GetArgStringWithDelimiter()));
         fixed_command.append(m_fixed_expression);
       } else
         fixed_command.append(m_fixed_expression);
       history.AppendString(fixed_command);
     }
-    // Increment statistics to record this expression evaluation success.
-    target.IncrementStats(StatisticKind::ExpressionSuccessful);
+    analytics.Record(Metric::ExpressionSuccess);
     return true;
   }
 
-  // Increment statistics to record this expression evaluation failure.
-  target.IncrementStats(StatisticKind::ExpressionFailure);
+  analytics.Record(Metric::ExpressionFailure);
   result.SetStatus(eReturnStatusFailed);
   return false;
 }

lldb/source/Commands/CommandObjectFrame.cpp

if (m_interpreter.TruncationWarningNecessary()) {
       result.GetOutputStream().Printf(m_interpreter.TruncationWarningText(),
                                       m_cmd_name.c_str());
       m_interpreter.TruncationWarningGiven();
     }
 
     // Increment statistics.
     bool res = result.Succeeded();
     Target &target = GetSelectedOrDummyTarget();
-    if (res)
-      target.IncrementStats(StatisticKind::FrameVarSuccess);
-    else
-      target.IncrementStats(StatisticKind::FrameVarFailure);
+    Analytics &analytics = target.GetAnalytics();
+    analytics.Record(res ? Metric::FrameVarSuccess : Metric::FrameVarFailure);
     return res;
   }
 
   OptionGroupOptions m_option_group;
   OptionGroupVariable m_option_variable;
   OptionGroupFormat m_option_format;
   OptionGroupValueObjectDisplay m_varobj_options;
 };

lldb/source/Commands/CommandObjectStats.cpp

CommandObjectStatsDump(CommandInterpreter &interpreter)
       : CommandObjectParsed(interpreter, "dump", "Dump statistics results",
                             nullptr, eCommandProcessMustBePaused) {}
 
   ~CommandObjectStatsDump() override = default;
 
 protected:
   bool DoExecute(Args &command, CommandReturnObject &result) override {
     Target &target = GetSelectedOrDummyTarget();
-
-    uint32_t i = 0;
-    for (auto &stat : target.GetStatistics()) {
-      result.AppendMessageWithFormat(
-          "%s : %u\n",
-          lldb_private::GetStatDescription(
-              static_cast<lldb_private::StatisticKind>(i))
-              .c_str(),
-          stat);
-      i += 1;
-    }
+    target.GetAnalytics().Print(result.GetOutputStream());
     result.SetStatus(eReturnStatusSuccessFinishResult);
     return true;
   }
 };
 
 CommandObjectStats::CommandObjectStats(CommandInterpreter &interpreter)
     : CommandObjectMultiword(interpreter, "statistics",
                              "Print statistics about a debugging session",

lldb/source/Target/Target.cpp

: TargetProperties(this),
       Broadcaster(debugger.GetBroadcasterManager(),
                   Target::GetStaticBroadcasterClass().AsCString()),
       ExecutionContextScope(), m_debugger(debugger), m_platform_sp(platform_sp),
       m_mutex(), m_arch(target_arch), m_images(this), m_section_load_history(),
       m_breakpoint_list(false), m_internal_breakpoint_list(true),
       m_watchpoint_list(), m_process_sp(), m_search_filter_sp(),
       m_image_search_paths(ImageSearchPathsChanged, this),
       m_source_manager_up(), m_stop_hooks(), m_stop_hook_next_id(0),
-      m_latest_stop_hook_id(0),
-      m_valid(true), m_suppress_stop_hooks(false),
+      m_latest_stop_hook_id(0), m_valid(true), m_suppress_stop_hooks(false),
       m_is_dummy_target(is_dummy_target),
       m_frame_recognizer_manager_up(
-          std::make_unique<StackFrameRecognizerManager>()),
-      m_stats_storage(static_cast<int>(StatisticKind::StatisticMax))
-
-{
+          std::make_unique<StackFrameRecognizerManager>()) {
   SetEventName(eBroadcastBitBreakpointChanged, "breakpoint-changed");
   SetEventName(eBroadcastBitModulesLoaded, "modules-loaded");
   SetEventName(eBroadcastBitModulesUnloaded, "modules-unloaded");
   SetEventName(eBroadcastBitWatchpointChanged, "watchpoint-changed");
   SetEventName(eBroadcastBitSymbolsLoaded, "symbols-loaded");
 
   CheckInWithManager();
 

lldb/source/Utility/Analytics.cpp

+//===-- Analytics.cpp -----------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "lldb/Utility/Analytics.h"
+
+#include "lldb/Utility/Stream.h"
+#include "lldb/Utility/StructuredData.h"
+
+#include <cassert>
+#include <memory>
+#include <utility>
+
+namespace lldb_private {
+
+static unsigned GetNumberOfMetrics() {
+  return static_cast<unsigned>(Metric::MaxID);
+}
+
+static unsigned GetCounterIndexForMetric(Metric S) {
+  assert(S != Metric::MaxID && "invalid metric");
+  return static_cast<unsigned>(S);
+}
+
+static Metric GetMetricForCounterIndex(unsigned index) {
+  assert(index < GetNumberOfMetrics() && "invalid metric");
+  return static_cast<Metric>(index);
+}
+
+static const char *GetMetricDescription(Metric S) {
+  switch (S) {
+#define LLDB_METRIC_DEF(EnumName, Desc)                                        \
+  case Metric::EnumName:                                                       \
+    return Desc;
+#include "lldb/Utility/Metrics.def"
+  case Metric::MaxID:
+    llvm_unreachable("invalid metric");
+  }
+}
+
+Analytics::Analytics() { m_counters.resize(GetNumberOfMetrics()); }
+
+void Analytics::Record(Metric metric) {
+  if (!m_collecting_analytics)
+    return;
+  ++m_counters[GetCounterIndexForMetric(metric)];
+}
+
+void Analytics::Print(Stream &stream) const {
+  for (unsigned metric_id = 0; metric_id < GetNumberOfMetrics(); ++metric_id) {
+    Metric s = GetMetricForCounterIndex(metric_id);
+    stream.Printf("%s : %u\n", GetMetricDescription(s), m_counters[metric_id]);
+  }
+}
+
+std::unique_ptr<StructuredData::Dictionary>
+Analytics::GetAsStructuredData() const {
+  auto metrics_up = std::make_unique<StructuredData::Dictionary>();
+  for (unsigned metric_id = 0; metric_id < GetNumberOfMetrics(); ++metric_id) {
+    Metric s = GetMetricForCounterIndex(metric_id);
+    metrics_up->AddIntegerItem(GetMetricDescription(s), m_counters[metric_id]);
+  }
+  return metrics_up;
+}
+
+} // namespace lldb_private

lldb/source/Utility/CMakeLists.txt

if (CMAKE_SYSTEM_NAME MATCHES "Windows")
   list(APPEND LLDB_SYSTEM_LIBS ws2_32 rpcrt4)
 endif ()
 
 if (NOT HAVE_CXX_ATOMICS64_WITHOUT_LIB )
     list(APPEND LLDB_SYSTEM_LIBS atomic)
 endif()
 
 add_lldb_library(lldbUtility
+  Analytics.cpp
   ArchSpec.cpp
   Args.cpp
   Baton.cpp
   Broadcaster.cpp
   CompletionRequest.cpp
   Connection.cpp
   ConstString.cpp
   DataBufferHeap.cpp

lldb/test/API/functionalities/stats_api/TestStatisticsAPI.py

def test_stats_api(self):
         target.SetCollectingStats(False)
         self.assertFalse(target.GetCollectingStats())
 
         # Test the function to get the statistics in JSON'ish.
         stats = target.GetStatistics()
         stream = lldb.SBStream()
         res = stats.GetAsJSON(stream)
         stats_json = sorted(json.loads(stream.GetData()))
-        self.assertEqual(len(stats_json), 4)
+        self.assertEqual(len(stats_json), 4, stream.GetData())
         self.assertIn("Number of expr evaluation failures", stats_json)
         self.assertIn("Number of expr evaluation successes", stats_json)
         self.assertIn("Number of frame var failures", stats_json)
         self.assertIn("Number of frame var successes", stats_json)