[Support] Allow nesting paired calls to {start,stop}Timer

[vsk]

Summary

The old Timer implementation (before r256258) allowed nesting calls to {start,stop}Timer. Bring that behavior back and add a regression test for it.

[davide]

include/llvm/Support/Timer.h

122

Any reason why you can't query an active timer?

lib/Support/Timer.cpp

155

hmm, imagine the following sequence:
startTimer()
startTimer()
clear()
stop()
stop()

You hit an assertion, no?

[• rafael]

How hard would it be to change the callers that currently depend on this?

[davide]

In D20735#442381, @rafael wrote:

How hard would it be to change the callers that currently depend on this?

I have a patch that changes the clang timer where I hit this issue. I'm going to put it up for review.
The real worry on my side is that I'm not aware of all the possible cases where we rely on this behavior.
We can just drop Vedant's patch on the floor, and wait until we hit the assertions, and fix the bugs as they show up.
What's your opinion?

[• rafael]

It seems pretty odd to have a timer that can be started multiple times. If
we can avoid it I think we should.

Cheers,
Rafael

[vsk]

@rafael we don't know how hard it would be to change users which expect to be able to nest {start,stop}Timer. In CodeGenAction.cpp alone it seems like there could be many such users.

The real worry on my side is that I'm not aware of all the possible cases where we rely on this behavior.

Right, that's what I'm concerned about too. I don't see any advantages of keeping the stricter (i.e non-nestable) {start,stop}Timer implementation.

include/llvm/Support/Timer.h

122

You'd get back a negative time, since startTimer() does Time -= CurrentTime.

lib/Support/Timer.cpp

155

Yes. It seems correct to me that you'd hit an assertion in that case. clear() would reset the total time, so there is nothing to stop.

[• rafael]

I am OK with this if it is cumbersome to use a non ref counted timer in the
current users.

[vsk]

In D20735#442417, @rafael wrote:

It seems pretty odd to have a timer that can be started multiple times. If
we can avoid it I think we should.

You're right that this is odd behavior, and ISTM the original API avoided advertising it on purpose.

Even so, nesting timers does make sense in the context of recursive functions. If we have e.g: foo() { T.start(); if (...) { foo(); } T.stop(); }, then it's natural for the timer to measure all the time spent in foo, including time spent setting up a timer. Without a change like this, we'd make that more difficult.

[davide]

In D20735#442427, @rafael wrote:

I am OK with this if it is cumbersome to use a non ref counted timer in the
current users.

http://reviews.llvm.org/D20735

[davide]

I think you can close this, Vedant. My fix for clang landed instead.

[vsk]

@davide thanks, closing

include/llvm/Support/Timer.h

 /// 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 never started.
 ///
 class Timer {
   TimeRecord Time;       // The total time captured
-  TimeRecord StartTime;  // The time startTimer() was last called
   std::string Name;      // The name of this time variable.
-  bool Running;          // Is the timer currently running?
+  unsigned RefCount;     // How many times has this timer been started?
   bool Triggered;        // Has the timer ever been triggered?
   TimerGroup *TG;        // The TimerGroup this Timer is in.
 
   Timer **Prev, *Next;   // Doubly linked list of timers in the group.
 public:
   explicit Timer(StringRef N) : TG(nullptr) { init(N); }
   Timer(StringRef N, TimerGroup &tg) : TG(nullptr) { init(N, tg); }
   Timer(const Timer &RHS) : TG(nullptr) {
public:
 
   /// Stop the timer.
   void stopTimer();
 
   /// Clear the timer state.
   void clear();
 
   /// Return the duration for which this timer has been running.
-  TimeRecord getTotalTime() const { return Time; }
+  TimeRecord getTotalTime() const {
+    assert(RefCount == 0 && "Cannot query active timer");
+    return Time;
+  }
 
 private:
   friend class TimerGroup;
 };
 
 /// The TimeRegion class is used as a helper class to call the startTimer() and
 /// stopTimer() methods of the Timer class.  When the object is constructed, it
 /// starts the timer specified as its argument.  When it is destroyed, it stops

lib/Support/Timer.cpp

 //===----------------------------------------------------------------------===//
 
 void Timer::init(StringRef N) {
   init(N, *getDefaultTimerGroup());
 }
 
 void Timer::init(StringRef N, TimerGroup &tg) {
   assert(!TG && "Timer already initialized");
+  Time = TimeRecord();
   Name.assign(N.begin(), N.end());
-  Running = Triggered = false;
+  Triggered = false;
+  RefCount = 0;
   TG = &tg;
   TG->addTimer(*this);
 }
 
 Timer::~Timer() {
   if (!TG) return;  // Never initialized, or already cleared.
   TG->removeTimer(*this);
 }

 
   Result.WallTime   =  now.seconds() +  now.microseconds() / 1000000.0;
   Result.UserTime   = user.seconds() + user.microseconds() / 1000000.0;
   Result.SystemTime =  sys.seconds() +  sys.microseconds() / 1000000.0;
   return Result;
 }
 
 void Timer::startTimer() {
-  assert(!Running && "Cannot start a running timer");
-  Running = Triggered = true;
-  StartTime = TimeRecord::getCurrentTime(true);
+  Triggered = true;
+  if (!RefCount)
+    Time -= TimeRecord::getCurrentTime(true);
+  ++RefCount;
 }
 
 void Timer::stopTimer() {
-  assert(Running && "Cannot stop a paused timer");
-  Running = false;
-  Time += TimeRecord::getCurrentTime(false);
-  Time -= StartTime;
+  assert(RefCount > 0 && "Cannot stop a paused timer");
+  --RefCount;
+  if (!RefCount)
+    Time += TimeRecord::getCurrentTime(false);
 }
 
 void Timer::clear() {
-  Running = Triggered = false;
-  Time = StartTime = TimeRecord();
+  RefCount = 0;
+  Triggered = false;
+  Time = TimeRecord();
 }
 
 static void printVal(double Val, double Total, raw_ostream &OS) {
   if (Total < 1e-7)   // Avoid dividing by zero.
     OS << "        -----     ";
   else
     OS << format("  %7.4f (%5.1f%%)", Val, Val*100/Total);
 }

unittests/Support/TimerTest.cpp

TEST(Timer, CheckIfTriggered) {
   EXPECT_TRUE(T1.hasTriggered());
   T1.stopTimer();
   EXPECT_TRUE(T1.hasTriggered());
 
   T1.clear();
   EXPECT_FALSE(T1.hasTriggered());
 }
 
+TEST(Timer, Nesting) {
+  Timer T1("T1");
+  T1.startTimer();
+  T1.startTimer();
+  T1.stopTimer();
+  T1.stopTimer();
+  EXPECT_TRUE(T1.hasTriggered());
+}
+
 } // end anon namespace