7
\$\begingroup\$

I have written a little class that is intended to be used to spend unused time in a loop body, sleeping.

That is, I have a loop. I want every iteration to take at least X seconds. If the operations actually complete quicker, the excess time shall be spent sleeping.

My implementation is based on a class that is created in a scope, and whose destructor sleeps for the required amount of time when the object's lifetime ends.

Is this a sane approach to the problem? Are there any flaws in the implementation? Is there potential for improvements regarding coding style?

#include <chrono>
#include <thread>

class TimeSink {
public:
    typedef std::chrono::milliseconds duration_ms;

    TimeSink(duration_ms const & min_duration)
    : min_duration(min_duration), start(std::chrono::steady_clock::now()) {}

    ~TimeSink() {
        time_point end = std::chrono::steady_clock::now();
        auto elapsed = end-start;
        auto remaining_time_to_sleep = min_duration - elapsed;
        if (remaining_time_to_sleep > epsilon) {
            std::this_thread::sleep_for(remaining_time_to_sleep);
        }
    }

private:
    typedef std::chrono::time_point<std::chrono::steady_clock> time_point;
    const duration_ms epsilon = duration_ms(0);
    duration_ms min_duration;
    time_point start;
};

int main() {
    TimeSink t(TimeSink::duration_ms(1000));
}
\$\endgroup\$
7
\$\begingroup\$

Allow users to customize your TimeSink class

We achieve this by making it a template class:

template <typename TimeUnit, typename Clock>
class time_sink;

This will allow your class to be used in different scenarios; it removes restrictions.

Since the TimeUnit template parameter is the one most likely to be changed, we can provide a default argument for Clock:

template <typename TimeUnit, typename Clock = std::chrono::steady_clock>
class time_sink;

Your destructor can be simplified

Instead of doing...

time_point end = std::chrono::steady_clock::now();
auto elapsed = end-start;
auto remaining_time_to_sleep = min_duration - elapsed;
if (remaining_time_to_sleep > epsilon)
{
    std::this_thread::sleep_for(remaining_time_to_sleep);
}

...we can do:

auto time_spent = Clock::now() - begin;
if ( time_spent < min_duration )
{
    std::this_thread::sleep_for( min_duration - time_spent );
}

I find this clearer and easier to understand.

Consider using std::chrono::high_resolution_clock as your default clock type

This stack overflow question should tell you why that's the preferred clock type for timing function execution.

template <typename TimeUnit, typename Clock = std::chrono::high_resolution_clock>
class time_sink;

Concise usage with the std::chrono_literals namespace and a template function

This would simply make the code shorter. Note that there is a repetition, as we must define time_sink's time unit type.

using namespace std::chrono_literals;
using time_sink = time_sink<std::chrono::milliseconds>;
time_sink{ 500ms };

In order to make usage fully concise (avoid repetition), we can use a template function and implement the idea proposed by 5gon12eder, in this comment which simply creates the time_sink object and uses template function type deduction to fill in the details.

template <typename TimeUnit, typename Clock = std::chrono::high_resolution_clock>
time_sink<TimeUnit, Clock> make_time_sink( TimeUnit const minimum_duration )
{
    return time_sink<TimeUnit, Clock>{ minimum_duration };
}

This allows us to create a time_sink in a short and clear way:

using namespace std::chrono_literals;
make_time_sink( 500ms );
// rest of function code, etc.

The improved code

Here's what the final code could look like:

#include <chrono>
#include <thread>

template <typename TimeUnit, typename Clock = std::chrono::high_resolution_clock>
class time_sink
{
public:
    time_sink( TimeUnit const minimum_duration ) :
        min_duration{ minimum_duration },
        begin{ Clock::now() }
    {}

    ~time_sink()
    {
        auto time_spent = Clock::now() - begin;
        if ( time_spent < min_duration )
        {
            std::this_thread::sleep_for( min_duration - time_spent );
        }
    }

private:
    TimeUnit min_duration;
    std::chrono::time_point<Clock> begin;
};

template <typename TimeUnit, typename Clock = std::chrono::high_resolution_clock>
time_sink<TimeUnit, Clock> make_time_sink( TimeUnit const minimum_duration )
{
    return time_sink<TimeUnit, Clock>{ minimum_duration };
}

Sample usage

As 5gon12eder points out, you must also ensure that you keep the return of make_time_sink in order to prevent time_sink's destructor from running early, since destructors are called at the "end of the full expression, for nameless temporaries" (source).

void f()
{
    using namespace std::chrono_literals;

    // we keep the return of make_time_sink in order to prevent
    // time_sink's destructor from executing before the end of the function
    auto ts = make_time_sink( 500ms );
    // ... do work ...
}

int main()
{
    f();
}
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Is it intentional that the “obvious choice” for the clock type is once std::chrono::steady_clock and once std::chrono::high_resolution_clock? As for the function, is it meant to be a convenience wrapper around the constructor (then why the lambda?) or would it block for up tp the specified amount of time after calling the lambda (then why the name?)? \$\endgroup\$ – 5gon12eder Dec 16 '15 at 21:55
  • \$\begingroup\$ 1. I changed it without really thinking about it, I'll add a note about it. Thanks for pointing this out. 2. I did leave this part kind of vague. I didn't see it as a wrapper, but now that you point it out, that's even better. Great suggestion. \$\endgroup\$ – user2296177 Dec 16 '15 at 22:46
  • \$\begingroup\$ I like the updated answer except for one important detail: You should assign the result of make_time_sink to a variable, otherwise the destructor will block right away when the temporary is destroyed. It doesn't matter for the main, however, because there is no other statement in its body anyway, but you should change it in the shorter snippet above. \$\endgroup\$ – 5gon12eder Dec 17 '15 at 1:49
  • \$\begingroup\$ @5gon12eder Thanks for pointing out the oversight. I'll include a better example. \$\endgroup\$ – user2296177 Dec 17 '15 at 1:57
  • \$\begingroup\$ You seem to use the braced initializer list everywhere. Is this the new way to initialize things? When would we resort to the "old" style? \$\endgroup\$ – moooeeeep Dec 17 '15 at 8:14

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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