[Support] TimerGroup changes

[lebedev.ri]

Summary

This is needed for the continuation of D46504,
to be able to store the timings.

The floating-point values are dumped with no precision loss.

See dependent differential D46602 for details/use-case.

[george.karpenkov]

@lebedev.ri LLVM already has facilities for outputting statistics in JSON, it seems that would be sufficient for your purposes?
I did something similar for the static analyzer in https://reviews.llvm.org/D43131

[lebedev.ri]

In D46603#1092135, @george.karpenkov wrote:

@lebedev.ri LLVM already has facilities for outputting statistics in JSON, it seems that would be sufficient for your purposes?
I did something similar for the static analyzer in https://reviews.llvm.org/D43131

YAML != JSON.
I did see all that TimerGroup stuff.
I will take a second look, but the dumpVal() change will still be needed either way.

[lebedev.ri]

1. Drop CSV stuff
2. Add constructor from previously-collected StringMap<TimeRecord>
3. Make printJSONValues() public.
4. Dump floating-point values in scientific format with full precision.

[lebedev.ri]

Add lock to now-public printJSONValues().

I have no understanding of the static TimerGroupList,
but this is symmetrical with other [public] TimerGroup::*print* methods.

[alexfh]

I wonder why JSON? What kind of a tool do folks use to process the outputs of printJSONValue? Is there anything existing or is JSON used "just in case"? I personally use either spreadsheets, python or shell when I deal with this kind of data, and all three of them can work reasonably well with CSV (plus the data is pretty simple).

[lebedev.ri]

• Use sane (not the same as the one right before..) suffix for when json-printing mem usage. Admittedly, i haven't tried it, but it just does not make sense otherwise.

[lebedev.ri]

Ping.
Any thoughts on these Timer changes?

[george.karpenkov]

I see four separate changes: s/.sys/mem one (can be committed without review), exposing printJSONValues and consequent adding of a lock, adding a constructor accepting a map, and fixing formatting in printJSONValue. All four look good to me, but probably should be reviewed separately.

lib/Support/Timer.cpp

385

This change seems unrelated to the new constructor accepting map, right?

405

That's independent from this change, right?

[lebedev.ri]

In D46603#1100455, @george.karpenkov wrote:

I see four separate changes: s/.sys/mem one (can be committed without review), exposing printJSONValues and consequent adding of a lock, adding a constructor accepting a map, and fixing formatting in printJSONValue. All four look good to me, but probably should be reviewed separately.

Thank you for taking a look!
Posted as D46936, D46937, D46938, D46939

[lebedev.ri]

In D46603#1101047, @lebedev.ri wrote:

In D46603#1100455, @george.karpenkov wrote:

I see four separate changes: s/.sys/mem one (can be committed without review), exposing printJSONValues and consequent adding of a lock, adding a constructor accepting a map, and fixing formatting in printJSONValue. All four look good to me, but probably should be reviewed separately.

Thank you for taking a look!
Posted as D46936, D46937, D46938, D46939

@alexfh & @george.karpenkov Thanks for reviewing those!
Not sure yet whether i will land them right away, or wait for clang-tidy part.

[george.karpenkov]

Not sure yet whether i will land them right away, or wait for clang-tidy part.

I think it's better to land, as otherwise you risk merge conflicts, and implicit conflicts (git reports no errors, but the resulting code is bad) can be very annoying.

include/llvm/Support/Timer.h

 //===-- llvm/Support/Timer.h - Interval Timing Support ----------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_SUPPORT_TIMER_H
 #define LLVM_SUPPORT_TIMER_H
 
+#include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/DataTypes.h"
 #include <cassert>
 #include <string>
 #include <utility>
 #include <vector>
 
 namespace llvm {
class TimerGroup {
 
   TimerGroup **Prev; ///< Pointer to Next field of previous timergroup in list.
   TimerGroup *Next;  ///< Pointer to next timergroup in list.
   TimerGroup(const TimerGroup &TG) = delete;
   void operator=(const TimerGroup &TG) = delete;
 
 public:
   explicit TimerGroup(StringRef Name, StringRef Description);
+
+  explicit TimerGroup(StringRef Name, StringRef Description,
+                      const StringMap<TimeRecord> &Records);
+
   ~TimerGroup();
 
   void setName(StringRef NewName, StringRef NewDescription) {
     Name.assign(NewName.begin(), NewName.end());
     Description.assign(NewDescription.begin(), NewDescription.end());
   }
 
   /// Print any started timers in this group and zero them.
   void print(raw_ostream &OS);
 
   /// This static method prints all timers and clears them all out.
   static void printAll(raw_ostream &OS);
 
+  const char *printJSONValues(raw_ostream &OS, const char *delim);
+
   /// Prints all timers as JSON key/value pairs, and clears them all out.
   static const char *printAllJSONValues(raw_ostream &OS, const char *delim);
 
   /// Ensure global timer group lists are initialized. This function is mostly
   /// used by the Statistic code to influence the construction and destruction
   /// order of the global timer lists.
   static void ConstructTimerLists();
 private:
   friend class Timer;
   friend void PrintStatisticsJSON(raw_ostream &OS);
   void addTimer(Timer &T);
   void removeTimer(Timer &T);
   void prepareToPrintList();
   void PrintQueuedTimers(raw_ostream &OS);
   void printJSONValue(raw_ostream &OS, const PrintRecord &R,
                       const char *suffix, double Value);
-  const char *printJSONValues(raw_ostream &OS, const char *delim);
 };
 
 } // end namespace llvm
 
 #endif

