7
\$\begingroup\$

I have written the following simple scoped timer struct in order to help me measure the execution time of arbitrary scopes.

Here is the code (live):

#include <chrono>
#include <thread>
#include <cstdio>
#include <fmt/core.h>
#include <fmt/chrono.h>


template < class Time  = std::chrono::microseconds,
           class Clock = std::chrono::steady_clock >
requires ( std::chrono::is_clock_v<Clock> )
struct ScopedTimer
{
    const std::chrono::time_point< Clock > m_start { Clock::now( ) };
          std::chrono::time_point< Clock > m_end;
    std::FILE* m_stream;

    ScopedTimer( std::FILE* const stream = stderr ) noexcept( noexcept( Clock::now( ) ) )
    : m_stream { stream }
    {
    }
    ~ScopedTimer( )
    {
        m_end = Clock::now( );

        fmt::print( m_stream, "\nTimer took {}\n",
                    std::chrono::duration_cast<Time>( m_end - m_start ) );
    }
    ScopedTimer( const ScopedTimer& ) = delete;
    ScopedTimer& operator=( const ScopedTimer& ) = delete;
};


int main( )
{
    // default instantiation
    ScopedTimer timer;
    // another instantiation
    // ScopedTimer<std::chrono::milliseconds, std::chrono::system_clock> timer;

    using std::chrono_literals::operator""ms;
    std::this_thread::sleep_for( 500ms );
}

Now I have a few question regarding its design:

  1. How do I put constraints on the template parameter Time just like I have done for Clock inside the requires clause? Because something like std::chrono::is_duration_v<Time> doesn't exist in the standard library.
  2. Should it only allow clock types that are trivial (i.e. satisfy the requirements of TrivialClock) or is it fine if it allows non-trivial ones too? If it should then how can I implement the constraint?
  3. Regarding the exception safety of the ctor and the dtor, for a given trivial clock, its now() method does not throw unlike a non-trivial clock. So in case I allow non-trivial clock types, then how should I make the ctor and dtor noexcept? My current idea is to use noexcept( noexcept( Clock::now() ) ) as can be seen in the above code snippet. And what should I do for the dtor since exceptions should not escape the dtor?
  4. Speaking of access specifiers, should the three member variables be public or private? I consider public as a better option since it allows the client code to access them using the dot operator (obj._member) without the need for getters or setters.
  5. Could any standard attributes (e.g. [[maybe_unused]]) be used to further enhance the correctness of the above struct?

Any other suggestions are welcome.

\$\endgroup\$

2 Answers 2

7
\$\begingroup\$

Answers to your questions

  1. How do I put constraints on the template parameter Time just like I have done for Clock inside the requires clause? Because something like std::chrono::is_duration_v<Time> doesn't exist in the standard library.

You can create a concept yourself, for example:

template<typename Clock, typename Duration>
concept can_yield_duration = requires(Clock::time_point t) {
    std::chrono::duration_cast<Duration>(t - t);
};

template < class Time  = std::chrono::milliseconds,
           class Clock = std::chrono::steady_clock >
requires ( std::chrono::is_clock_v<Clock> && can_yield_duration<Clock, Time> )
struct ScopedTimer …
  1. Should it only allow clock types that are trivial (i.e. satisfy the requirements of TrivialClock) or is it fine if it allows non-trivial ones too? If it should then how can I implement the constraint?

Don't restrict things unnecessarily. Even if you cannot see the usefulness, someone else might. If you are sure using any non-trivial clock type would cause the template to fail to instantiate or would cause it to do the wrong thing, then it might make sense to constrain it.

  1. Regarding the exception safety of the ctor and the dtor, for a given trivial clock, its now() method does not throw unlike a non-trivial clock. So in case I allow non-trivial clock types, then how should I make the ctor and dtor noexcept? My current idea is to use noexcept( noexcept( Clock::now() ) ) as can be seen in the above code snippet. And what should I do for the dtor since exceptions should not escape the dtor?

The noexcept specifier you wrote for the constructor looks fine to me. As for the destructor, you can use a noexcept specifier for it as well. But would it be useful? Would anyone catch it? I would either just not specify it, then it will implicitly be noexcept(true), and then any exception thrown inside the destructor will cause the program to abort. Alternatively, you could catch any exception in the destructor and then just print an error message, although fmt::print() can throw as well.

  1. Speaking of access specifiers, should the three member variables be public or private? I consider public as a better option since it allows the client code to access them using the dot operator (obj._member) without the need for getters or setters.

I don't see any reason for client code to modify those member variables. The more you make public, the less chance you have of making changes to your code would braking backwards compatibility.

  1. Could any standard attributes (e.g. [[maybe_unused]]) be used to further enhance the correctness of the above struct?

I don't think so. Your class has side effects, so [[maybe_unused]] is not necessary. I also don't think any other attribute applies.

Simpler way to write types

Instead of writing std::chrono::time_point<Clock>, you can write Clock::time_point. If you are going to write the same long type name multiple times, you could also consider creating an alias for it:

using time_point = typename Clock::time_point;
const time_point m_start = { Clock::now(); };

Avoid declaring unnecessary member variables

The variable m_end is only used inside the destructor, so there is no need to make this a member variable, just make it a local function variable.

Don't use C I/O in a C++ program

Why use FILE* in a C++ program? You should pass a reference to a std::ostream instead:

std::ostream& m_stream;

