9
\$\begingroup\$

A very bare-bones timer class created using the std::chrono library. I would like to know if any optimisations are possible and just to clarity, I am using MSVC (this code will not compile with certain compilers).

Timer.h:

#pragma once

#include <chrono>

class Timer
{
private:
    std::chrono::time_point<std::chrono::steady_clock> m_StartTime;

public:
    void Start();
    float GetDuration();
};

Timer.cpp:

#include "Timer.h"

void Timer::Start()
{
    m_StartTime = std::chrono::high_resolution_clock::now();
}

float Timer::GetDuration()
{
    std::chrono::duration<float> duration = std::chrono::high_resolution_clock::now() - m_StartTime;
    return duration.count();
}
\$\endgroup\$
  • 1
    \$\begingroup\$ This looks more like a stopwatch, rather than timer. \$\endgroup\$ – Incomputable Jun 10 '18 at 23:05
  • 1
    \$\begingroup\$ I called my version Stopwatch. Feel free to browse and borrow any ideas you like. \$\endgroup\$ – Edward Jun 11 '18 at 12:25
13
\$\begingroup\$

Keep solid picture of usage in your head

When creating a library, it is extremely important to always keep the intended usage in mind. If the usage syntax is disgusting, it is better to think of a new one before writing the library (of course within reasons). In this case, I believe it is not what you wanted.

Make interfaces easy to use correctly and hard to use incorrectly

Start() function could be built into constructor. If user didn't use Start(), they'll get wrong results. Constructor, on the other hand, will enforce initialization. Never write non-fully-initializable objects, unless it is not possible to do otheriwse (extremely rarely). Then, users will only need to create object at the right place.

Don't mix clocks

Doing this:

std::chrono::time_point<std::chrono::steady_clock> m_StartTime;

and then

m_StartTime = std::chrono::high_resolution_clock::now(); 

is extremely bad idea. Not all standard libraries alias std::chrono::high_resolution_clock to steady_clock. Though I guess this is a feature of the code rather than a bug. The above will trigger compilation error in libc++ at least.

Know the subject

I know this is extremely simple clock, but since std::chrono::high_resolution_clock is used, I'd expect at least some mechanism to prevent compiler/execution unit optimizations, like std::atomic_thread_fence around it to prevent compiler optimizations and in general, make the execution point in which the time is taken observable. If none of what I said makes any sense to you, then your usage cases don't care about this.

Make it more reusable

I'd expect something like this:

template <typename Clock = std::chrono::high_resolution_lock>
class stopwatch
{
    /*...*/
};

Both time_point and duration are inferrable from Clock. I'd also like to see this:

template <typename Units = std::chrono::nanoseconds, typename Rep = double>
Rep elapsed_time()
{
    //ns chrono omitted for brevity
    return static_cast<Rep>(duration_cast<Units>(/*...*/).count());
}

