A header only class for C++ time measurement
$begingroup$
I wrote a header only class (actually there is a second one for data bookkeeping and dumping) to measure the execution time of a C++ scope without worrying too much about boilerplates. The idea being to be able to simply instantiate a class at the begining of the scope you want to measure, and do a single call at the end to dump the measure.
I rely on the fact that the class instantiation is at the beginning of the scope, and its destruction at the very end. So my main worry is about compile time optimizations that could change the order of execution and bias the measure. Also I'm not satisfy on how I retrieve the type for ScopeTimer::Duration
but I couldn't find the type properly :/
Here is the code:
scope_timer.hpp
#ifndef SCOPE_TIMER
#define SCOPE_TIMER
#include <chrono>
#include <string>
#include <vector>
#include <map>
#include <fstream>
class ScopeTimer {
public:
using ScopeSignature = std::string;
using DurationType = std::chrono::microseconds;
using Duration = decltype(std::chrono::duration_cast<DurationType>(std::chrono::high_resolution_clock::now() - std::chrono::high_resolution_clock::now()).count());
ScopeTimer(const ScopeSignature& scopeName);
~ScopeTimer();
Duration getDurationFromStart() const;
private:
ScopeTimer();
const ScopeSignature scopeName;
const std::chrono::high_resolution_clock::time_point start;
};
class ScopeTimerStaticCore {
public:
static void addTimingToNamedScope(const ScopeTimer::ScopeSignature& scopeName, const ScopeTimer::Duration& duration);
static void dumpTimingToFile(const std::string& path);
static void clearAllTiming();
static void clearTimingForNamedScope(const ScopeTimer::ScopeSignature& scopeName);
private:
using TimingVector = std::vector<ScopeTimer::Duration>;
using ScopesTiming = std::map<ScopeTimer::ScopeSignature, TimingVector>;
static ScopesTiming& getScopesTimingStaticInstance() {
static ScopesTiming scopesTimingContainer;
return (scopesTimingContainer);
};
};
/*******************************************************Implementations*******************************************************/
inline ScopeTimer::ScopeTimer(const ScopeSignature& scopeName) : scopeName(scopeName), start(std::chrono::high_resolution_clock::now()) {};
inline ScopeTimer::~ScopeTimer() {
const Duration scopeTimerLifetimeDuration = this->getDurationFromStart();
ScopeTimerStaticCore::addTimingToNamedScope(this->scopeName, scopeTimerLifetimeDuration);
return ;
};
inline ScopeTimer::Duration ScopeTimer::getDurationFromStart() const {
using std::chrono::duration_cast;
const std::chrono::high_resolution_clock::time_point now = std::chrono::high_resolution_clock::now();
return (duration_cast<DurationType>(now - this->start).count());
};
inline void ScopeTimerStaticCore::addTimingToNamedScope(const ScopeTimer::ScopeSignature& scopeName, const ScopeTimer::Duration& duration) {
ScopesTiming& scopesTimingContainer = ScopeTimerStaticCore::getScopesTimingStaticInstance();
scopesTimingContainer[scopeName].push_back(duration);
return ;
};
inline void ScopeTimerStaticCore::dumpTimingToFile(const std::string& path) {
const ScopesTiming& scopesTimingContainer = ScopeTimerStaticCore::getScopesTimingStaticInstance();
std::ofstream dumpfile;
dumpfile.open(path, std::ios::out | std::ios::trunc);
for (ScopesTiming::const_iterator it_scopes = scopesTimingContainer.begin(); it_scopes != scopesTimingContainer.end(); ++it_scopes) {
const ScopeTimer::ScopeSignature& currentScope = it_scopes->first;
const TimingVector& timings = it_scopes->second;
for (TimingVector::const_iterator it_timings = timings.begin(); it_timings != timings.end(); ++it_timings)
dumpfile << currentScope << "," << *it_timings << std::endl;
}
dumpfile.close();
return ;
};
inline void ScopeTimerStaticCore::clearAllTiming() {
ScopesTiming& scopesTimingContainer = ScopeTimerStaticCore::getScopesTimingStaticInstance();
scopesTimingContainer.clear();
return ;
};
inline void ScopeTimerStaticCore::clearTimingForNamedScope(const ScopeTimer::ScopeSignature& scopeName) {
ScopesTiming& scopesTimingContainer = ScopeTimerStaticCore::getScopesTimingStaticInstance();
ScopesTiming::iterator it_scopes = scopesTimingContainer.find(scopeName);
if (it_scopes != scopesTimingContainer.end())
it_scopes->second.clear();
return ;
};
#endif /* SCOPE_TIMER */
And an dummy program that use it
main.cpp
#include "../include/scope_timer.hpp"
void functionA();
void functionB();
int main() {
for (size_t i = 0; i < 3; ++i) {
functionA();
functionB();
}
ScopeTimerStaticCore::dumpTimingToFile("/tmp/scope-timer_dump-dummy-test.csv");
return (0);
};
dumb_functions.cpp
#include <thread>
#include <chrono>
#include "../include/scope_timer.hpp"
void functionA() {
ScopeTimer scopeTimer("functionA");
std::this_thread::sleep_for (std::chrono::milliseconds(500));
return ;
};
void functionB() {
ScopeTimer scopeTimer("functionB");
std::this_thread::sleep_for (std::chrono::seconds(1));
return ;
};
if you want to retrieve the code quickly here is the repo link
c++ performance c++11 benchmarking
$endgroup$
add a comment |
$begingroup$
I wrote a header only class (actually there is a second one for data bookkeeping and dumping) to measure the execution time of a C++ scope without worrying too much about boilerplates. The idea being to be able to simply instantiate a class at the begining of the scope you want to measure, and do a single call at the end to dump the measure.
I rely on the fact that the class instantiation is at the beginning of the scope, and its destruction at the very end. So my main worry is about compile time optimizations that could change the order of execution and bias the measure. Also I'm not satisfy on how I retrieve the type for ScopeTimer::Duration
but I couldn't find the type properly :/
Here is the code:
scope_timer.hpp
#ifndef SCOPE_TIMER
#define SCOPE_TIMER
#include <chrono>
#include <string>
#include <vector>
#include <map>
#include <fstream>
class ScopeTimer {
public:
using ScopeSignature = std::string;
using DurationType = std::chrono::microseconds;
using Duration = decltype(std::chrono::duration_cast<DurationType>(std::chrono::high_resolution_clock::now() - std::chrono::high_resolution_clock::now()).count());
ScopeTimer(const ScopeSignature& scopeName);
~ScopeTimer();
Duration getDurationFromStart() const;
private:
ScopeTimer();
const ScopeSignature scopeName;
const std::chrono::high_resolution_clock::time_point start;
};
class ScopeTimerStaticCore {
public:
static void addTimingToNamedScope(const ScopeTimer::ScopeSignature& scopeName, const ScopeTimer::Duration& duration);
static void dumpTimingToFile(const std::string& path);
static void clearAllTiming();
static void clearTimingForNamedScope(const ScopeTimer::ScopeSignature& scopeName);
private:
using TimingVector = std::vector<ScopeTimer::Duration>;
using ScopesTiming = std::map<ScopeTimer::ScopeSignature, TimingVector>;
static ScopesTiming& getScopesTimingStaticInstance() {
static ScopesTiming scopesTimingContainer;
return (scopesTimingContainer);
};
};
/*******************************************************Implementations*******************************************************/
inline ScopeTimer::ScopeTimer(const ScopeSignature& scopeName) : scopeName(scopeName), start(std::chrono::high_resolution_clock::now()) {};
inline ScopeTimer::~ScopeTimer() {
const Duration scopeTimerLifetimeDuration = this->getDurationFromStart();
ScopeTimerStaticCore::addTimingToNamedScope(this->scopeName, scopeTimerLifetimeDuration);
return ;
};
inline ScopeTimer::Duration ScopeTimer::getDurationFromStart() const {
using std::chrono::duration_cast;
const std::chrono::high_resolution_clock::time_point now = std::chrono::high_resolution_clock::now();
return (duration_cast<DurationType>(now - this->start).count());
};
inline void ScopeTimerStaticCore::addTimingToNamedScope(const ScopeTimer::ScopeSignature& scopeName, const ScopeTimer::Duration& duration) {
ScopesTiming& scopesTimingContainer = ScopeTimerStaticCore::getScopesTimingStaticInstance();
scopesTimingContainer[scopeName].push_back(duration);
return ;
};
inline void ScopeTimerStaticCore::dumpTimingToFile(const std::string& path) {
const ScopesTiming& scopesTimingContainer = ScopeTimerStaticCore::getScopesTimingStaticInstance();
std::ofstream dumpfile;
dumpfile.open(path, std::ios::out | std::ios::trunc);
for (ScopesTiming::const_iterator it_scopes = scopesTimingContainer.begin(); it_scopes != scopesTimingContainer.end(); ++it_scopes) {
const ScopeTimer::ScopeSignature& currentScope = it_scopes->first;
const TimingVector& timings = it_scopes->second;
for (TimingVector::const_iterator it_timings = timings.begin(); it_timings != timings.end(); ++it_timings)
dumpfile << currentScope << "," << *it_timings << std::endl;
}
dumpfile.close();
return ;
};
inline void ScopeTimerStaticCore::clearAllTiming() {
ScopesTiming& scopesTimingContainer = ScopeTimerStaticCore::getScopesTimingStaticInstance();
scopesTimingContainer.clear();
return ;
};
inline void ScopeTimerStaticCore::clearTimingForNamedScope(const ScopeTimer::ScopeSignature& scopeName) {
ScopesTiming& scopesTimingContainer = ScopeTimerStaticCore::getScopesTimingStaticInstance();
ScopesTiming::iterator it_scopes = scopesTimingContainer.find(scopeName);
if (it_scopes != scopesTimingContainer.end())
it_scopes->second.clear();
return ;
};
#endif /* SCOPE_TIMER */
And an dummy program that use it
main.cpp
#include "../include/scope_timer.hpp"
void functionA();
void functionB();
int main() {
for (size_t i = 0; i < 3; ++i) {
functionA();
functionB();
}
ScopeTimerStaticCore::dumpTimingToFile("/tmp/scope-timer_dump-dummy-test.csv");
return (0);
};
dumb_functions.cpp
#include <thread>
#include <chrono>
#include "../include/scope_timer.hpp"
void functionA() {
ScopeTimer scopeTimer("functionA");
std::this_thread::sleep_for (std::chrono::milliseconds(500));
return ;
};
void functionB() {
ScopeTimer scopeTimer("functionB");
std::this_thread::sleep_for (std::chrono::seconds(1));
return ;
};
if you want to retrieve the code quickly here is the repo link
c++ performance c++11 benchmarking
$endgroup$
add a comment |
$begingroup$
I wrote a header only class (actually there is a second one for data bookkeeping and dumping) to measure the execution time of a C++ scope without worrying too much about boilerplates. The idea being to be able to simply instantiate a class at the begining of the scope you want to measure, and do a single call at the end to dump the measure.
I rely on the fact that the class instantiation is at the beginning of the scope, and its destruction at the very end. So my main worry is about compile time optimizations that could change the order of execution and bias the measure. Also I'm not satisfy on how I retrieve the type for ScopeTimer::Duration
but I couldn't find the type properly :/
Here is the code:
scope_timer.hpp
#ifndef SCOPE_TIMER
#define SCOPE_TIMER
#include <chrono>
#include <string>
#include <vector>
#include <map>
#include <fstream>
class ScopeTimer {
public:
using ScopeSignature = std::string;
using DurationType = std::chrono::microseconds;
using Duration = decltype(std::chrono::duration_cast<DurationType>(std::chrono::high_resolution_clock::now() - std::chrono::high_resolution_clock::now()).count());
ScopeTimer(const ScopeSignature& scopeName);
~ScopeTimer();
Duration getDurationFromStart() const;
private:
ScopeTimer();
const ScopeSignature scopeName;
const std::chrono::high_resolution_clock::time_point start;
};
class ScopeTimerStaticCore {
public:
static void addTimingToNamedScope(const ScopeTimer::ScopeSignature& scopeName, const ScopeTimer::Duration& duration);
static void dumpTimingToFile(const std::string& path);
static void clearAllTiming();
static void clearTimingForNamedScope(const ScopeTimer::ScopeSignature& scopeName);
private:
using TimingVector = std::vector<ScopeTimer::Duration>;
using ScopesTiming = std::map<ScopeTimer::ScopeSignature, TimingVector>;
static ScopesTiming& getScopesTimingStaticInstance() {
static ScopesTiming scopesTimingContainer;
return (scopesTimingContainer);
};
};
/*******************************************************Implementations*******************************************************/
inline ScopeTimer::ScopeTimer(const ScopeSignature& scopeName) : scopeName(scopeName), start(std::chrono::high_resolution_clock::now()) {};
inline ScopeTimer::~ScopeTimer() {
const Duration scopeTimerLifetimeDuration = this->getDurationFromStart();
ScopeTimerStaticCore::addTimingToNamedScope(this->scopeName, scopeTimerLifetimeDuration);
return ;
};
inline ScopeTimer::Duration ScopeTimer::getDurationFromStart() const {
using std::chrono::duration_cast;
const std::chrono::high_resolution_clock::time_point now = std::chrono::high_resolution_clock::now();
return (duration_cast<DurationType>(now - this->start).count());
};
inline void ScopeTimerStaticCore::addTimingToNamedScope(const ScopeTimer::ScopeSignature& scopeName, const ScopeTimer::Duration& duration) {
ScopesTiming& scopesTimingContainer = ScopeTimerStaticCore::getScopesTimingStaticInstance();
scopesTimingContainer[scopeName].push_back(duration);
return ;
};
inline void ScopeTimerStaticCore::dumpTimingToFile(const std::string& path) {
const ScopesTiming& scopesTimingContainer = ScopeTimerStaticCore::getScopesTimingStaticInstance();
std::ofstream dumpfile;
dumpfile.open(path, std::ios::out | std::ios::trunc);
for (ScopesTiming::const_iterator it_scopes = scopesTimingContainer.begin(); it_scopes != scopesTimingContainer.end(); ++it_scopes) {
const ScopeTimer::ScopeSignature& currentScope = it_scopes->first;
const TimingVector& timings = it_scopes->second;
for (TimingVector::const_iterator it_timings = timings.begin(); it_timings != timings.end(); ++it_timings)
dumpfile << currentScope << "," << *it_timings << std::endl;
}
dumpfile.close();
return ;
};
inline void ScopeTimerStaticCore::clearAllTiming() {
ScopesTiming& scopesTimingContainer = ScopeTimerStaticCore::getScopesTimingStaticInstance();
scopesTimingContainer.clear();
return ;
};
inline void ScopeTimerStaticCore::clearTimingForNamedScope(const ScopeTimer::ScopeSignature& scopeName) {
ScopesTiming& scopesTimingContainer = ScopeTimerStaticCore::getScopesTimingStaticInstance();
ScopesTiming::iterator it_scopes = scopesTimingContainer.find(scopeName);
if (it_scopes != scopesTimingContainer.end())
it_scopes->second.clear();
return ;
};
#endif /* SCOPE_TIMER */
And an dummy program that use it
main.cpp
#include "../include/scope_timer.hpp"
void functionA();
void functionB();
int main() {
for (size_t i = 0; i < 3; ++i) {
functionA();
functionB();
}
ScopeTimerStaticCore::dumpTimingToFile("/tmp/scope-timer_dump-dummy-test.csv");
return (0);
};
dumb_functions.cpp
#include <thread>
#include <chrono>
#include "../include/scope_timer.hpp"
void functionA() {
ScopeTimer scopeTimer("functionA");
std::this_thread::sleep_for (std::chrono::milliseconds(500));
return ;
};
void functionB() {
ScopeTimer scopeTimer("functionB");
std::this_thread::sleep_for (std::chrono::seconds(1));
return ;
};
if you want to retrieve the code quickly here is the repo link
c++ performance c++11 benchmarking
$endgroup$
I wrote a header only class (actually there is a second one for data bookkeeping and dumping) to measure the execution time of a C++ scope without worrying too much about boilerplates. The idea being to be able to simply instantiate a class at the begining of the scope you want to measure, and do a single call at the end to dump the measure.
I rely on the fact that the class instantiation is at the beginning of the scope, and its destruction at the very end. So my main worry is about compile time optimizations that could change the order of execution and bias the measure. Also I'm not satisfy on how I retrieve the type for ScopeTimer::Duration
but I couldn't find the type properly :/
Here is the code:
scope_timer.hpp
#ifndef SCOPE_TIMER
#define SCOPE_TIMER
#include <chrono>
#include <string>
#include <vector>
#include <map>
#include <fstream>
class ScopeTimer {
public:
using ScopeSignature = std::string;
using DurationType = std::chrono::microseconds;
using Duration = decltype(std::chrono::duration_cast<DurationType>(std::chrono::high_resolution_clock::now() - std::chrono::high_resolution_clock::now()).count());
ScopeTimer(const ScopeSignature& scopeName);
~ScopeTimer();
Duration getDurationFromStart() const;
private:
ScopeTimer();
const ScopeSignature scopeName;
const std::chrono::high_resolution_clock::time_point start;
};
class ScopeTimerStaticCore {
public:
static void addTimingToNamedScope(const ScopeTimer::ScopeSignature& scopeName, const ScopeTimer::Duration& duration);
static void dumpTimingToFile(const std::string& path);
static void clearAllTiming();
static void clearTimingForNamedScope(const ScopeTimer::ScopeSignature& scopeName);
private:
using TimingVector = std::vector<ScopeTimer::Duration>;
using ScopesTiming = std::map<ScopeTimer::ScopeSignature, TimingVector>;
static ScopesTiming& getScopesTimingStaticInstance() {
static ScopesTiming scopesTimingContainer;
return (scopesTimingContainer);
};
};
/*******************************************************Implementations*******************************************************/
inline ScopeTimer::ScopeTimer(const ScopeSignature& scopeName) : scopeName(scopeName), start(std::chrono::high_resolution_clock::now()) {};
inline ScopeTimer::~ScopeTimer() {
const Duration scopeTimerLifetimeDuration = this->getDurationFromStart();
ScopeTimerStaticCore::addTimingToNamedScope(this->scopeName, scopeTimerLifetimeDuration);
return ;
};
inline ScopeTimer::Duration ScopeTimer::getDurationFromStart() const {
using std::chrono::duration_cast;
const std::chrono::high_resolution_clock::time_point now = std::chrono::high_resolution_clock::now();
return (duration_cast<DurationType>(now - this->start).count());
};
inline void ScopeTimerStaticCore::addTimingToNamedScope(const ScopeTimer::ScopeSignature& scopeName, const ScopeTimer::Duration& duration) {
ScopesTiming& scopesTimingContainer = ScopeTimerStaticCore::getScopesTimingStaticInstance();
scopesTimingContainer[scopeName].push_back(duration);
return ;
};
inline void ScopeTimerStaticCore::dumpTimingToFile(const std::string& path) {
const ScopesTiming& scopesTimingContainer = ScopeTimerStaticCore::getScopesTimingStaticInstance();
std::ofstream dumpfile;
dumpfile.open(path, std::ios::out | std::ios::trunc);
for (ScopesTiming::const_iterator it_scopes = scopesTimingContainer.begin(); it_scopes != scopesTimingContainer.end(); ++it_scopes) {
const ScopeTimer::ScopeSignature& currentScope = it_scopes->first;
const TimingVector& timings = it_scopes->second;
for (TimingVector::const_iterator it_timings = timings.begin(); it_timings != timings.end(); ++it_timings)
dumpfile << currentScope << "," << *it_timings << std::endl;
}
dumpfile.close();
return ;
};
inline void ScopeTimerStaticCore::clearAllTiming() {
ScopesTiming& scopesTimingContainer = ScopeTimerStaticCore::getScopesTimingStaticInstance();
scopesTimingContainer.clear();
return ;
};
inline void ScopeTimerStaticCore::clearTimingForNamedScope(const ScopeTimer::ScopeSignature& scopeName) {
ScopesTiming& scopesTimingContainer = ScopeTimerStaticCore::getScopesTimingStaticInstance();
ScopesTiming::iterator it_scopes = scopesTimingContainer.find(scopeName);
if (it_scopes != scopesTimingContainer.end())
it_scopes->second.clear();
return ;
};
#endif /* SCOPE_TIMER */
And an dummy program that use it
main.cpp
#include "../include/scope_timer.hpp"
void functionA();
void functionB();
int main() {
for (size_t i = 0; i < 3; ++i) {
functionA();
functionB();
}
ScopeTimerStaticCore::dumpTimingToFile("/tmp/scope-timer_dump-dummy-test.csv");
return (0);
};
dumb_functions.cpp
#include <thread>
#include <chrono>
#include "../include/scope_timer.hpp"
void functionA() {
ScopeTimer scopeTimer("functionA");
std::this_thread::sleep_for (std::chrono::milliseconds(500));
return ;
};
void functionB() {
ScopeTimer scopeTimer("functionB");
std::this_thread::sleep_for (std::chrono::seconds(1));
return ;
};
if you want to retrieve the code quickly here is the repo link
c++ performance c++11 benchmarking
c++ performance c++11 benchmarking
edited Jan 20 at 15:46
shorty_ponton
asked Jan 20 at 14:32
shorty_pontonshorty_ponton
234
234
add a comment |
add a comment |
1 Answer
1
active
oldest
votes
$begingroup$
using Duration = decltype(std::chrono::duration_cast<DurationType>(std::chrono::high_resolution_clock::now() - std::chrono::high_resolution_clock::now()).count());
I'm confused by this line. decltype(std::chrono::duration_cast<DurationType>(expr))
is invariably DurationType
, isn't it? That's why it's a "cast"? So this simplifies down to decltype(std::declval<DurationType&>().count())
, which I'm pretty sure can be spelled as DurationType::rep
unless you're really eager to support non-standard duration types that might not have a rep
member. So:
using Duration = typename DurationType::rep;
And now it appears that maybe Duration
is the wrong name for this typedef, eh?
(EDIT: Oops, the keyword typename
is not needed here because DurationType
is not dependent. Just using Duration = DurationType::rep;
should be sufficient.)
static ScopesTiming& getScopesTimingStaticInstance() {
static ScopesTiming scopesTimingContainer;
return (scopesTimingContainer);
};
Minor nits on whitespace and naming and parentheses and trailing semicolons:
static ScopesTiming& getScopesTimingStaticInstance() {
static ScopesTiming instance;
return instance;
}
The defining quality of instance
is that it's a static instance of ScopesTiming
. If you want to convey the additional information that ScopesTiming
is actually a container type, then that information belongs in the name of the type. Personally I'd call it something like TimingVectorMap
, because it's a map of TimingVector
s.
Since the static map is not guarded by any mutex, your function addTimingToNamedScope
(which mutates the map) is not safe to call from multiple threads concurrently. This could be a problem for real-world use.
ScopeTimer
has two const
-qualified fields. This doesn't do anything except pessimize its implicitly generated move-constructor into a copy-constructor. I recommend removing the const
.
ScopeTimer
also has an implicit conversion from ScopeSignature
a.k.a. std::string
, so that for example
void f(ScopeTimer timer);
std::string hello = "hello world";
f(hello); // compiles without any diagnostic
I very strongly suggest that you never enable any implicit conversion unless you have a very good reason for it. This means putting explicit
on every constructor and conversion operator.
explicit ScopeTimer(const ScopeSignature& scopeName);
dumpfile.open(path, std::ios::out | std::ios::trunc);
Should you check to see if the open succeeded?
inline void ScopeTimerStaticCore::clearTimingForNamedScope(const ScopeTimer::ScopeSignature& scopeName) {
ScopesTiming& scopesTimingContainer = ScopeTimerStaticCore::getScopesTimingStaticInstance();
ScopesTiming::iterator it_scopes = scopesTimingContainer.find(scopeName);
if (it_scopes != scopesTimingContainer.end())
it_scopes->second.clear();
return ;
};
This would be a good place to use C++11 auto
:
inline void ScopeTimerStaticCore::clearTimingForNamedScope(const ScopeTimer::ScopeSignature& scopeName) {
ScopesTiming& instance = getScopesTimingStaticInstance();
auto it = instance.find(scopeName);
if (it != instance.end()) {
it->second.clear();
}
}
Or, if you don't mind removing the element from the map completely, you could just use erase
:
inline void ScopeTimerStaticCore::clearTimingForNamedScope(const ScopeTimer::ScopeSignature& scopeName) {
ScopesTiming& instance = getScopesTimingStaticInstance();
instance.erase(scopeName);
}
I also notice that these functions would get a lot shorter and simpler to read if you put their definitions in-line into the class body of ScopeTimerStaticCore
. In this case you could omit the keyword inline
and the qualification of the parameter type:
void clearTimingForNamedScope(const ScopeSignature& scopeName) {
ScopesTiming& instance = getScopesTimingStaticInstance();
instance.erase(scopeName);
}
(Assuming that ScopeTimerStaticCore
contains a member typedef using ScopeSignature = ScopeTimer::ScopeSignature;
, I guess. It probably should — or vice versa.)
$endgroup$
$begingroup$
I cannot compile the example code when I try to useDurationType::rep
:/ About the codestyle I'll add some of your suggestions thanks Totally agree for the "open check" "full erase" and "explicit declaration" (I didn't know about that last one, so big win for me thanks)
$endgroup$
– shorty_ponton
Jan 22 at 9:37
$begingroup$
Works fine for me, but I admit that thetypename
is not needed, becauseDurationType
is not template-dependent. If your compiler is complaining about thetypename
keyword, you can safely remove it. Otherwise, it should work fine.
$endgroup$
– Quuxplusone
Jan 22 at 17:49
add a comment |
Your Answer
StackExchange.ifUsing("editor", function () {
return StackExchange.using("mathjaxEditing", function () {
StackExchange.MarkdownEditor.creationCallbacks.add(function (editor, postfix) {
StackExchange.mathjaxEditing.prepareWmdForMathJax(editor, postfix, [["\$", "\$"]]);
});
});
}, "mathjax-editing");
StackExchange.ifUsing("editor", function () {
StackExchange.using("externalEditor", function () {
StackExchange.using("snippets", function () {
StackExchange.snippets.init();
});
});
}, "code-snippets");
StackExchange.ready(function() {
var channelOptions = {
tags: "".split(" "),
id: "196"
};
initTagRenderer("".split(" "), "".split(" "), channelOptions);
StackExchange.using("externalEditor", function() {
// Have to fire editor after snippets, if snippets enabled
if (StackExchange.settings.snippets.snippetsEnabled) {
StackExchange.using("snippets", function() {
createEditor();
});
}
else {
createEditor();
}
});
function createEditor() {
StackExchange.prepareEditor({
heartbeatType: 'answer',
autoActivateHeartbeat: false,
convertImagesToLinks: false,
noModals: true,
showLowRepImageUploadWarning: true,
reputationToPostImages: null,
bindNavPrevention: true,
postfix: "",
imageUploader: {
brandingHtml: "Powered by u003ca class="icon-imgur-white" href="https://imgur.com/"u003eu003c/au003e",
contentPolicyHtml: "User contributions licensed under u003ca href="https://creativecommons.org/licenses/by-sa/3.0/"u003ecc by-sa 3.0 with attribution requiredu003c/au003e u003ca href="https://stackoverflow.com/legal/content-policy"u003e(content policy)u003c/au003e",
allowUrls: true
},
onDemand: true,
discardSelector: ".discard-answer"
,immediatelyShowMarkdownHelp:true
});
}
});
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
StackExchange.ready(
function () {
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f211857%2fa-header-only-class-for-c-time-measurement%23new-answer', 'question_page');
}
);
Post as a guest
Required, but never shown
1 Answer
1
active
oldest
votes
1 Answer
1
active
oldest
votes
active
oldest
votes
active
oldest
votes
$begingroup$
using Duration = decltype(std::chrono::duration_cast<DurationType>(std::chrono::high_resolution_clock::now() - std::chrono::high_resolution_clock::now()).count());
I'm confused by this line. decltype(std::chrono::duration_cast<DurationType>(expr))
is invariably DurationType
, isn't it? That's why it's a "cast"? So this simplifies down to decltype(std::declval<DurationType&>().count())
, which I'm pretty sure can be spelled as DurationType::rep
unless you're really eager to support non-standard duration types that might not have a rep
member. So:
using Duration = typename DurationType::rep;
And now it appears that maybe Duration
is the wrong name for this typedef, eh?
(EDIT: Oops, the keyword typename
is not needed here because DurationType
is not dependent. Just using Duration = DurationType::rep;
should be sufficient.)
static ScopesTiming& getScopesTimingStaticInstance() {
static ScopesTiming scopesTimingContainer;
return (scopesTimingContainer);
};
Minor nits on whitespace and naming and parentheses and trailing semicolons:
static ScopesTiming& getScopesTimingStaticInstance() {
static ScopesTiming instance;
return instance;
}
The defining quality of instance
is that it's a static instance of ScopesTiming
. If you want to convey the additional information that ScopesTiming
is actually a container type, then that information belongs in the name of the type. Personally I'd call it something like TimingVectorMap
, because it's a map of TimingVector
s.
Since the static map is not guarded by any mutex, your function addTimingToNamedScope
(which mutates the map) is not safe to call from multiple threads concurrently. This could be a problem for real-world use.
ScopeTimer
has two const
-qualified fields. This doesn't do anything except pessimize its implicitly generated move-constructor into a copy-constructor. I recommend removing the const
.
ScopeTimer
also has an implicit conversion from ScopeSignature
a.k.a. std::string
, so that for example
void f(ScopeTimer timer);
std::string hello = "hello world";
f(hello); // compiles without any diagnostic
I very strongly suggest that you never enable any implicit conversion unless you have a very good reason for it. This means putting explicit
on every constructor and conversion operator.
explicit ScopeTimer(const ScopeSignature& scopeName);
dumpfile.open(path, std::ios::out | std::ios::trunc);
Should you check to see if the open succeeded?
inline void ScopeTimerStaticCore::clearTimingForNamedScope(const ScopeTimer::ScopeSignature& scopeName) {
ScopesTiming& scopesTimingContainer = ScopeTimerStaticCore::getScopesTimingStaticInstance();
ScopesTiming::iterator it_scopes = scopesTimingContainer.find(scopeName);
if (it_scopes != scopesTimingContainer.end())
it_scopes->second.clear();
return ;
};
This would be a good place to use C++11 auto
:
inline void ScopeTimerStaticCore::clearTimingForNamedScope(const ScopeTimer::ScopeSignature& scopeName) {
ScopesTiming& instance = getScopesTimingStaticInstance();
auto it = instance.find(scopeName);
if (it != instance.end()) {
it->second.clear();
}
}
Or, if you don't mind removing the element from the map completely, you could just use erase
:
inline void ScopeTimerStaticCore::clearTimingForNamedScope(const ScopeTimer::ScopeSignature& scopeName) {
ScopesTiming& instance = getScopesTimingStaticInstance();
instance.erase(scopeName);
}
I also notice that these functions would get a lot shorter and simpler to read if you put their definitions in-line into the class body of ScopeTimerStaticCore
. In this case you could omit the keyword inline
and the qualification of the parameter type:
void clearTimingForNamedScope(const ScopeSignature& scopeName) {
ScopesTiming& instance = getScopesTimingStaticInstance();
instance.erase(scopeName);
}
(Assuming that ScopeTimerStaticCore
contains a member typedef using ScopeSignature = ScopeTimer::ScopeSignature;
, I guess. It probably should — or vice versa.)
$endgroup$
$begingroup$
I cannot compile the example code when I try to useDurationType::rep
:/ About the codestyle I'll add some of your suggestions thanks Totally agree for the "open check" "full erase" and "explicit declaration" (I didn't know about that last one, so big win for me thanks)
$endgroup$
– shorty_ponton
Jan 22 at 9:37
$begingroup$
Works fine for me, but I admit that thetypename
is not needed, becauseDurationType
is not template-dependent. If your compiler is complaining about thetypename
keyword, you can safely remove it. Otherwise, it should work fine.
$endgroup$
– Quuxplusone
Jan 22 at 17:49
add a comment |
$begingroup$
using Duration = decltype(std::chrono::duration_cast<DurationType>(std::chrono::high_resolution_clock::now() - std::chrono::high_resolution_clock::now()).count());
I'm confused by this line. decltype(std::chrono::duration_cast<DurationType>(expr))
is invariably DurationType
, isn't it? That's why it's a "cast"? So this simplifies down to decltype(std::declval<DurationType&>().count())
, which I'm pretty sure can be spelled as DurationType::rep
unless you're really eager to support non-standard duration types that might not have a rep
member. So:
using Duration = typename DurationType::rep;
And now it appears that maybe Duration
is the wrong name for this typedef, eh?
(EDIT: Oops, the keyword typename
is not needed here because DurationType
is not dependent. Just using Duration = DurationType::rep;
should be sufficient.)
static ScopesTiming& getScopesTimingStaticInstance() {
static ScopesTiming scopesTimingContainer;
return (scopesTimingContainer);
};
Minor nits on whitespace and naming and parentheses and trailing semicolons:
static ScopesTiming& getScopesTimingStaticInstance() {
static ScopesTiming instance;
return instance;
}
The defining quality of instance
is that it's a static instance of ScopesTiming
. If you want to convey the additional information that ScopesTiming
is actually a container type, then that information belongs in the name of the type. Personally I'd call it something like TimingVectorMap
, because it's a map of TimingVector
s.
Since the static map is not guarded by any mutex, your function addTimingToNamedScope
(which mutates the map) is not safe to call from multiple threads concurrently. This could be a problem for real-world use.
ScopeTimer
has two const
-qualified fields. This doesn't do anything except pessimize its implicitly generated move-constructor into a copy-constructor. I recommend removing the const
.
ScopeTimer
also has an implicit conversion from ScopeSignature
a.k.a. std::string
, so that for example
void f(ScopeTimer timer);
std::string hello = "hello world";
f(hello); // compiles without any diagnostic
I very strongly suggest that you never enable any implicit conversion unless you have a very good reason for it. This means putting explicit
on every constructor and conversion operator.
explicit ScopeTimer(const ScopeSignature& scopeName);
dumpfile.open(path, std::ios::out | std::ios::trunc);
Should you check to see if the open succeeded?
inline void ScopeTimerStaticCore::clearTimingForNamedScope(const ScopeTimer::ScopeSignature& scopeName) {
ScopesTiming& scopesTimingContainer = ScopeTimerStaticCore::getScopesTimingStaticInstance();
ScopesTiming::iterator it_scopes = scopesTimingContainer.find(scopeName);
if (it_scopes != scopesTimingContainer.end())
it_scopes->second.clear();
return ;
};
This would be a good place to use C++11 auto
:
inline void ScopeTimerStaticCore::clearTimingForNamedScope(const ScopeTimer::ScopeSignature& scopeName) {
ScopesTiming& instance = getScopesTimingStaticInstance();
auto it = instance.find(scopeName);
if (it != instance.end()) {
it->second.clear();
}
}
Or, if you don't mind removing the element from the map completely, you could just use erase
:
inline void ScopeTimerStaticCore::clearTimingForNamedScope(const ScopeTimer::ScopeSignature& scopeName) {
ScopesTiming& instance = getScopesTimingStaticInstance();
instance.erase(scopeName);
}
I also notice that these functions would get a lot shorter and simpler to read if you put their definitions in-line into the class body of ScopeTimerStaticCore
. In this case you could omit the keyword inline
and the qualification of the parameter type:
void clearTimingForNamedScope(const ScopeSignature& scopeName) {
ScopesTiming& instance = getScopesTimingStaticInstance();
instance.erase(scopeName);
}
(Assuming that ScopeTimerStaticCore
contains a member typedef using ScopeSignature = ScopeTimer::ScopeSignature;
, I guess. It probably should — or vice versa.)
$endgroup$
$begingroup$
I cannot compile the example code when I try to useDurationType::rep
:/ About the codestyle I'll add some of your suggestions thanks Totally agree for the "open check" "full erase" and "explicit declaration" (I didn't know about that last one, so big win for me thanks)
$endgroup$
– shorty_ponton
Jan 22 at 9:37
$begingroup$
Works fine for me, but I admit that thetypename
is not needed, becauseDurationType
is not template-dependent. If your compiler is complaining about thetypename
keyword, you can safely remove it. Otherwise, it should work fine.
$endgroup$
– Quuxplusone
Jan 22 at 17:49
add a comment |
$begingroup$
using Duration = decltype(std::chrono::duration_cast<DurationType>(std::chrono::high_resolution_clock::now() - std::chrono::high_resolution_clock::now()).count());
I'm confused by this line. decltype(std::chrono::duration_cast<DurationType>(expr))
is invariably DurationType
, isn't it? That's why it's a "cast"? So this simplifies down to decltype(std::declval<DurationType&>().count())
, which I'm pretty sure can be spelled as DurationType::rep
unless you're really eager to support non-standard duration types that might not have a rep
member. So:
using Duration = typename DurationType::rep;
And now it appears that maybe Duration
is the wrong name for this typedef, eh?
(EDIT: Oops, the keyword typename
is not needed here because DurationType
is not dependent. Just using Duration = DurationType::rep;
should be sufficient.)
static ScopesTiming& getScopesTimingStaticInstance() {
static ScopesTiming scopesTimingContainer;
return (scopesTimingContainer);
};
Minor nits on whitespace and naming and parentheses and trailing semicolons:
static ScopesTiming& getScopesTimingStaticInstance() {
static ScopesTiming instance;
return instance;
}
The defining quality of instance
is that it's a static instance of ScopesTiming
. If you want to convey the additional information that ScopesTiming
is actually a container type, then that information belongs in the name of the type. Personally I'd call it something like TimingVectorMap
, because it's a map of TimingVector
s.
Since the static map is not guarded by any mutex, your function addTimingToNamedScope
(which mutates the map) is not safe to call from multiple threads concurrently. This could be a problem for real-world use.
ScopeTimer
has two const
-qualified fields. This doesn't do anything except pessimize its implicitly generated move-constructor into a copy-constructor. I recommend removing the const
.
ScopeTimer
also has an implicit conversion from ScopeSignature
a.k.a. std::string
, so that for example
void f(ScopeTimer timer);
std::string hello = "hello world";
f(hello); // compiles without any diagnostic
I very strongly suggest that you never enable any implicit conversion unless you have a very good reason for it. This means putting explicit
on every constructor and conversion operator.
explicit ScopeTimer(const ScopeSignature& scopeName);
dumpfile.open(path, std::ios::out | std::ios::trunc);
Should you check to see if the open succeeded?
inline void ScopeTimerStaticCore::clearTimingForNamedScope(const ScopeTimer::ScopeSignature& scopeName) {
ScopesTiming& scopesTimingContainer = ScopeTimerStaticCore::getScopesTimingStaticInstance();
ScopesTiming::iterator it_scopes = scopesTimingContainer.find(scopeName);
if (it_scopes != scopesTimingContainer.end())
it_scopes->second.clear();
return ;
};
This would be a good place to use C++11 auto
:
inline void ScopeTimerStaticCore::clearTimingForNamedScope(const ScopeTimer::ScopeSignature& scopeName) {
ScopesTiming& instance = getScopesTimingStaticInstance();
auto it = instance.find(scopeName);
if (it != instance.end()) {
it->second.clear();
}
}
Or, if you don't mind removing the element from the map completely, you could just use erase
:
inline void ScopeTimerStaticCore::clearTimingForNamedScope(const ScopeTimer::ScopeSignature& scopeName) {
ScopesTiming& instance = getScopesTimingStaticInstance();
instance.erase(scopeName);
}
I also notice that these functions would get a lot shorter and simpler to read if you put their definitions in-line into the class body of ScopeTimerStaticCore
. In this case you could omit the keyword inline
and the qualification of the parameter type:
void clearTimingForNamedScope(const ScopeSignature& scopeName) {
ScopesTiming& instance = getScopesTimingStaticInstance();
instance.erase(scopeName);
}
(Assuming that ScopeTimerStaticCore
contains a member typedef using ScopeSignature = ScopeTimer::ScopeSignature;
, I guess. It probably should — or vice versa.)
$endgroup$
using Duration = decltype(std::chrono::duration_cast<DurationType>(std::chrono::high_resolution_clock::now() - std::chrono::high_resolution_clock::now()).count());
I'm confused by this line. decltype(std::chrono::duration_cast<DurationType>(expr))
is invariably DurationType
, isn't it? That's why it's a "cast"? So this simplifies down to decltype(std::declval<DurationType&>().count())
, which I'm pretty sure can be spelled as DurationType::rep
unless you're really eager to support non-standard duration types that might not have a rep
member. So:
using Duration = typename DurationType::rep;
And now it appears that maybe Duration
is the wrong name for this typedef, eh?
(EDIT: Oops, the keyword typename
is not needed here because DurationType
is not dependent. Just using Duration = DurationType::rep;
should be sufficient.)
static ScopesTiming& getScopesTimingStaticInstance() {
static ScopesTiming scopesTimingContainer;
return (scopesTimingContainer);
};
Minor nits on whitespace and naming and parentheses and trailing semicolons:
static ScopesTiming& getScopesTimingStaticInstance() {
static ScopesTiming instance;
return instance;
}
The defining quality of instance
is that it's a static instance of ScopesTiming
. If you want to convey the additional information that ScopesTiming
is actually a container type, then that information belongs in the name of the type. Personally I'd call it something like TimingVectorMap
, because it's a map of TimingVector
s.
Since the static map is not guarded by any mutex, your function addTimingToNamedScope
(which mutates the map) is not safe to call from multiple threads concurrently. This could be a problem for real-world use.
ScopeTimer
has two const
-qualified fields. This doesn't do anything except pessimize its implicitly generated move-constructor into a copy-constructor. I recommend removing the const
.
ScopeTimer
also has an implicit conversion from ScopeSignature
a.k.a. std::string
, so that for example
void f(ScopeTimer timer);
std::string hello = "hello world";
f(hello); // compiles without any diagnostic
I very strongly suggest that you never enable any implicit conversion unless you have a very good reason for it. This means putting explicit
on every constructor and conversion operator.
explicit ScopeTimer(const ScopeSignature& scopeName);
dumpfile.open(path, std::ios::out | std::ios::trunc);
Should you check to see if the open succeeded?
inline void ScopeTimerStaticCore::clearTimingForNamedScope(const ScopeTimer::ScopeSignature& scopeName) {
ScopesTiming& scopesTimingContainer = ScopeTimerStaticCore::getScopesTimingStaticInstance();
ScopesTiming::iterator it_scopes = scopesTimingContainer.find(scopeName);
if (it_scopes != scopesTimingContainer.end())
it_scopes->second.clear();
return ;
};
This would be a good place to use C++11 auto
:
inline void ScopeTimerStaticCore::clearTimingForNamedScope(const ScopeTimer::ScopeSignature& scopeName) {
ScopesTiming& instance = getScopesTimingStaticInstance();
auto it = instance.find(scopeName);
if (it != instance.end()) {
it->second.clear();
}
}
Or, if you don't mind removing the element from the map completely, you could just use erase
:
inline void ScopeTimerStaticCore::clearTimingForNamedScope(const ScopeTimer::ScopeSignature& scopeName) {
ScopesTiming& instance = getScopesTimingStaticInstance();
instance.erase(scopeName);
}
I also notice that these functions would get a lot shorter and simpler to read if you put their definitions in-line into the class body of ScopeTimerStaticCore
. In this case you could omit the keyword inline
and the qualification of the parameter type:
void clearTimingForNamedScope(const ScopeSignature& scopeName) {
ScopesTiming& instance = getScopesTimingStaticInstance();
instance.erase(scopeName);
}
(Assuming that ScopeTimerStaticCore
contains a member typedef using ScopeSignature = ScopeTimer::ScopeSignature;
, I guess. It probably should — or vice versa.)
edited Jan 22 at 17:50
answered Jan 20 at 16:09
QuuxplusoneQuuxplusone
12.3k12061
12.3k12061
$begingroup$
I cannot compile the example code when I try to useDurationType::rep
:/ About the codestyle I'll add some of your suggestions thanks Totally agree for the "open check" "full erase" and "explicit declaration" (I didn't know about that last one, so big win for me thanks)
$endgroup$
– shorty_ponton
Jan 22 at 9:37
$begingroup$
Works fine for me, but I admit that thetypename
is not needed, becauseDurationType
is not template-dependent. If your compiler is complaining about thetypename
keyword, you can safely remove it. Otherwise, it should work fine.
$endgroup$
– Quuxplusone
Jan 22 at 17:49
add a comment |
$begingroup$
I cannot compile the example code when I try to useDurationType::rep
:/ About the codestyle I'll add some of your suggestions thanks Totally agree for the "open check" "full erase" and "explicit declaration" (I didn't know about that last one, so big win for me thanks)
$endgroup$
– shorty_ponton
Jan 22 at 9:37
$begingroup$
Works fine for me, but I admit that thetypename
is not needed, becauseDurationType
is not template-dependent. If your compiler is complaining about thetypename
keyword, you can safely remove it. Otherwise, it should work fine.
$endgroup$
– Quuxplusone
Jan 22 at 17:49
$begingroup$
I cannot compile the example code when I try to use
DurationType::rep
:/ About the codestyle I'll add some of your suggestions thanks Totally agree for the "open check" "full erase" and "explicit declaration" (I didn't know about that last one, so big win for me thanks)$endgroup$
– shorty_ponton
Jan 22 at 9:37
$begingroup$
I cannot compile the example code when I try to use
DurationType::rep
:/ About the codestyle I'll add some of your suggestions thanks Totally agree for the "open check" "full erase" and "explicit declaration" (I didn't know about that last one, so big win for me thanks)$endgroup$
– shorty_ponton
Jan 22 at 9:37
$begingroup$
Works fine for me, but I admit that the
typename
is not needed, because DurationType
is not template-dependent. If your compiler is complaining about the typename
keyword, you can safely remove it. Otherwise, it should work fine.$endgroup$
– Quuxplusone
Jan 22 at 17:49
$begingroup$
Works fine for me, but I admit that the
typename
is not needed, because DurationType
is not template-dependent. If your compiler is complaining about the typename
keyword, you can safely remove it. Otherwise, it should work fine.$endgroup$
– Quuxplusone
Jan 22 at 17:49
add a comment |
Thanks for contributing an answer to Code Review Stack Exchange!
- Please be sure to answer the question. Provide details and share your research!
But avoid …
- Asking for help, clarification, or responding to other answers.
- Making statements based on opinion; back them up with references or personal experience.
Use MathJax to format equations. MathJax reference.
To learn more, see our tips on writing great answers.
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
StackExchange.ready(
function () {
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f211857%2fa-header-only-class-for-c-time-measurement%23new-answer', 'question_page');
}
);
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown