4
\$\begingroup\$

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;
};
\$\endgroup\$
  • 1
    \$\begingroup\$ Not a fan of your design because it requires changing functions that have already been written. I personally like this design. \$\endgroup\$ – ncalmbeblpaicr0011 Aug 2 '17 at 18:58
  • 1
    \$\begingroup\$ 2 questions: why do you use system_clock instead of the possibly more accurate high_resolution_clock? And why do you calculate elapsed_seconds as seconds, but manually convert it to milliseconds for output (you could do the calculations for milliseconds directly, which might be more accurate b/c of floating point weirdness)? \$\endgroup\$ – hoffmale Aug 2 '17 at 19:01
  • \$\begingroup\$ Why a pointer to stream rather than a reference? You never check it's not null. \$\endgroup\$ – Toby Speight Aug 2 '17 at 19:44
2
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ I'll definitely alias the clock, and change it to a higher resolution one as well in the process - thanks for the tip! I hadn't realised that I could construct start like that, so I'll do that in the initialiser list as well. The list is formatted without lines as that's how clang-format formats it with the LLVM styling, so it's not really under my control. I hadn't spotted the bug in the destructor though - thanks for pointing it out - I'll modify it to fix it! \$\endgroup\$ – AdamHarries Aug 3 '17 at 9:54
2
\$\begingroup\$

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).

\$\endgroup\$
  • \$\begingroup\$ Thanks for the feedback! In terms of design, I've specifically written it this way round (rather than wrapping functions, as you suggest), as I'm not trying to benchmark functions, but profile a much larger application. I realise that profiling at the call site might be better, but as I control the whole codebase, there isn't much downside to modifying the functions that I want to extract data from. \$\endgroup\$ – AdamHarries Aug 3 '17 at 9:48
  • \$\begingroup\$ I'll change the parameters to const references, and the default str to a reference. I've used std::endl as my understanding was that it was more portable? Given that my application is fairly performance sensitive though I'll change that to an '\n'! \$\endgroup\$ – AdamHarries Aug 3 '17 at 9:50
  • 1
    \$\begingroup\$ std::end; is not more portable. These two have the same affect: out << std::endl; and out << '\n' << std::flush; They both output '\n' to the stream. If your stream is in text mode (ie not binary) then \n will be converted to the platform specific end of line sequence (but this applies to both methods). \$\endgroup\$ – Martin York Aug 3 '17 at 18:29

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.