lib/Support/Timer.cpp

 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Format.h"
 #include "llvm/Support/ManagedStatic.h"
 #include "llvm/Support/Mutex.h"
 #include "llvm/Support/Process.h"
 #include "llvm/Support/YAMLTraits.h"
 #include "llvm/Support/raw_ostream.h"
+#include <limits>
+
 using namespace llvm;
 
 // This ugly hack is brought to you courtesy of constructor/destructor ordering
 // being unspecified by C++.  Basically the problem is that a Statistic object
 // gets destroyed, which ends up calling 'GetLibSupportInfoOutputFile()'
 // (below), which calls this function.  LibSupportInfoOutputFilename used to be
 // a global variable, but sometimes it would get destroyed before the Statistic,
 // causing havoc to ensue.  We "fix" this by creating the string the first time
TimerGroup::TimerGroup(StringRef Name, StringRef Description)
   sys::SmartScopedLock<true> L(*TimerLock);
   if (TimerGroupList)
     TimerGroupList->Prev = &Next;
   Next = TimerGroupList;
   Prev = &TimerGroupList;
   TimerGroupList = this;
 }
 
+TimerGroup::TimerGroup(StringRef Name, StringRef Description,
+                       const StringMap<TimeRecord> &Records)
+    : TimerGroup(Name, Description) {
+  TimersToPrint.reserve(Records.size());
+  for (const auto &P : Records)
+    TimersToPrint.emplace_back(P.getValue(), P.getKey(), P.getKey());
+  assert(TimersToPrint.size() == Records.size() && "Size mismatch");
+}
+
 TimerGroup::~TimerGroup() {
   // If the timer group is destroyed before the timers it owns, accumulate and
   // print the timing data.
   while (FirstTimer)
     removeTimer(*FirstTimer);
 
   // Remove the group from the TimerGroupList.
   sys::SmartScopedLock<true> L(*TimerLock);
void TimerGroup::printAll(raw_ostream &OS) {
 
   for (TimerGroup *TG = TimerGroupList; TG; TG = TG->Next)
     TG->print(OS);
 }
 
 void TimerGroup::printJSONValue(raw_ostream &OS, const PrintRecord &R,
                                 const char *suffix, double Value) {
   assert(yaml::needsQuotes(Name) == yaml::QuotingType::None &&
-         "TimerGroup name needs no quotes");
+         "TimerGroup name should not need quotes");
   assert(yaml::needsQuotes(R.Name) == yaml::QuotingType::None &&
-         "Timer name needs no quotes");
-  OS << "\t\"time." << Name << '.' << R.Name << suffix << "\": " << Value;
+         "Timer name should not need quotes");
+  constexpr auto max_digits10 = std::numeric_limits<double>::max_digits10;
+  OS << "\t\"time." << Name << '.' << R.Name << suffix
+     << "\": " << format("%.*e", max_digits10 - 1, Value);
 }
 
 const char *TimerGroup::printJSONValues(raw_ostream &OS, const char *delim) {
+  sys::SmartScopedLock<true> L(*TimerLock);
+
   prepareToPrintList();
   for (const PrintRecord &R : TimersToPrint) {
     OS << delim;
     delim = ",\n";
 
     const TimeRecord &T = R.Time;
     printJSONValue(OS, R, ".wall", T.getWallTime());
     OS << delim;
     printJSONValue(OS, R, ".user", T.getUserTime());
     OS << delim;
     printJSONValue(OS, R, ".sys", T.getSystemTime());
     if (T.getMemUsed()) {
       OS << delim;
-      printJSONValue(OS, R, ".sys", T.getMemUsed());
+      printJSONValue(OS, R, ".mem", T.getMemUsed());
     }
   }
   TimersToPrint.clear();
   return delim;
 }
 
 const char *TimerGroup::printAllJSONValues(raw_ostream &OS, const char *delim) {
   sys::SmartScopedLock<true> L(*TimerLock);
   for (TimerGroup *TG = TimerGroupList; TG; TG = TG->Next)
     delim = TG->printJSONValues(OS, delim);
   return delim;
 }
 
 void TimerGroup::ConstructTimerLists() {
   (void)*NamedGroupedTimers;
 }