Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Timer utilities #1526

Merged
merged 2 commits into from Jan 7, 2019

Conversation

Projects
4 participants
@cryptocode
Copy link
Collaborator

commented Jan 1, 2019

Documentation started here: https://github.com/cryptocode/notes/wiki/Timer

I've introduced it into a couple of node.cpp functions; seems to simplify the timing code a fair amount.

@cryptocode cryptocode force-pushed the cryptocode:feature/timer-utility branch from cc21279 to 587fe2c Jan 1, 2019

@cryptocode cryptocode force-pushed the cryptocode:feature/timer-utility branch from 587fe2c to 3df7c88 Jan 1, 2019

unsigned measurements{ 0 };

template <typename U = UNIT>
std::string typed_unit (typename std::enable_if<std::is_same<U, std::chrono::nanoseconds>::value, std::chrono::nanoseconds>::type dummy) const

This comment has been minimized.

Copy link
@wezrule

wezrule Jan 1, 2019

Collaborator

I quite like this approach. There is a helper alias std::enable_if_t in C++14 to shorten this syntax. You can also avoid the "dummy" variable by using it in the return type, i.e:
std::enable_if_t<std::is_same<U, std::chrono::microseconds>::value, std::string> typed_unit () { return "microseconds";}

Or even better inside the template argument to clean up the function interface:

template <typename U,
          std::enable_if_t<std::is_same<U, std::chrono::nanoseconds>::value>* = nullptr >
std::string typed_unit ()
{
	return "nanoseconds";
}

You could also create your own template alias to reduce some of the duplication and make it a bit tidier, something like this:

template<typename U, typename V>
using MatchesType = std::enable_if_t<std::is_same<U, V>::value>;

template <typename U, MatchesType<U, std::chrono::nanoseconds>* = 0>
std::string typed_unit ()
{
	return "nanoseconds";
}

template <typename U, MatchesType<U, std::chrono::microseconds>* = 0>
std::string typed_unit ()
{
	return "microseconds";
}

...

Then your unit () function would just contain:
return typed_unit<UNIT>();

This comment has been minimized.

Copy link
@cryptocode

cryptocode Jan 1, 2019

Author Collaborator

Yeah, the C++14 ...if_t is nicer - fixed.

This comment has been minimized.

Copy link
@wezrule

wezrule Jan 2, 2019

Collaborator

Looks good! In C++17 there is also a helper variable template std::is_same_v<U, V>as well


namespace nano
{
enum timer_state

This comment has been minimized.

Copy link
@wezrule

wezrule Jan 1, 2019

Collaborator

This should use the modern enum class

This comment has been minimized.

Copy link
@cryptocode

cryptocode Jan 1, 2019

Author Collaborator

Yep, thanks

Show resolved Hide resolved nano/lib/timer.hpp
@clemahieu

This comment has been minimized.

Copy link
Collaborator

commented Jan 1, 2019

Would this be a replacement for the other timing we do, like RPC calls? Where else are you eyeing up to place it?

@cryptocode

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 1, 2019

@clemahieu I think so, we use std::chrono directly a few places and it's pretty verbose. I didn't want to change everything at once though. The other use is as an ad-hoc debugging tool when profiling tools don't cut it (e.g when a function is is sometimes slow due to locking or whatever, profilers just average it out - with the timer you can set a limit for when to output measurements, and child timers can do averaging of inner loops, etc)

@cryptocode cryptocode force-pushed the cryptocode:feature/timer-utility branch from 8561d66 to f6ce800 Jan 1, 2019

@zhyatt zhyatt added this to the V18.0 milestone Jan 2, 2019

@zhyatt zhyatt added this to Unassigned in V18 Jan 2, 2019

@zhyatt zhyatt moved this from Unassigned to Unscheduled in V18 Jan 2, 2019

@zhyatt zhyatt moved this from Unscheduled to Unassigned in V18 Jan 2, 2019

@wezrule

wezrule approved these changes Jan 2, 2019

@zhyatt zhyatt moved this from Unassigned to CP 1 (2018-01-09) in V18 Jan 4, 2019

@zhyatt zhyatt merged commit 6736141 into nanocurrency:master Jan 7, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.