7
\$\begingroup\$

I used the std::chrono library for computing the timing of a function and need some review on it.

Here is the code:

namespace gstd
{

#ifdef GTIME
    inline namespace gtime
    {
        template<typename type, typename period>
        using duration = std::chrono::duration<type, period>;
        std::function<std::chrono::time_point<std::chrono::steady_clock>( )>current_time = std::chrono::high_resolution_clock::now;

        template<typename RetType>
        std::optional<double> getFunctionTime(std::function<RetType(void)>function) {
            try {    
                auto start_time = current_time( );
                function( );
                auto end_time = current_time( );
                duration<double, std::milli> dur = end_time - start_time;
                return dur.count( );
            }
            catch ( ... ) {
                return {};    
            }
        }
        template <typename RetType, typename arg>
        std::optional<double> getFunctionTime1(std::function<RetType(arg)> function, arg value) {
            try {
                auto start_time = current_time( );
                function(value);
                auto end_time = current_time( );
                duration<double, std::milli> dur = end_time - start_time;
                return dur.count( );
            }
            catch ( ... ) {
                return {};
            }
        }
        template<typename RetType, typename arg1, typename arg2>
        std::optional<double> getFunction2(std::function<RetType(arg1, arg2)>function, arg1 firstparam, arg2 secondparam) {
            try {                   
                auto start_time = current_time( );
                function(firstparam, secondparam);
                auto end_time = current_time( );
                duration<double, std::milli> dur = end_time - start_time;
                return dur.count( );
            }
            catch ( ... ) {
                return {};
            }
        }
    }

#endif // GTIME    
}

This is the code in the header file. Now how i am using it is here:

#define GTIME

#include "GStandard.h"
#include <iostream>

int number_crunching( ) {
    int sum { 0 };
    for ( int i = 0; i < 10000000; i++ )
        sum += 1;
    return sum;
}
int multiplier(int number) {
    for ( int i = 0; i < 10000000; i++ ) {
        number *= ( i + 1 );    
    }
    return number;
}
int sum( int a, int b) {
    return a + b;
}
int main( ) {
    auto ftime = gstd::getFunctionTime<int>(number_crunching);
    auto f1time = gstd::getFunctionTime1<int, int>(multiplier, 5);
    auto f2time = gstd::getFunction2<int, int,  int>(sum, 4, 5);

    std::cout << ftime.value( ) << '\n';
    std::cout << f1time.value( ) << '\n';
    std::cout << f2time.value( ) << '\n';

    std::cin.get( );
    return 0;
}

Some suggestions were previously made on making it variadic but I am not doing it so that the students at my college can make improvement to this code. I am looking forward to removing the try catch block and if there are any more changes that you guys could suggest that would be very nice.

\$\endgroup\$

1 Answer 1

Reset to default
10
\$\begingroup\$

Be more generic

As I've suggested here, you should not force the use of an std::function. Instead, take any type of callable and either a fixed or an unlimited number of arguments, using std::invoke inside your implementation.

More importantly, you really don't need multiple functions for a different number of arguments; use variadic templates, e.g. have a signature such as:

template< class F, class... Args>
std::optional<double> getFunctionTime(F&& f, Args&&... args) noexcept;

(ignoring any other potential changes)

This will make it easier for you to invoke, too:

auto ftime = gstd::getFunctionTime(number_crunching);
auto f1time = gstd::getFunctionTime(multiplier, 5);
auto f2time = gstd::getFunctionTime(sum, 4, 5);

Alternatively, you could only accept 0-argument runnables, in which case you simply pass lambdas:

auto f1time = gstd::getFunctionTime([]() { return multiplier(5); });

Return a chrono::duration

Don't convert your duration to a plain numeric value - keep the more specific type.

Use integral values for durations

Why use a floating-point value in the duration? Stick with integral values unless you have a good reason to do otherwise.

Consider changing the name

I'd use time_an_invocation or time_invocation but I guess that's a matter of taste.

Consider leaving it to the caller to handle exceptions

You're forcing a certain behavior w.r.t. exceptions. I'd consider simply not handling them, so that timing the invocation of a function that throws an exception, throws an exception. That would simplify your code and avoid a dependence on std::optional, which the caller might not be interested in. Remember the caller has to have additional code to handle the empty-optional case anyway.

Consider using a stopwatch class instead

In a related question, @Edward suggests keeping the invocation and the time measurement separate, using a "stop-watch" class. That might not be "better" but it's certainly worth considering.

\$\endgroup\$

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.