patterncppMinor
Measuring executing time
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
Example:
```
#include "Timer.h"
#include
#include
void fo
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{};
};
}
#endifExample:
```
#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:
As you can see, single
it will be just
If it is so wanted, you can write freestanding
What is done right:
The code is really generic, which is nice for portability reasons.
Rule of five nailed down.
Bad stuff:
This line is like calling for a challenge "Try to understand me!".
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
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.