Constructor start, destructor end timer in C++

Question

The following is a short timing utility I've written for a larger project that I'm working on. The larger project is designed to be called (via ffi) from a Scala/Java codebase, which makes using standard profiling tools somewhat difficult - especially at the granularity that we want.

Essentially, we want to be able to track roughly how long a few key functions are taking, and we want a low overhead way of doing so. The following code is my implementation of a timer class that starts timing when constructed, and then finishes (and reports data) when it destructs.

I chose this way of doing things as it gives quite a succinct way of benchmarking a function, take for example, a function foo that we wanted to benchmark, all we would have to do is the following:

void foo(/*some arguments*/) {
  auto timer = CSDSTimer("foo", "example");
  /*do some more stuff*/
}

I think this is quite a nice solution, but I'd be interested in feedback on my implementation, especially of my handling of streams, and whether there's any way to set the stream statically (see the commented out code).

Thanks!

#pragma once

#include <chrono>
#include <ctime>
#include <iostream>

class CSDSTimer {

public:
  // Construct and start the timer
  CSDSTimer(std::string name, std::string context = "global",
            std::ostream &stream = std::cout)
      : _name(name), _context(context), _default_str(&stream) {
    _start = std::chrono::system_clock::now();
  }

  // Destruct and stop the timer, reporting the elapseed time
  ~CSDSTimer() {
    _end = std::chrono::system_clock::now();
    std::chrono::duration<double> elapsed_seconds = _end - _start;
    *_default_str << "PROFILING_DATUM(\"" << _name << "\", \"" << _context
                  << "\", " << elapsed_seconds.count() * 1000 << ", \"C++\")"
                  << std::endl;
  }

  // static void SetStream(std::ostream &str) { _default_str = &str; }

private:
  std::string _name;
  std::string _context;
  std::chrono::time_point<std::chrono::system_clock> _start, _end;
  std::ostream *_default_str;

  // static std::ostream *_default_str;
};

Answer 1

Use aliases

Instead of writing std::chrono::system_clock:: everywhere, simply create an alias:

using clock_type = std::chrono::system_clock;

This is a good scenario for an alias because it shortens your typing. More importantly, you have defined what clock type you use in one specific place; this allows you to change the clock type at one spot and have it affect your entire class.

You could even make it a template parameter if you want to allow users to provide their own clock type.

---

Constructor

I'm not sure why you initialize _start inside the constructor body instead of the constructor initializer list.

CSDSTimer(std::string name, std::string context = "global",
    std::ostream &stream = std::cout)
    : _name(name)
    , _context(context)
    , _default_str(&stream)
    , _start{ clock_type::now() } // init here instead of ctor body; note alias usage
{}

Additionally, it might be cleaner if you add some new lines like I did, but in all honesty it's not that hard to read... so this one's up to you.

---

Destructor

Here, we notice that your only usage of _end is in the destructor. You don't provide any query member functions, so let's just go ahead and remove _end altogether. This will make the class smaller.

You also want milliseconds, but first take seconds and then multiply by 1000. You can directly get milliseconds by using std::chrono::duration_cast().

~CSDSTimer()
{
    // we can directly compute the elapsed time without storing `_end`
    auto elapsed_ms = std::chrono::duration_cast<std::chrono::milliseconds>(
            clock_type::now() - _start);
    
    *_default_str << "PROFILING_DATUM(\"" << _name << "\", \"" << _context
        << "\", " << elapsed_ms.count() << ", \"C++\")" // no more multiply by 1000
        << std::endl;
}

There is a small subtle bug you can avoid by doing it like shown above, but I will describe it so you can be on the look out in the future.

The bug is that the elapsed time is stored in a std::chrono::duration<double>, but _end - _start basically returns std::chrono::nanoseconds. How is this an issue? Well, std::chrono::nanoseconds must be represented by a signed integer type of at least 64 bits (see: http://en.cppreference.com/w/cpp/chrono/duration).

While a double does have 64 bits, it's certainly lacking the ability to represent values from the integer point of view. This would result in a loss of precision/erroneous results.

Answer 2

Design:

The problem I have is that you need to modify the thing you want to time (which means changing the timing).

  void functionIWantToTime()
  {
       CSDSTimer time("functionIWantToTime");

       // Stuff that takes time
  }

What would be better is a way to time a function without modifying it.

 CSDSTimer("functionIWantToTime", &functionIWantToTime);

This also allows you to time random set of stuff that is not a function by using a lambda to combine them.

 CSDSTimer("Stuff", [](){
     function1();
     function2();
 )};

Code Review

Pass big parameters by const reference

To avoid copying objects pass by const reference.

CSDSTimer(std::string name, std::string context = "global", std::ostream &stream = std::cout)
                  ^^^^^^ Add const&  ^^^^^

Avoid holding pointers:

std::ostream *_default_str;

You pass by reference. Why not store a reference?

Prefer '\n' over std::endl

The difference is a flush. Flushing is usually sub-optimal; the stream will flush at the optimal time. Manual flushing usually just retards performance.

Timer Accuracy

I did not spot it. But @hoffmale suggested using a more accurate timer. I agree that is probably a good idea (you have nothing to lose with more accuracy).