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

Fix issues in Aux::Timer #368

Merged
merged 9 commits into from
Jul 19, 2019
Merged

Fix issues in Aux::Timer #368

merged 9 commits into from
Jul 19, 2019

Conversation

manpen
Copy link
Contributor

@manpen manpen commented Jul 2, 2019

Clang rightfully complained about Aux::Timer not having a virtual destructor.While fixing this, I spotted a number of additional issues, justifying a PR on its own.

This PR includes:

  • A virtual destructor for Timer
  • Most of Timer's methods are now const and nothrow (where applicable)
  • A unit test for Timer
  • A bug fix for elapsedMicroseconds() and elapsedNanoseconds(): The documentation stated that elapsed*() methods returned the time elapsed since the start and now (if Timer is still running) or
    between start and stop if the Timer was stopped. elapsed() and elapsedMilliseconds() behaved that way, while elapsedMicroseconds() and elapsedNanoseconds() only considered the latter case.
  • Timer defined the macro my_steady_clock. While this should have been a using declaration not polluting global namespace.
  • Add StartedTimer which is identical to Timer but does automatically call start() on construction.

Remarks:

  • I do not see a reason to have virtual members in Timer and suggest to drop that (AFAICS its not used in NetworKit itself). But there might be external users. So I did not do it yet.
  • I do know why code for MIC uses std::chrono::monotonic_clock
  • To choice of returning a std::chrono::duration in integer milliseconds seems strange; but we cannot really change that now.

@avdgrinten
Copy link
Contributor

I think we should just remove the virtual methods. Given that the methods return steady_clock::time_point, there is really no other implementation possible.

monotonic_clock was a name that appeared in earlier drafts of C++11, I guess that the Intel MIC compiler does not (or did not?) follow the updated standard.

@manpen manpen force-pushed the fix/Timer branch 2 times, most recently from a9e0dbc to 2150c3a Compare July 13, 2019 14:47
* to exist until the end of the scope. If the time measured seems too short, make you created
* a named instances.
*/
class ScopedTimer : public StartedTimer {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This class has no users. Do we need it? When / how will users be added?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I consider this a developing tool. I typically have a copy of this class in my source tree and remove it before pushing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case, would you consider renaming it to something like DebuggingTimer / DevelopmentTimer / LoggingTimer (or some bikesheded version of that)?

* to exist until the end of the scope. If the time measured seems too short, make you created
* a named instances.
*/
class ScopedTimer : public StartedTimer {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case, would you consider renaming it to something like DebuggingTimer / DevelopmentTimer / LoggingTimer (or some bikesheded version of that)?

*
* @warning Similarly as std::unique_lock the timer needs a name ("someName" in the example)
* to exist until the end of the scope. If the time measured seems too short, make you created
* a named instances.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I first I thought you were referring to the label. name -> named variable.


ss << "ran for " << (elapsedMicroseconds() * 1e-3) << " ms";

std::cout << ss.str() << std::endl; // we really want to flush here!
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should use NetworKit's logging framework, e.g., INFO().

manpen and others added 9 commits July 17, 2019 17:13
The documentation said that elapsed*() methods returned the time
elapsed since the start and now (if Timer is still running) or
between start and stop if the Timer was stopped.

elapsed() and elapsedMilliseconds() behaved that way, while
elapsedMicroseconds() and elapsedNanoseconds() only considered stop.

Also added const and nothrow where applicable.
@manpen
Copy link
Contributor Author

manpen commented Jul 17, 2019

Thanks for your feedback. For a proper LoggingTimer I wanted the following features:

  • User can provide loglevel at which it's active
  • If the Timer is inactive it shoud be very cheap, s.t. we can leave it in the code (nb. calling std::chrono::*::now() roughly takes 2.5 us on my system, so you really want to deactivate it, if it's not going to be used).

To do so, I added two functions to Log.h:

  • isLogLevelEnabled(loglevel) returns true iff the level will currently cause an output (duh!)
  • Add LOG_AT(level, ...) and LOG_ATF(level, ...) macros which basically act like INFO and such but only with a user provided loglevel.

Note that based on LOG_AT(F?) all other macros are much easier to express ...

One "bug" the current solution has is that it will print the line number of the LoggingTimer's destructor if srcloc is enabled. The only solution around that is to provide the LoggingTimer via a macro which I do not really like. I consider this acceptable.

The PR also contains a mini-bugfix for a recently introduced signed/unsigned comparison; let me know, if I should separate it into its own PR.

(BTW: The diff of the force-push only is that large because I rebased to current Dev)

@avdgrinten
Copy link
Contributor

Thank you for the revision. The LoggingTimer interface looks much better now; I think this has been a real improvement 👍. The whole PR LGTM.

@avdgrinten avdgrinten merged commit 01a19bf into networkit:Dev Jul 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants