[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

406

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

421

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

void operator-=(const TimeRecord &RHS) {
     UserTime   -= RHS.UserTime;
     SystemTime -= RHS.SystemTime;
     MemUsed    -= RHS.MemUsed;
   }
 
   /// Print the current time record to \p OS, with a breakdown showing
   /// contributions to the \p Total time record.
   void print(const TimeRecord &Total, raw_ostream &OS) const;
+
+  /// Print the current time record to \p OS as CSV, with full precision.
+  /// Only the values in this timer are printed, no percentages.
+  void printCSV(const TimeRecord &Total, raw_ostream &OS) const;
 };
 
 /// This class is used to track the amount of time spent between invocations of
 /// its startTimer()/stopTimer() methods.  Given appropriate OS support it can
 /// also keep track of the RSS of the program at various points.  By default,
 /// the Timer will print the amount of time it has captured to standard error
 /// when the last timer is destroyed, otherwise it is printed when its
 /// TimerGroup is destroyed.  Timers do not print their information if they are

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
void TimeRecord::print(const TimeRecord &Total, raw_ostream &OS) const {
   printVal(getWallTime(), Total.getWallTime(), OS);
 
   OS << "  ";
 
   if (Total.getMemUsed())
     OS << format("%9" PRId64 "  ", (int64_t)getMemUsed());
 }
 
+static void dumpVal(double Val, raw_ostream &OS) {
+  constexpr auto max_digits10 = std::numeric_limits<double>::max_digits10;
+  // FIXME: can ',' be a decimal separator?
+  OS << format("%.*e", max_digits10 - 1, Val);
+}
+
+void TimeRecord::printCSV(const TimeRecord &Total, raw_ostream &OS) const {
+  int Column = 0;
+  auto comma = [&Column, &OS]() {
+    if (Column)
+      OS << ','; // FIXME: can ',' be a decimal separator?
+    ++Column;
+  };
+  auto printColumn = [comma, &OS](bool Print, double Val) {
+    if (!Print)
+      return;
+    comma();
+    dumpVal(Val, OS);
+  };
+
+  printColumn(Total.getUserTime(), getUserTime());
+  printColumn(Total.getSystemTime(), getSystemTime());
+  printColumn(Total.getProcessTime(), getProcessTime());
+  printColumn(true, getWallTime());
+  if (Total.getMemUsed()) {
+    comma();
+    OS << format("%9" PRId64, (int64_t)getMemUsed());
+  }
+}
 
 //===----------------------------------------------------------------------===//
 //   NamedRegionTimer Implementation
 //===----------------------------------------------------------------------===//
 
 namespace {
 
 typedef StringMap<Timer> Name2TimerMap;

 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");
   assert(yaml::needsQuotes(R.Name) == yaml::QuotingType::None &&
          "Timer name needs no quotes");
   OS << "\t\"time." << Name << '.' << R.Name << suffix << "\": " << Value;
 }
 
 const char *TimerGroup::printJSONValues(raw_ostream &OS, const char *delim) {
   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());
     }
   }
   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;
 }