-
-
Notifications
You must be signed in to change notification settings - Fork 427
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Replace Custom Benchmarking Code with Nanobench #6357
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make this constexpr?
OFF | ||
CATEGORY "Build Targets" | ||
ADVANCED | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we fetch this by default only if HPX_WITH_TESTS_BENCHMARKS=On
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this work if nanobench is not fetched?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is #include <iosfwd>
sufficient?
|
||
map_t m_map; | ||
#define NANOBENCH_EPOCHS 24 | ||
#define NANOBENCH_WARMUP 40 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we keep std::cout
out of the API?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure thing! Which of the following do you think is the better way to do it?
- Leave
std::cout
as the default stream, but handle it on the.cpp
file 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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