nonius icon indicating copy to clipboard operation
nonius copied to clipboard

Adjust macro interface for generating benchmarks

Open acki-m opened this issue 9 years ago • 6 comments

The interface for automatically register benchmarks is the following:

NONIUS_BENCHMARK("My Benchmark", [](nonius::chronometer meter)
{
//...
}) 

Because of that it is not possible to use #ifdef inside the benchmark itself. it looks also not nice, see the end of the braces? }) This makes it harder for the client writing benchmarks.

I came up with following idea:

NONIUS_BENCHMARK("Group Sorting", "Sort 1")
{
    meter.measure([&]() 
    {
       // benchmark code
    }
}

You can look in the testing framework catch, which you already use, how it is implemented there, but it's pretty easy. Basically the macro has to expand to a temporary function which gets as argument this nonius::chronometer meter

Additionally: Like you can see, I would like to group benchmarks together, but this is another big request, but you should keep this in mind when redesigning the interface.

acki-m avatar May 02 '15 06:05 acki-m

I considered a similar option when I was designing the interface, but now I don't quite remember why I didn't do it that way. I should have written notes :| I'll look into it.

rmartinho avatar May 07 '15 09:05 rmartinho

So, the reason I didn't do this is simple: the benchmark function can return anything. That's by design. I can't use the same technique that Catch uses, which relies on the ability to declare the function ahead of time, since Catch test functions always have a void return type.

So, an implementation as follows, would not work, because we need to define this T type.

#define NONIUS_BENCHMARK_INTERNAL(name, internal_name, ...) \
    namespace { \
        struct internal_name { \
            static T run(::nonius::chronometer meter); \
        }; \
        static ::nonius::benchmark_registrar NONIUS_DETAIL_UNIQUE_NAME(benchmark_registrar) \
               (::nonius::global_benchmark_registry(), name, &internal_name::run); \
    } \
    T internal_name::run(::nonius::chronometer meter)

Suggestions are welcome. Note that the function needs to be used before the definition, so C++14 auto won't cut it.

rmartinho avatar May 06 '16 19:05 rmartinho

Hi!

I know this is closed, but I do have an idea that might work. The idea is to achieve a syntax like this:

NONIUS_BENCHMARK("do sth") [](nonius::chronometer meter) {
    ...
};

Note that here the user can still choose the signature, so the argument-less version or the parameters version as suggested in #56 are all possible. The way this would work is by having the registrar define a binary operator (for example operator +) that takes a partial registrar capturing the name of the benchmark and whatever other metadata is passed to the macro, and as second argument, it takes a callable (via a template). The macro then would expand to something like:

#define NONIUS_BENCHMARK(__VA_ARGS__)                            \
    static auto NONIUS_DETAIL_UNIQUE_NAME(benchmark_registrar) = \
                    ::nonius::benchmark_registrar{__VA_ARGS__} +

The only difference is that the thing can not be wrapped in an anonymous namespace, but that should not matter because it is static anyways.

Do you think this makes it worth reopening the issue @rmartinho?

arximboldi avatar Aug 07 '16 15:08 arximboldi

That sounds feasible. I reopened it.

Note that this would be a breaking change (though one with a simple migration path) unless we introduce a differently-named macro.

rmartinho avatar Aug 08 '16 14:08 rmartinho

Actually, the two syntaxes can be disambiguated by the number of arguments used so it doesn't really need to be a breaking change.

rmartinho avatar Aug 08 '16 14:08 rmartinho

@rmartinho yeah, I think that this can be done in an "almost" non breaking change. The problem is that the macro code to disambiguate the number of parameters hardcodes the maximum number of parameters allowed. The way the old macro works, the number of arguments depends on the number of (un-parenthesized) commas in the benchmarking function provided. I would imagine that some users may write benchmarking functions with lots of commas, when they include some test data in the definition. On the other hand hardcoded number may be made arbitrarily large, at the expense of compile time. It's a trade-off, and I myself can't decide what would be better (API break, new name or "almost" backward compatibility).

arximboldi avatar Aug 08 '16 14:08 arximboldi