HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Measuring executing time

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
executingtimemeasuring

Problem

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 
#include 
#include 
#include 

namespace library {

    // Default unit: std::chrono::milliseconds
    template
    class Timer {
    public:
        using Clock = std::conditional_t;

        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 and find the unit.
            @return The unit name.
        */
        const char* unit_name() {
            const std::map 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 
                (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
#include

void fo

Solution

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
class session {
    Func function;
    std::vector> timings;
public:
    session(const Func& f) :
            function(f)
    {}

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

    template
    void measure(Args &&... args) {
        auto start = std::chrono::high_resolution_clock::now();
        function(std::forward(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
    void get_as(OutputIt first, OutputIt last) {
        auto it = timings.begin();
        for (; it != timings.end() && first != last;
               ++first, ++it) {
            *first = std::chrono::duration_cast(*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 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.

Code Snippets

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);
        }
    }
};
session<T> s(t);
session s(t);
(m_name == "" ? ": " : " (" + m_name + "): ");
m_end = Clock::now();
 output();

Context

StackExchange Code Review Q#151768, answer score: 4

Revisions (0)

No revisions yet.