Make STATISTIC() values available programmatically

[dsanders]

Summary

It can be useful for tools to be able to retrieve the values of variables
declared via STATISTIC() directly without having to emit them and parse
them back. Use cases include:

• Needing to report specific statistics to a test harness
• Wanting to post-process statistics. For example, to produce a percentage of functions that were fully selected by GlobalISel

Make this possible by adding llvm::GetStatistics() which returns an
iterator_range that can be used to inspect the statistics that have been
touched during execution. When statistics are disabled (NDEBUG and not
LLVM_ENABLE_STATISTICS) this method will return an empty range.

This patch doesn't address the effect of multiple compilations within the same
process. In such situations, the statistics will be cumulative for all
compilations up to the GetStatistics() call.

[dsanders]

During further testing, I noticed that the pre-existing LLVM_ENABLE_STATS option
isn't actually available to CMake and therefore the -DLLVM_ENABLE_STATS option
that PrintStatistics() suggests in its error message doesn't do anything.

Updated the patch to fix that at the same time

[dsanders]

Remove unintentional change. ResetStatistics() is something I'm considering implementing later

[rtereshin]

LGTM

My only concern here is that I don't quite understand why only the ManagedStatic<StatisticInfo> StatInfo's writer is guarded by a mutex, but non of the readers. As the newly added GetStatistics() method leaks a read-access to StatInfo into the wild losing any control over it, it might be a problem.

Could you please clarify that a little bit?

Thanks! I really like the direction this is going =)

[rtereshin]

Ah, yeah, also, as the range returned is a couple of vector iterators, it looks like bumping any counter anywhere would potentially invalidate all the ranges existing. I suspect that might be a little error-prone and fragile even in a single-threaded environment. Maybe replace that vector with a list? And add a test that will check we don't regress on this little detail.

[rtereshin]

Alternatively, we could just copy the whole thing instead of returning iterators to it. Potentially protect that copy with that mutex and then we're rock solid.
But again, why do we even worry about this being thread-safe in any way one more time?

[dsanders]

Thanks

In D43901#1024928, @rtereshin wrote:

LGTM

My only concern here is that I don't quite understand why only the ManagedStatic<StatisticInfo> StatInfo's writer is guarded by a mutex, but non of the readers. As the newly added GetStatistics() method leaks a read-access to StatInfo into the wild losing any control over it, it might be a problem.

Could you please clarify that a little bit?

The value itself isn't protected by a mutex but rather ensures atomic updates with std::atomic. Do you mean the StatLock mutex referenced by RegisterStatistic()? That's protecting against a race that can occur on the first modification to the value. The StatisticInfo object records a list of all Statistic objects that were modified at some point. The first time Statistic modified it registers itself with the StatisticInfo object and we need a lock to avoid double-registration (or worse).

Ah, yeah, also, as the range returned is a couple of vector iterators, it looks like bumping any counter anywhere would potentially invalidate all the ranges existing. I suspect that might be a little error-prone and fragile even in a single-threaded environment. Maybe replace that vector with a list? And add a test that will check we don't regress on this little detail.
Alternatively, we could just copy the whole thing instead of returning iterators to it. Potentially protect that copy with that mutex and then we're rock solid.
But again, why do we even worry about this being thread-safe in any way one more time?

For a lot of tools, returning iterators is going to be fine. The process would typically be:

• compile, possibly multiple compilations on multiple threads
• wait for compilations
• extract statistics

That's certainly good enough for my aims. However, you're right that there's cases where races can happen. For the most part, they're not an issue since to use this as a measuring tool you already need to manage your threads such that you don't have noise in the statistics from things you didn't want to measure. With those requirements, there's no new registrations while the results are retrieved. However, if it's used to capture a snapshot of the current values then stopping the world is a burden and it's possible for a new statistic to be registered while you're reading them, invalidating the iterators.

I'll switch it to return a std::vector<std::pair<StringRef(), unsigned>>. That will cover the latter use case too

[dsanders]

Fix a race between GetStatistics() and addStatistic()
Documented caveats that are the callers responsibility to handle if it matters to them.

[rtereshin]

Thanks! That's looking great!

As discussed, a separate patch could add a lock on that mutex in PrintStatistics and PrintStatisticsJSON to avoid the race.

I'm amazed that C++ standard library doesn't appear to have a Writer/Reader Lock, only the regular mutex. That's a shame. That thing could be useful here.

LGTM (again =))

[bogner]

include/llvm/ADT/Statistic.h

63 ↗