(The static_cast is just to remove warnings when narrowing is done, which is intentional in this case, as default doesn't perform narrowing).

The project might suggest to change ordering of the template parameters, as Rep might be used more compared to Units.


Minor

  • Naming

CamelCase is usually used for type names in C++, the rest is camelCase. I personally prefer snake_case for everything, except template parameter and concept names, for which I use CamelCase. It is a matter of preference and consistency though.

  • Make header only if feasible

In this case, adding .cpp file makes it harder to use. .cpp file doesn't seem to make any difference, as any linkage nuances are not mentioned in the post. Also, making it header-only will make it even more simpler, which is I guess, the intention.


Putting it all together

Final code roughly looks like this:

#include <chrono>
#include <atomic>

namespace shino
{
    template <typename Clock = std::chrono::high_resolution_clock>
    class stopwatch
    {
        const typename Clock::time_point start_point;
    public:
        stopwatch() : 
            start_point(Clock::now())
        {}

        template <typename Rep = typename Clock::duration::rep, typename Units = typename Clock::duration>
        Rep elapsed_time() const
        {
            std::atomic_thread_fence(std::memory_order_relaxed);
            auto counted_time = std::chrono::duration_cast<Units>(Clock::now() - start_point).count();
            std::atomic_thread_fence(std::memory_order_relaxed);
            return static_cast<Rep>(counted_time);
        }
    };

    using precise_stopwatch = stopwatch<>;
    using system_stopwatch = stopwatch<std::chrono::system_clock>;
    using monotonic_stopwatch = stopwatch<std::chrono::steady_clock>;
}

As you probably noticed, I changed default template arguments in elapsed_time(), and also incorporated const correctness from @JDługosz answer. The defaults change is to support all possible clocks and still avoid lossy conversionsby default.

Lets use this:

Measuring scheduler overhead using stopwatch

The measurements are rather naive:

#include <iostream>
#include <thread>

int main()
{
    std::cout << "testing scheduler overhead\n";
    using namespace std::literals;
    for (auto wait_time = 100ms; wait_time <= 1s; wait_time += 100ms)
    {
        shino::precise_stopwatch stopwatch;
        std::this_thread::sleep_for(wait_time);
        auto actual_wait_time = stopwatch.elapsed_time<unsigned int, std::chrono::microseconds>();
        std::cout << "Scheduler overhead is roughly " << actual_wait_time - (wait_time + 0us).count() << " microseconds"
                  << " for " << wait_time.count() << " milliseconds of requested sleep time\n";
    }
}

(Weird +0us is to let compiler handle the conversion from ms to us)

On my system gives:

testing scheduler overhead
Scheduler overhead is roughly 114 microseconds for 100 milliseconds of requested sleep time
Scheduler overhead is roughly 110 microseconds for 200 milliseconds of requested sleep time
Scheduler overhead is roughly 75 microseconds for 300 milliseconds of requested sleep time
Scheduler overhead is roughly 111 microseconds for 400 milliseconds of requested sleep time
Scheduler overhead is roughly 111 microseconds for 500 milliseconds of requested sleep time
Scheduler overhead is roughly 107 microseconds for 600 milliseconds of requested sleep time
Scheduler overhead is roughly 110 microseconds for 700 milliseconds of requested sleep time
Scheduler overhead is roughly 110 microseconds for 800 milliseconds of requested sleep time
Scheduler overhead is roughly 112 microseconds for 900 milliseconds of requested sleep time
Scheduler overhead is roughly 93 microseconds for 1000 milliseconds of requested sleep time

Full code to copy and run

#include <chrono>
#include <atomic>

namespace shino
{
    template <typename Clock = std::chrono::high_resolution_clock>
    class stopwatch
    {
        const typename Clock::time_point start_point;
    public:
        stopwatch() : 
            start_point(Clock::now())
        {}

        template <typename Rep = typename Clock::duration::rep, typename Units = typename Clock::duration>
        Rep elapsed_time() const
        {
            std::atomic_thread_fence(std::memory_order_relaxed);
            auto counted_time = std::chrono::duration_cast<Units>(Clock::now() - start_point).count();
            std::atomic_thread_fence(std::memory_order_relaxed);
            return static_cast<Rep>(counted_time);
        }
    };

    using precise_stopwatch = stopwatch<>;
    using system_stopwatch = stopwatch<std::chrono::system_clock>;
    using monotonic_stopwatch = stopwatch<std::chrono::steady_clock>;
}

#include <iostream>
#include <thread>

int main()
{
    std::cout << "testing scheduler overhead\n";
    using namespace std::literals;
    for (auto wait_time = 100ms; wait_time <= 1s; wait_time += 100ms)
    {
        shino::precise_stopwatch stopwatch;
        std::this_thread::sleep_for(wait_time);
        auto actual_wait_time = stopwatch.elapsed_time<unsigned int, std::chrono::microseconds>();
        std::cout << "Scheduler overhead is roughly " << actual_wait_time - (wait_time + 0us).count() << " microseconds"
                  << " for " << wait_time.count() << " milliseconds of requested sleep time\n";
    }
}

Wandbox demo.

\$\endgroup\$
  • \$\begingroup\$ This answer gives tons of good advice but I feel that a synthesis (in the form of the final code) would be nice since it would look so drastically different from OP’s code. \$\endgroup\$ – Konrad Rudolph Jun 11 '18 at 13:55
  • \$\begingroup\$ @KonradRudolph, done. I guess the example usage is not the most common, but it is the only thing that came to my mind. \$\endgroup\$ – Incomputable Jun 11 '18 at 15:20
  • \$\begingroup\$ I believe the correct term would be sleep time resolution, but neither struck me as great, so I stick with the former. \$\endgroup\$ – Incomputable Jun 11 '18 at 15:30
5
\$\begingroup\$
float Timer::GetDuration()
{
    std::chrono::duration<float> duration = std::chrono::high_resolution_clock::now() - m_StartTime;
    return duration.count();
}

Why isn’t this a const member?

Is there some reason not to use auto when declaring the local variable duration?

\$\endgroup\$
1
\$\begingroup\$

I would leave the type and period explicitly to the user by templating them. The unit of the float is otherwise unclear.

#include <chrono> 

template<typename type = float, typename period = std::milli>
class stopwatch
{
public:
  using clock      = std::chrono::high_resolution_clock;
  using duration   = std::chrono::duration<type, period>;
  using time_point = std::chrono::time_point<clock, duration>;

  stopwatch           () : time_(clock::now()) { }
  stopwatch           (const stopwatch&  that) = default;
  stopwatch           (      stopwatch&& temp) = default;
 ~stopwatch           ()                       = default;
  stopwatch& operator=(const stopwatch&  that) = default;
  stopwatch& operator=(      stopwatch&& temp) = default;

  duration tick ()
  {
    time_point time  = clock::now();
    duration   delta = time - time_;
    time_            = time;
    return delta;
  }

private:
  time_point time_;
};
\$\endgroup\$
  • 2
    \$\begingroup\$ It would be great to add some explanations to why you think the code you presented is beneficial. It looks good just needs more explanations and motivations. \$\endgroup\$ – Incomputable Jun 11 '18 at 2:58

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.