Replace Custom Benchmarking Code with Nanobench #6357
Conversation
|
This still needs work on the fetching mechanism |
|
|
||
| json_perf_times& times() | ||
| char const* nanobench_hpx_template() noexcept |
| OFF | ||
| CATEGORY "Build Targets" | ||
| ADVANCED | ||
| ) |
There was a problem hiding this comment.
Should we fetch this by default only if HPX_WITH_TESTS_BENCHMARKS=On?
There was a problem hiding this comment.
Yes, so if hpx is built with benchmarks on, by default we fetch nanobench unless the user species the nanobench root directory.
Should the default case be the non-nanobench version?
| @@ -28,6 +29,6 @@ add_hpx_module( | |||
| HEADERS ${testing_headers} | |||
| COMPAT_HEADERS ${testing_compat_headers} | |||
| MODULE_DEPENDENCIES hpx_assertion hpx_config hpx_format hpx_functional | |||
| hpx_preprocessor hpx_util | |||
| hpx_preprocessor hpx_util nanobench | |||
There was a problem hiding this comment.
Will this work if nanobench is not fetched?
There was a problem hiding this comment.
I don't think so, this part is really incomplete, I tried fetching nanobench only for internal use by this module but while it builds, cmake throws some non fatal errors, as the nanobench module is not being properly exported. I am working on that
| @@ -10,6 +10,7 @@ | |||
| #include <hpx/functional/function.hpp> | |||
|
|
|||
| #include <cstddef> | |||
| #include <iostream> | |||
There was a problem hiding this comment.
Is #include <iosfwd> sufficient?
|
|
||
| map_t m_map; | ||
| #define NANOBENCH_EPOCHS 24 | ||
| #define NANOBENCH_WARMUP 40 |
There was a problem hiding this comment.
Can you convert this to constexpr variables?
| // templ is a mustache-style template for the output | ||
| // used by the nanobench reporter | ||
| HPX_CORE_EXPORT void perftests_print_times( | ||
| std::ostream& strm = std::cout, char const* templ = nullptr); |
There was a problem hiding this comment.
Could we keep std::cout out of the API?
There was a problem hiding this comment.
Sure thing! Which of the following do you think is the better way to do it?
- Leave
std::coutas the default stream, but handle it on the.cppfile with polymorphism instead of the header. - Do not provide a default stream.
The original API does not accept a stream parameter
There was a problem hiding this comment.
Let's add another overload that allows to specify the ostream and handle the default case in the source file.
A while ago I had tried using google benchmark https://github.com/SAtacker/hpx-template, even if you don't want to change to google bench the cmake fetch content stuff should be similar |
|
Continued by #6448 |
This Pull Request focuses on updating the benchmarks and performance testing functionality of HPX. This involves replacing the previous custom performance timing code with the Nanobench library and incorporating the usage of Mustache templates for intuitive results formatting.
The key benefits are:
Still needs: Getting nanobench into the build system properly