Avoiding spurious construction and destruction in RAII timer object

Question

I have a Timer object which should time the region of code from its construction to its destruction. These Timer objects are created by and associated with a long-lived TimerManager object. In fact, the Timer objects are just thin wrappers around a pointer to TimerManager which does the heavy lifting.

I'd like the user the Timer objects like this:

TimerManager manager; // maybe a global, thread local, whatever

...
{
  Timer timer = manager.newTimer();
  // code under test

} // timer object destroyed and timer stops here

The Timer object can be as simple as:

class Timer {
  TimerManager* manager_;
public:
  Timer(TimerManager* manager) : manager_(manager) {
    manager_->start();
  }

  ~Timer() {
    manager_->stop();
  }
};

Here all the heavy lifting of starting and stopping the timer is delegated to the manager.

However, if I implement TimerManager::newTimer() like so:

TimerManager::newTimer() {
  Timer t(this);
  // ...
  return t;
}

Then depending on whether RVO kicks in, I may get a spurious construction and destruction of Timer object t, different from the real region I want to time in the calling code.

I could instead use the following code to initialize Timer objects:

{
  Timer timer(&manager);
  // code under test

} // timer object destroyed and timer stops here

This ensures extra Timer objects are not created or destroyed, but I prefer the assignment syntax, especially since it lets me have various newTimer() type methods with different behavior. Is there any way to get something like this w/o having the extra side effects of Timer creation and destruction.

Performance matters here.

I am not using C++17 so I cannot avail myself of guaranteed return value optimization.

Well, even though you prefer the assignment syntax you cannot use it because you do not have guaranteed RVO pre C++17. Either update to C++17, or use the alternative syntax. This is pretty much cut and dry. I see no workaround, here, except perhaps have `newTimer()` return a `std::shared_ptr`. But, since you claim that "performance matters", this translates to two extra heap allocations, one for the `Timer`, and one for the reference counter that `std::shared_ptr` independently allocates. Those are, pretty much, your only choices. Pick one. — Sam Varshavchik, Jul 28 '19 at 01:41
@SamVarshavchik - I see some alternatives: e.g., the `newTimer()` method could return some type of proxy object rather than a `Timer` directly, which could wrap the same pointer, but without the construction/destruction semantics. Or maybe the `Timer` object could use a reference count to detect the "last" destruction. Those don't seem all that great, but it makes me think that it is not all that cut and dry and that better may exist. — BeeOnRope, Jul 28 '19 at 01:44
Give `Timer` a move constructor and move assignment operator. Don't call `start` there. Reset `manager_` to `nullptr` in moved-from object, so its destructor doesn't call `stop`. Basically, `Timer` will be similar to `unique_ptr` — Igor Tandetnik, Jul 28 '19 at 01:50
@IgorTandetnik: Basically `Timer` should be based on `std::unique_ptr` (containment or private inheritance) and then all the right special member functions immediately work and the right ones are implicitly deleted. — Ben Voigt, Jul 28 '19 at 02:52

L. F. · Answer 1 · 2019-07-28T02:57:09.090

3

You need to make the manager noncopyable and provide the appropriate move operations. The move operations should transfer the resource and set the moved-from manager to nullptr. The destructor should be able to handle the nullptr case. As in:

class Timer {
  TimerManager* manager_;
public:
  Timer(TimerManager* manager) : manager_(manager) {
    manager_->start();
  }

  Timer(const Timer&) = delete; // noncopyable

  Timer(Timer&& timer)          // move constructor
    :manager_{nullptr}
  {
    swap(*this, timer);
  }

  Timer& operator=(Timer timer) // (move-only) assignment operator
  {
    swap(*this, timer);
    return *this;
  }

  friend void swap(Timer& lhs, Timer& rhs)
  {
    swap(lhs.manager_, rhs.manager_);
  }

  ~Timer() {                    // take care of nullptr
    if (manager_)
      manager_->stop();
  }
};

I used the copy-and-swap idiom here. This way, if the Timer is returned, as in

TimerManager::newTimer() {
  Timer t(this);
  // ...
  return t;
}

Then t is moved instead of copied. Only pointers are passed and the timer is not interrupted. And the timer is only started and stopped once.

Also, the whole thing is unnecessary if you make effective use of the library, i.e., unique_ptr with a custom deleter:

struct Stopper {
    void operator()(TimerManager* tm)
    {
        tm->stop();
    }
};

class Timer {
    std::unique_ptr<TimerManager, Stopper> manager_;
public:
    Timer(TimerManager* manager)
        :manager_{manager}
    {
        manager_->start();
    }

    // everything is automatically correct
};

edited Jul 28 '19 at 02:57

answered Jul 28 '19 at 02:15

L. F.

19,445
8
48
82

@BenVoigt My theory is that the assignment operator can only be called with an rvalue, so there is no copy after all. What am I missing? – L. F. Jul 28 '19 at 02:29
@BenVoigt Well, is swapping with the rvalue directly good practice I wonder? My first impression is that the assignment operator should terminate the current timer, but I may be wrong. – L. F. Jul 28 '19 at 02:37
@BenVoigt But that way we have to duplicate the destructor code. – L. F. Jul 28 '19 at 02:43
@BenVoigt If you don't stop the timer in the move assignment, the pointer to the timer is lost. How to stop it then? – L. F. Jul 28 '19 at 02:47
@BenVoigt The self-assignment check may still be omitted with `manager_ = exchange(other.manager_, nullptr)`, though, so I don't consider it an advantage of the current version – L. F. Jul 28 '19 at 02:49
1

Oh you mean the one formerly owned by lhs... I had overlooked that. Initially I was thinking of a call to `reset()` (which would have run the cleanup on the existing value) but then I saw that the pointer was raw. Really the whole thing could be done with `std::unique_ptr` for the type of the member pointer, and then the Rule of Zero applies and you don't need to write move constructor, assignment, or swap yourself, and write `TimerStopper` instead of a destructor. – Ben Voigt Jul 28 '19 at 02:50
@BenVoigt Agree on `unique_ptr`. I'm updating my answer. – L. F. Jul 28 '19 at 02:53
Let us [continue this discussion in chat](https://chat.stackoverflow.com/rooms/197107/discussion-between-l-f-and-ben-voigt). – L. F. Jul 28 '19 at 02:58

score 0 · Answer 2 · answered Jul 28 '19 at 08:16

If you can reorganize your function to:

Timer TimerManager::newTimer() {
   // ...
   t(this);
   return t; // Depend of NRVO :(
}

then you might use copy-list-initialization which construct returned object in place:

Timer TimerManager::newTimer() {
   // ...
   return {this}; // construct returned object in place, as NRVO :-)
}

But you still have possible move with:

Timer timer = manager.newTimer();

You might change it to:

const Timer& timer = manager.newTimer();

or

Timer&& timer = manager.newTimer();

deleting copy and move constructor makes that construct viable.

Demo

Avoiding spurious construction and destruction in RAII timer object

2 Answers2