[Support] Make printAllJSONValues public, for custom output.

[graydon]

Summary

This changes the static method TimerGroup::printAllJSONValues from private to
public, to match the static method TimerGroup::printAll. When trying to drive
the reporting machinery by hand, the existing API is _almost_ flexible enough,
but this entrypoint is required to intermix printing timers with other
non-timer output.

The underlying motive here is a Swift change to consolidate the collection of
timers, LLVM statistics and other (non-assert-dependent) counters into JSON
files, which requires a bit of manual intervention in LLVM's stat and timer
output routines. See https://github.com/apple/swift/pull/8477 for details.

[MatzeB]

We could go ahead and make this function public for now.

However I don't expect this API to be stable. Long term we really should move away from a global list of timers accessed by TimerGroup. In fact I am planning changes to the statistics system right now. Are you prepared to loose the function again in a few weeks?

Also the fact that you need to call this function seems suspicious to me. Could you elaborate a bit why PrintStatisticsJSON() wasn't enough for your use case?

[graydon]

In D31566#716865, @MatzeB wrote:

We could go ahead and make this function public for now.

However I don't expect this API to be stable. Long term we really should move away from a global list of timers accessed by TimerGroup. In fact I am planning changes to the statistics system right now. Are you prepared to loose the function again in a few weeks?

I guess. We use it in swift, so I'd prefer you not just break swift's usage! But if you're modifying it in a way that's still usable by outside clients, we can probably adapt. What do you have planned?

Also the fact that you need to call this function seems suspicious to me. Could you elaborate a bit why PrintStatisticsJSON() wasn't enough for your use case?

Because PrintStatisticsJSON() prints an opening- and close-brace, meaning it cannot be used as part of another printing function (at least not without introducing a superfluous level of JSON-object-nesting).

The root problem is that LLVM statistics are enabled/disabled at compile time, by a #define that I'm not really comfortable switching globally (they wind up being used pervasively in clang and llvm, and they cost a global lock + mfence every time they're incremented). So while I can rely on the _timer_ facility always being present, the statistics facility is present or absent depending on whether I'm using an asserts build. Thus for any counters that I want to "always measure", even in release builds (independent of asserts) I need to count them independently and place their output in the place where LLVM would normally put its statistics output.

[MatzeB]

In D31566#716880, @graydon wrote:

In D31566#716865, @MatzeB wrote:

We could go ahead and make this function public for now.

However I don't expect this API to be stable. Long term we really should move away from a global list of timers accessed by TimerGroup. In fact I am planning changes to the statistics system right now. Are you prepared to loose the function again in a few weeks?

I guess. We use it in swift, so I'd prefer you not just break swift's usage! But if you're modifying it in a way that's still usable by outside clients, we can probably adapt. What do you have planned?

The motivation on our side is that we want statistics (I think of timers as being part of statistics) per module or per function and not just per compiler invocation. A global timer list probably won't help here. I hope we can move to some model where you have a statistic stream object maintaining the current context (context being module/function in our case) that you send statistic events to (= timer or other statistic values). Unfortunately this is all early stage planing/talking to people right now so I cannot give you any details.

Anyway without even the new system in place I cannot say much about how to transition to it... It'll surely be usably by code outside of LLVM, I guess we have to discuss how to transition users of the old system when we we know how the new one looks like.

Also the fact that you need to call this function seems suspicious to me. Could you elaborate a bit why PrintStatisticsJSON() wasn't enough for your use case?

Because PrintStatisticsJSON() prints an opening- and close-brace, meaning it cannot be used as part of another printing function (at least not without introducing a superfluous level of JSON-object-nesting).

The root problem is that LLVM statistics are enabled/disabled at compile time, by a #define that I'm not really comfortable switching globally (they wind up being used pervasively in clang and llvm, and they cost a global lock + mfence every time they're incremented). So while I can rely on the _timer_ facility always being present, the statistics facility is present or absent depending on whether I'm using an asserts build. Thus for any counters that I want to "always measure", even in release builds (independent of asserts) I need to count them independently and place their output in the place where LLVM would normally put its statistics output.

Sounds like we have very similar problems. Unfortunately my attempts at making them cheaper failed with the current system because people rejected global constructors/variables (which however is the only way in the current system), see also http://lists.llvm.org/pipermail/llvm-dev/2016-December/108088.html

[graydon]

In D31566#716897, @MatzeB wrote:

Anyway without even the new system in place I cannot say much about how to transition to it... It'll surely be usably by code outside of LLVM, I guess we have to discuss how to transition users of the old system when we we know how the new one looks like.

Ok, cool! I'll keep my eyes open for a change; if you happen to decide / know a change is coming I'd appreciate a CC on it, but otherwise I'll just wait for the build to break :)

Sounds like we have very similar problems. Unfortunately my attempts at making them cheaper failed with the current system because people rejected global constructors/variables (which however is the only way in the current system), see also http://lists.llvm.org/pipermail/llvm-dev/2016-December/108088.html

Yeah. It's tricky to balance all the concerns around globals/statics/inits. I'm fine with whatever happens, including status quo. It's usable enough.

Thanks!

llvm/trunk/include/llvm/Support/Timer.h

public:
   }
 
   /// 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);
 
+  /// 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);
-  static const char *printAllJSONValues(raw_ostream &OS, const char *delim);
 };
 
 } // end namespace llvm
 
 #endif