6
\$\begingroup\$

I've written a small piece of code to measure execution time. As this is my first attempt at something meaningful, I would really like some input on ways to improve my code.

Timer.h

/**
    Purpose: Measuring execution time using chrono.

    @author Rick Regeling
    @version 1.0 2017/01/05
*/

#ifndef TIMER_H
#define TIMER_H

#include <chrono>
#include <iostream>
#include <string>
#include <map>

namespace library {

    // Default unit: std::chrono::milliseconds
    template<typename Unit = std::chrono::milliseconds>
    class Timer {
    public:
        using Clock = std::conditional_t<std::chrono::high_resolution_clock::is_steady,
            std::chrono::high_resolution_clock,
            std::chrono::steady_clock>;

        Timer() = default;
        Timer(const std::string& name) : m_name{ std::move(name) } {}
        ~Timer() {
            m_end = Clock::now();
            output();
        }

        // Disabled operations:
        Timer(const Timer&) = delete;
        Timer(Timer&&) = delete;
        Timer& operator=(const Timer&) = delete;
        Timer& operator=(Timer&&) = delete;
    private:
        /**
            Build a map<const char*, const char*> and find the unit.
            @return The unit name.
        */
        const char* unit_name() {
            const std::map<const char* const, const char*> units{
                { typeid(std::chrono::hours).name(),"h" },
                { typeid(std::chrono::minutes).name(), "m" },
                { typeid(std::chrono::seconds).name(), "s" },
                { typeid(std::chrono::nanoseconds).name(), "ns" },
                { typeid(std::chrono::microseconds).name(), "mms" },
                { typeid(std::chrono::milliseconds).name(), "ms" } };
            auto result = units.find(typeid(Unit).name());
            return result->second;
        }

        /**
            Output the information.
        */
        void output() {
            std::cout << "Elapsed time";
            // Determine if a name is used:
            std::cout << (m_name == "" ? ": " : " (" + m_name + "): ");
            // Calculate the total time and get the unit:
            std::cout << (std::chrono::duration_cast<Unit>
                (m_end - m_start).count()) << unit_name() << '\n';
        }

        // Members:
        const Clock::time_point m_start = Clock::now();
        Clock::time_point m_end{};
        const std::string m_name{};
    };
}
#endif

Example:

#include "Timer.h"
#include <vector>
#include <numeric>

void foo() {
    std::vector<int> v(1'000'000, 42);
    std::accumulate(std::begin(v), std::end(v), 0);
}

int main() {
    // default behavior:
    {
        library::Timer<> timer;
        foo();
    }

    // different unit + description:
    {
        library::Timer<std::chrono::nanoseconds> ex_timer("foo");
        foo();
    }
    return EXIT_SUCCESS;
}
\$\endgroup\$
3
  • 2
    \$\begingroup\$ I don't mean this to discourage your efforts to learn how to improve your own code, but if you're serious about measuring execution time, you should be aware of Google's benchmarking library. In fact, studying the code could itself be an educational experience. \$\endgroup\$ Commented Jan 5, 2017 at 15:43
  • \$\begingroup\$ Don't worry about discouraging me, that won't happen. This is my first attempt at programming and I am here to learn, so I appreciate any advise. Thank you for the link! \$\endgroup\$ Commented Jan 5, 2017 at 20:47
  • \$\begingroup\$ Ah, well, for your first attempt at programming, that will almost certainly be overkill. Focus on correctness first, before you start worrying about execution time. And when you do worry about execution time, think in terms of high level algorithmic efficiency (i.e., does the code have a linear or exponential run-time). This is easier to reason about and has an obvious enough impact that you won't need to benchmark it. Leave the benchmarking and micro-optimization for later. It's tricky, often counterintuitive, and requires lots of low-level knowledge. Not for everyone. \$\endgroup\$ Commented Jan 6, 2017 at 5:28

1 Answer 1

4
\$\begingroup\$

Usage:

Famous Scott Meyers' words: "Make interfaces easy to use correctly and hard to use incorrectly". It should be kept in mind.

It is really cumbersome to use. Users have to create different scopes and they don't have any control over what's being output and where it's being output.

Also, it seems like the class is somewhat one shot, so users can't make multiple runs. Usually people make considerable amount of runs to normalize the numbers and diminish the noise. The interface could emphasize that.

It is needed to keep in mind that the responsibility of this class is to measure the timings. Don't do anything with it, just provide a way to read it.

Alternative solution:

template<typename Func>
class session {
    Func function;
    std::vector<std::chrono::duration<double>> timings;
public:
    session(const Func& f) :
            function(f)
    {}

    session(Func&& f) :
            function(std::move(f)) {}

    template<class ... Args>
    void measure(Args &&... args) {
        auto start = std::chrono::high_resolution_clock::now();
        function(std::forward<Args>(args)...);
        auto end = std::chrono::high_resolution_clock::now();

        timings.push_back(end - start);
    }

    //writes into the range until either exhausts it or timings vector is exhausted
    template<typename Unit, typename OutputIt>
    void get_as(OutputIt first, OutputIt last) {
        auto it = timings.begin();
        for (; it != timings.end() && first != last;
               ++first, ++it) {
            *first = std::chrono::duration_cast<Unit>(*it);
        }
    }
};

As you can see, single measure() call doesn't return the time, but rather get_as() is used to get all of the measurements at once, and throw it into some function that computes average and generally does processing of the collected data. The interface does little dictating on what users should do, and provides enough flexibility. In C++17, there even won't be a need to write

session<T> s(t);

it will be just

session s(t);

If it is so wanted, you can write freestanding measure() function to get timings of one function call.

What is done right:

The code is really generic, which is nice for portability reasons.

Rule of five nailed down.

Bad stuff:

(m_name == "" ? ": " : " (" + m_name + "): ");

This line is like calling for a challenge "Try to understand me!".

 m_end = Clock::now();
 output();

My opinion is that if you don't initialize a member variable during construction, then the member variable is a candidate for local variable and being passed by reference/const reference.

Don't postpone to runtime:

Get the string for units right in the beginning using the fact that units are aliases for std::ratio<>s.

\$\endgroup\$
1
  • \$\begingroup\$ I appreciate your answer Incomputable, thanks! I was going for a class that I can use to test small pieces of code, algorithms and differences between member functions. It was actually a lot smaller before, but I decided that if I want to change the units that it should also print the unit correctly. I don't even know how efficient it was written and I spend hours on finding ways to output the correct unit matching the one used in the template. I like your way of thinking! I will try something else next time. Thanks again! \$\endgroup\$ Commented Jan 5, 2017 at 20:57

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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