Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I had time measuring pretty much figured out when I ended up using this structure.

#include <iostream>
#include <chrono>

template<typename TimeT = std::chrono::milliseconds>
struct measure
{
    template<typename F>
    static typename TimeT::rep execution(F const &func)
    {
        auto start = std::chrono::system_clock::now();
        func();
        auto duration = std::chrono::duration_cast< TimeT>(
            std::chrono::system_clock::now() - start);
        return duration.count();
    }
};

a use case would be

struct functor
{
    int state;
    functor(int state) : state(state) {}
    void operator()() const
    {
        std::cout << "In functor run for ";
    }
};

void func()
{
    std::cout << "In function, run for " << std::endl;
}

void func(int arg) {}

int main()
{
    int dummy(1);

    std::cout << measure<>::execution( [&]() {  
        func(dummy);
    }) << std::endl;

    std::cout << measure<>::execution(functor(dummy)) << std::endl;
std::cout << measure<>::execution(func);

    return 0;
}

But since I started using my struct in various posts on Stack Exchange sites, I've had remarks and comments about the code (like on func being a const ref or not etc).

I'd like a review on the above and your suggestions on how I can approve this code (or leave it alone at last).

share|improve this question
1  
Your class won't work with functions which need paramaters. Is it supposed to be so? –  black May 3 at 18:42
    
It'll work, just call the function throught a lambda –  Nikos Athanasiou May 3 at 18:45
    
@ NikosAthanasiou Yes, that's possible. But measurements will be out of phase, since you are measuring the other function's execution time too. –  black May 3 at 18:50
    
@black Lambda functions are always inlined, so the code is called in place. Besides, for any measurement to take place, a single function call (which as stated before will be inlined) is not a factor. –  Nikos Athanasiou May 3 at 18:53
    
@NikosAthanasiou As you prefer. But consider that it is neither logical nor expected to work that way. –  black May 3 at 19:07

4 Answers 4

up vote 10 down vote accepted
  1. Extend your timer function so it can take all the parameters needed by func()
  2. Don bother to pass func by const reference.

What I would have done

template<typename TimeT = std::chrono::milliseconds>
struct measure
{
    template<typename F, typename ...Args>
    static typename TimeT::rep execution(F func, Args&&... args)
    {
        auto start = std::chrono::system_clock::now();

        // Now call the function with all the parameters you need.
        func(std::forward<Args>(args)...);

        auto duration = std::chrono::duration_cast< TimeT>(std::chrono::system_clock::now() - start);

        return duration.count();
    }
};
share|improve this answer
    
+1 . Those variadic templates solve so many problems! Would you recommend a source where I can learn more ? (please don't say the Standard) –  Nikos Athanasiou May 3 at 19:34
    
@NikosAthanasiou: This person seems to know what he is talking about. codereview.stackexchange.com/users/39083/iavr read some of his answers –  Loki Astari May 3 at 19:36
    
Quick question. Why can't it compile the original example ? I'm supposing due to the overloads func, but can I have any thoughts on that? –  Nikos Athanasiou May 3 at 19:52
1  
@NikosAthanasiou: Its because you have two functions named func(). It does not know which one you mean when you go measure<>::execution(func); (it could be either). You can either name the functions differently or you can be specific when specifying them by providing the functions type measure<>::execution<void()>(func);. –  Loki Astari May 3 at 23:20

It seems a bit of a bother to approach it as a function template when it seems to me that it's more like an object. For that reason, here's an alternative that might be a little more natural to use:

template<typename TimeT = std::chrono::microseconds, 
    typename ClockT=std::chrono::high_resolution_clock,
    typename DurationT=double>
class Stopwatch
{
private:
    std::chrono::time_point<ClockT> _start, _end;
public:
    Stopwatch() { start(); }
    void start() { _start = _end = ClockT::now(); }
    DurationT stop() { _end = ClockT::now(); return elapsed();}
    DurationT elapsed() { 
        auto delta = std::chrono::duration_cast<TimeT>(_end-_start);
        return delta.count(); 
    }
};

Then you can use it like this:

Stopwatch<> sw;
functionToBeTimed(arg1, arg2);
sw.stop();
std::cout << "functionToBeTimed took " << sw.elapsed() << " microseconds\n";
share|improve this answer
    
Disagree with more natural. But this has the advantage you can time multiple function calls. –  Loki Astari May 3 at 23:29
1  
You can also time things that are not functions. –  Edward May 4 at 0:38
2  
I would be careful about timing things that are not functions because of optimizations that the compiler is allowed to do. I am not sure about the sequencing requirements and the observable behavior understanding of such a program. With a function call the compiler is not allowed to move statements across the call point (they are sequenced before or after the call). –  Loki Astari May 6 at 15:52
    
+1. I find this quite natural. I usually write my own timer object like that, with members called tic(), toc() (instead of start(), stop()), following the matlab style. Plus tac(), which measures elapsed time without stopping the timer. But all this is complementary rather than competitive to the proposed measure() function template (including parameters). Why not have all in a timer object? –  iavr May 7 at 0:53
    
@iavr: good point about them being complementary mechanisms. Also, while I never liked the overly cute matlab names, I'll point out that stop() isn't an entirely accurate name and actually functions more like toc() in that neither stop() nor toc() actually stop the timer. –  Edward May 7 at 1:51

You should use std::chrono::steady_clock instead of std::chrono::system_clock. This is because system_clock may get adjusted every now and then (e.g. it may rewind back a bit, or jump forward more than the actual elapsed time in order to keep in sync with the wall/world clock), whereas steady_clock is a monotonic clock that is never adjusted. Therefore, steady_clock is better suited for what you're doing.

P.S. If you need to know how much CPU-time your function is actually taking, even steady_clock may not be ideal, as it will only measure the duration from the time your function is called, to the time it returns. This may not necessarily be equal to the amount of CPU-time the function took, as other processes and threads can interrupt your function and result in significantly longer execution times. If you want to isolate the effects of other system activities, you should google OS/platform-specific alternatives for getting the thread execution time or look up the chrono functions in Boost (look for the process_...cpu_clock and thread_clock classes).

share|improve this answer

There isn't much more to improve after the code has begun to accept function parameters.

These are some details, you could freely ignore them:

  • Class names should begin with an uppercase letter

    struct Measure
    
  • You don't need the duration variable; just do:

    return std::chrono::duration_cast< TimeT>(std::chrono::system_clock::now() - start).count();
    
share|improve this answer
2  
I agree that it is a good idea to have type names begin with Upper case letters (seems to be a common practice). But should is a bit strong. –  Loki Astari May 3 at 23:25
2  
Disagree on removing the variable duration. The compiler will remove all wasted space so the code will still be optimal (ie no different than your version) with the variable. But it helps in reading the code (and code is for the human to read so make it easy on us humans and leave the variables in to make it easier to read). Also when debugging it makes it easier to check the result when you have it stored in a variable. –  Loki Astari May 3 at 23:27
    
I kind of disagree on both: duration removal for the same reason as Loki, class name capitalization because you are free to choose your own code style (actually standard library classes are not capitalized, aren't they?) –  pqnet Aug 6 at 4:58

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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