ScopedTimer( std::ostream& stream = std::cerr ) noexcept(…)
    : m_stream { stream }
{
}

What if you don't want to print the elapsed time?

If all you care about is that the elapsed time is printed to some stream, this is fine. But what if you want to do something else? You could consider passing a callback function instead of passing a stream to the constructor, and have it call that function in the destructor, so you can write something like:

ScopedTimer timer( [] ( auto duration ) {
    fmt::print( std::cerr, "Timer took {}\n", duration.count() );
});

This removes responsibility from the ScopedTimer class, and while it does less, it is now much more flexible.

I would then also not have Time as a template parameter, but just have the result of the subtraction passed directly to the callback function, which can then decide if it wants to do a duration cast or not.

To pass a function to the constructor and to store it until the timer is destructed, use std::function:

#include <chrono>
#include <fmt/core.h>
#include <fmt/ostream.h>
#include <functional>
#include <iostream>

template < class Clock = std::chrono::steady_clock >
requires ( std::chrono::is_clock_v< Clock > )
class ScopedTimer
{
    using TimePoint = typename Clock::time_point;
    using Duration = typename Clock::duration;
    using Callback = std::function< void( Duration ) >;

    Callback m_callback;
    TimePoint m_start{ Clock::now() };

    static void default_callback( Duration duration ) {
        fmt::print( std::cerr, "Timer took {}\n", duration.count() );
    }

public:
    ScopedTimer( Callback callback = default_callback )
        : m_callback { callback }
    {
    }

    ~ScopedTimer()
    {
        TimePoint end{ Clock::now() };
        m_callback( end - m_start );
    }
};

For demonstration purposes I've also shown how you can still have a default value for the callback parameter, however I would not include it, as it introduces a dependency on both <fmt/*.h> and <iostream>.

\$\endgroup\$
5
  • \$\begingroup\$ Thanks a lot for the tips. Your last suggestion seems very interesting to me. I don't exactly know how to implement it. Could you implement one as a sample so I can understand the basics of passing callbacks to a class ctor? \$\endgroup\$
    – digito_evo
    Feb 4, 2023 at 11:19
  • 1
    \$\begingroup\$ I've added an example of the entire class with support for callbacks, I hope that helps. \$\endgroup\$
    – G. Sliepen
    Feb 4, 2023 at 13:04
  • \$\begingroup\$ Thanks again. And I have made my own custom struct based on the two answers that I've got. Please have a look at it. I'd like to know what you think about it (link). If you guys accept it as a good solution I will post it as an answer. \$\endgroup\$
    – digito_evo
    Feb 5, 2023 at 22:56
  • \$\begingroup\$ That's great, but you should post it as a new question here on Code Review. \$\endgroup\$
    – G. Sliepen
    Feb 5, 2023 at 23:37
  • 1
    \$\begingroup\$ Ok then. I will do it soon. \$\endgroup\$
    – digito_evo
    Feb 6, 2023 at 8:24
9
\$\begingroup\$

I don't see why m_end is a member, as it's unassigned for the object's entire life, and only meaningful within the destructor. I would make it a local variable during destruction.

The constructor ought to be explicit.

I don't think we need to prohibit copy construction - there may be use-cases for a series of timings with common start time.

Beware of exceptions thrown in a destructor, as that will cause program termination. The documentation I've found for fmt::print() is silent on which exceptions it might throw. In any case, I recommend sticking to standard C++ and using <iostream> in preference to <stdio> (which makes it more flexible, as that enables writing to non-file streams such as std::ostringstream, and also allows us to use the standard log stream rather than error stream).

Instead of the the constraint on Clock alone, I would test the combination of Clock and Time thus:

    requires requires { std::chrono::time_point<Clock,Time>{}; }

Consider storing the time-point in the desired output precision (i.e. std::chrono::time_point<Clock,Time>).


Modified code

#include <chrono>
#include <iostream>

template<typename Time = std::chrono::microseconds,
         typename Clock = std::chrono::steady_clock>
    requires requires { std::chrono::time_point<Clock,Time>{}; }
struct ScopedTimer
{
    std::chrono::time_point<Clock,Time> const m_start = now();
    std::ostream& m_stream;

    explicit ScopedTimer(std::ostream& stream = std::clog) noexcept(noexcept(now()))
        : m_stream{stream}
    {}

    ~ScopedTimer()
    {
        try {
            m_stream << "Timer took " << (now() - m_start) << '\n';
        } catch (...) {
            // ignore
        }
    }

    auto now() const noexcept(noexcept(Clock::now()))
    {
         return std::chrono::time_point_cast<Time>(Clock::now());
    }
};
\$\endgroup\$
4
  • 1
    \$\begingroup\$ Oh nice, I totally missed that you can just pass both the clock and time type as template parameters to std::chrono::time_point! I also never used time_point_cast() before... TIL! \$\endgroup\$
    – G. Sliepen
    Feb 3, 2023 at 9:04
  • \$\begingroup\$ My first use of that function, too! \$\endgroup\$ Feb 3, 2023 at 9:11
  • \$\begingroup\$ Extremely helpful tips. Thanks a lot. Also do you I should mark those three member functions (ctor, dtor and now) as inline? \$\endgroup\$
    – digito_evo
    Feb 4, 2023 at 11:23
  • \$\begingroup\$ I don't see a good reason to use inline. But some compilers find it a useful hint to their optimisers. \$\endgroup\$ Feb 7, 2023 at 8:13

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.