(On Diff #136796)

This assumes that every file that includes llvm/ADT/Statistic.h included llvm/Config/config.h first, which is actually never true due to our rules about include order. OTOH, we can't include config.h in this header, because it's llvm internal. I think if we want to do this we need to publish LLVM_ENABLE_STATS into llvm-config.h as API.

include/llvm/ADT/StatisticInfo.h

24–54

Why move this to the header / make this type public? AFAICT nobody uses it since the only interesting API for users is GetStatistics() below.

63

Don't capitalize function names. This should probably be getStatistics().

unittests/ADT/StatisticTest.cpp

29

Related to the Config.h comment above, I'm pretty sure this is always false in the test, as is.

[dsanders]

include/llvm/ADT/Statistic.h

63 ↗

(On Diff #136796)

I've moved it to llvm-config.h and made Statistics.h include it.

As for whether it belongs in llvm-config.h or Config.h. At the moment, we ship Statistics.h and it needs to have access to the macro so llvm-config.h makes sense right now.

I do want to look into making STATISTIC() more fine-grained though. I haven't thought much about how I'm going to achieve that but if I can move the #if into the cpp file instead then it will make more sense to use Config.h. That said, Statistic will still be public and anyone using it will be quite surprised if it doesn't count and they can't see that in the header.

include/llvm/ADT/StatisticInfo.h

24–54

I'll move this back. An earlier version of the patch had GetStatistics() return this object itself

63

This one is capitalized to match the existing PrintStatistics() and PrintStatisticsJSON(). It would be strange to have one function in this family with a lower case first letter

unittests/ADT/StatisticTest.cpp

29

Well spotted.

[dsanders]

Statistics can actually be gathered (LLVM_ENABLE_STATS isn't always false anymore). Tested out of the patch using #error in the unit test.
Moved StatisticInfo back

[bogner]

One last comment, then I think this is good to go.

lib/Support/Statistic.cpp

54

I think you lost the anonymous namespace here by accident.

include/llvm/ADT/StatisticInfo.h

+//===-- llvm/ADT/StatisticInfo.h - Easy way to expose stats -----*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+//
+// This file defines the 'StatisticInfo' class, which provides access to the
+// library of statistics that are collected.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_ADT_STATISTICINFO_H
+#define LLVM_ADT_STATISTICINFO_H
+
+#include "llvm/ADT/iterator_range.h"
+#include "llvm/ADT/Statistic.h"
+#include <vector>
+
+namespace llvm {
+
+/// This class is used in a ManagedStatic so that it is created on demand (when
+/// the first statistic is bumped) and destroyed only when llvm_shutdown is
+/// called. We print statistics from the destructor.
+/// This class is also used to look up statistic values from applications that
+/// use LLVM.
+class StatisticInfo {
+  std::vector<const Statistic*> Stats;
+
+  friend void llvm::PrintStatistics();
+  friend void llvm::PrintStatistics(raw_ostream &OS);
+  friend void llvm::PrintStatisticsJSON(raw_ostream &OS);
+
+  /// Sort statistics by debugtype,name,description.
+  void sort();
+public:
+  using const_iterator = std::vector<const Statistic *>::const_iterator;
+
+  StatisticInfo();
+  ~StatisticInfo();
+
+  void addStatistic(const Statistic *S) {
+    Stats.push_back(S);
+  }
+
+  const_iterator begin() const { return Stats.begin(); }
+  const_iterator end() const { return Stats.end(); }
+  iterator_range<const_iterator> statistics() const {
+    return {begin(), end()};
+  }
+};
+
+/// \brief Get the statistics. This can be used to look up the value of
+/// statistics without needing to parse JSON.
+const iterator_range<StatisticInfo::const_iterator> GetStatistics();
+
+} // end namespace llvm
+
+#endif // LLVM_ADT_STATISTICINFO_H

lib/Support/Statistic.cpp

 //
 // static Statistic NumInstEliminated("GCSE", "Number of instructions killed");
 //
 // Later, in the code: ++NumInstEliminated;
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/ADT/Statistic.h"
+#include "llvm/ADT/StatisticInfo.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Compiler.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/Format.h"
 #include "llvm/Support/ManagedStatic.h"
 #include "llvm/Support/Mutex.h"
 #include "llvm/Support/Timer.h"

 
 static cl::opt<bool> StatsAsJSON("stats-json",
                                  cl::desc("Display statistics as json data"),
                                  cl::Hidden);
 
 static bool Enabled;
 static bool PrintOnExit;
 
-namespace {
-/// StatisticInfo - This class is used in a ManagedStatic so that it is created
-/// on demand (when the first statistic is bumped) and destroyed only when
-/// llvm_shutdown is called.  We print statistics from the destructor.
-class StatisticInfo {
-  std::vector<const Statistic*> Stats;
-  friend void llvm::PrintStatistics();
-  friend void llvm::PrintStatistics(raw_ostream &OS);
-  friend void llvm::PrintStatisticsJSON(raw_ostream &OS);
-
-  /// Sort statistics by debugtype,name,description.
-  void sort();
-public:
-  StatisticInfo();
-  ~StatisticInfo();
-
-  void addStatistic(const Statistic *S) {
-    Stats.push_back(S);
-  }
-};
-}
-
 static ManagedStatic<StatisticInfo> StatInfo;
 static ManagedStatic<sys::SmartMutex<true> > StatLock;
 
 /// RegisterStatistic - The first time a statistic is bumped, this method is
 /// called.
 void Statistic::RegisterStatistic() {
   // If stats are enabled, inform StatInfo that this statistic should be
   // printed.
#else
   if (Stats) {
     // Get the stream to write to.
     std::unique_ptr<raw_ostream> OutStream = CreateInfoOutputFile();
     (*OutStream) << "Statistics are disabled.  "
                  << "Build with asserts or with -DLLVM_ENABLE_STATS\n";
   }
 #endif
 }
+
+const iterator_range<StatisticInfo::const_iterator> llvm::GetStatistics() {
+  return StatInfo->statistics();
+}

unittests/ADT/CMakeLists.txt

set(ADTSources
   SetVectorTest.cpp
   SimpleIListTest.cpp
   SmallPtrSetTest.cpp
   SmallStringTest.cpp
   SmallVectorTest.cpp
   SparseBitVectorTest.cpp
   SparseMultiSetTest.cpp
   SparseSetTest.cpp
+  StatisticTest.cpp
   StringExtrasTest.cpp
   StringMapTest.cpp
   StringRefTest.cpp
   StringSwitchTest.cpp
   TinyPtrVectorTest.cpp
   TripleTest.cpp
   TwineTest.cpp
   VariadicFunctionTest.cpp
  )
 
 add_llvm_unittest(ADTTests
   ${ADTSources}
   )
 
 add_dependencies(ADTTests intrinsics_gen)

unittests/ADT/StatisticTest.cpp

+//===- llvm/unittest/ADT/StatisticTest.cpp - Statistic unit tests ---------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "llvm/ADT/Statistic.h"
+#include "llvm/ADT/StatisticInfo.h"
+#include "llvm/Support/raw_ostream.h"
+#include "gtest/gtest.h"
+using namespace llvm;
+
+namespace {
+#define DEBUG_TYPE "unittest"
+STATISTIC(Counter, "Counts things");
+STATISTIC(Counter2, "Counts other things");
+
+TEST(StatisticTest, Count) {
+  EnableStatistics();
+
+  Counter = 0;
+  EXPECT_EQ(Counter, 0u);
+  Counter++;
+  Counter++;
+#if !defined(NDEBUG) || defined(LLVM_ENABLE_STATS)
+  EXPECT_EQ(Counter, 2u);
+#else
+  EXPECT_EQ(Counter, 0u);
+#endif
+}
+
+TEST(StatisticTest, Assign) {
+  EnableStatistics();
+
+  Counter = 2;
+#if !defined(NDEBUG) || defined(LLVM_ENABLE_STATS)
+  EXPECT_EQ(Counter, 2u);
+#else
+  EXPECT_EQ(Counter, 0u);
+#endif
+}
+
+TEST(StatisticTest, API) {
+  EnableStatistics();
+
+  Counter = 0;
+  EXPECT_EQ(Counter, 0u);
+  Counter++;
+  Counter++;
+#if !defined(NDEBUG) || defined(LLVM_ENABLE_STATS)
+  EXPECT_EQ(Counter, 2u);
+#else
+  EXPECT_EQ(Counter, 0u);
+#endif
+
+#if !defined(NDEBUG) || defined(LLVM_ENABLE_STATS)
+  const auto Range1 = GetStatistics();
+  EXPECT_NE(Range1.begin(), Range1.end());
+  EXPECT_EQ(Range1.begin() + 1, Range1.end());
+
+  const Statistic *S1 = nullptr;
+  const Statistic *S2 = nullptr;
+  for (const auto *S : Range1) {
+    if (std::string(S->getName()) == "Counter")
+      S1 = S;
+    if (std::string(S->getName()) == "Counter2")
+      S2 = S;
+  }
+
+  EXPECT_NE(S1, nullptr);
+  EXPECT_EQ(S2, nullptr);
+
+  // Counter2 will be registered when it's first touched.
+  Counter2++;
+
+  const auto Range2 = GetStatistics();
+  EXPECT_NE(Range2.begin(), Range2.end());
+  EXPECT_EQ(Range2.begin() + 2, Range2.end());
+
+  S1 = nullptr;
+  for (const auto *S : Range2) {
+    if (std::string(S->getName()) == "Counter")
+      S1 = S;
+    if (std::string(S->getName()) == "Counter2")
+      S2 = S;
+  }
+
+  EXPECT_NE(S1, nullptr);
+  EXPECT_NE(S2, nullptr);
+
+  EXPECT_EQ(S1->getName(), "Counter");
+  EXPECT_EQ(S1->getDesc(), "Counts things");
+  EXPECT_EQ(S1->getValue(), 2u);
+
+  EXPECT_EQ(S2->getName(), "Counter2");
+  EXPECT_EQ(S2->getDesc(), "Counts other things");
+  EXPECT_EQ(S2->getValue(), 1u);
+#else
+  Counter2++;
+  auto &Range = GetStatistics();
+  EXPECT_EQ(Range.begin(), Range.end());
+#endif
+}
+
+} // end anonymous namespace