-
Notifications
You must be signed in to change notification settings - Fork 71
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
add eytzinger to benchmarks #372
Conversation
a88dc9f
to
d5dd9ec
Compare
@HDembinski I was trying to reproduce the errors caused by clang version changing from 10 to 14 (seems the commit disappered at this noon so do the errors). But following
|
I fixed the bugs a few hours ago and rebased this PR that's why the errors disappeared, you can implement your code now. Do not worry about coverage for now, coverage measurements are flaky. You found the right script, but it is not tested on Windows, so probably does not work. I will update the CONTRIBUTE.md. You also figured out how to install gcc on Windows, very good, but you can also compile with msvc. Only for coverage you need gcc. gcc-8 is known to work, but gcc-12 (I tested yesterday) also works. You need a matching version of gcov (gcov-12) for it to work, this is important if you have several different versions of gcc installed. The script tools/cov.py is not smart enough to pick the matching version of gcov. b2filt may or may not work on Windows, it is not tested on Windows. |
@jhcarl0814 I found a cross-platform implementation of prefetch in Boost.Context (it often pays off to mine Boost code for hidden gems). I added that so that we can experiment with it. On my platform, prefetch does not improve performance. |
@HDembinski That's cool! At least the missing part is filled. Maybe there are other scenarios or platforms that will benefit from it, but we don't know yet. We'll see. |
@HDembinski Sorry to trouble you, this is my first time to do PR. The change (has passed The forking_repo's Should I The git remote config is like this:
The default layout_and_algorithm is controled by default type template parameter in struct sorted_array_and_binary_search{};
struct eytzinger_layout_and_eytzinger_binary_search{};
template <class Value = double, class MetaData = use_default, class Options = use_default,
class Allocator = std::allocator<Value>, class DataStructure = eytzinger_layout_and_eytzinger_binary_search>
class variable; The actual code of each layout_and_algorithm is in template <typename T = double>
struct sorted_array_and_binary_search {...};
template <typename T = double>
struct eytzinger_layout_and_eytzinger_binary_search {...}; |
There are some other very small problems found when reading the code: 1
It would be better to let user use a type alias and seldom use the class template name, so the conditional operations can terminate at the type alias and does not propagate further. namespace boost
{
namespace histogram
{
namespace axis
{
// template<class Value = double, class MetaData = use_default, class Options = use_default, class Allocator = std::allocator<Value>, class DataStructure = eytzinger_layout_and_eytzinger_binary_search>
// using variable_t = boost::histogram::axis::variable<
// std::conditional_t<std::is_same_v<Value, use_default>, double, Value>,
// std::conditional_t<std::is_same_v<MetaData, use_default>, std::string, MetaData>,
// std::conditional_t<std::is_same_v<Options, use_default>, decltype(option::underflow | option::overflow), Options>,
// std::conditional_t<std::is_same_v<Allocator, use_default>, std::allocator<std::conditional_t<std::is_same_v<Value, use_default>, double, Value>>, Allocator>,
// std::conditional_t<std::is_same_v<DataStructure, use_default>, eytzinger_layout_and_eytzinger_binary_search, DataStructure>
// >;
template<class Value = double, class MetaData = use_default, class Options = use_default, class Allocator = std::allocator<Value>, class DataStructure = eytzinger_layout_and_eytzinger_binary_search>
using variable_t = boost::histogram::axis::variable<
detail::replace_default<Value,double>,
detail::replace_default<MetaData,std::string>,
detail::replace_default<Options,decltype(option::underflow | option::overflow)>,
detail::replace_default<Allocator,std::allocator<detail::replace_default<Value,double>>>,
detail::replace_default<DataStructure,eytzinger_layout_and_eytzinger_binary_search>
>;
}
} // namespace histogram
} // namespace boost
static_assert(!std::is_same_v<boost::histogram::axis::variable<double, std::string, boost::use_default>, boost::histogram::axis::variable<double, std::string, decltype(boost::histogram::axis::option::underflow | boost::histogram::axis::option::overflow)>>);
static_assert(std::is_same_v<boost::histogram::axis::variable_t<double, std::string, boost::use_default>, boost::histogram::axis::variable_t<double, std::string, decltype(boost::histogram::axis::option::underflow | boost::histogram::axis::option::overflow)>>); 2
3The {
// using A = axis::variable<double, axis::null_type, op::circular_t>; // using double solves the errors
using A = axis::variable<float, axis::null_type, op::circular_t>;
auto a = A({
1.,
1e+8,
2e+8,
});
BOOST_TEST_EQ(a.index(1e8), 1);
BOOST_TEST_EQ(a.index(2e8), 0); // test 'a.index(2e8) == 0' ('-1' == '0') failed
BOOST_TEST_EQ(a.index(4e8), 0); // test 'a.index(4e8) == 0' ('-1' == '0') failed
}
{
// using A = axis::variable<double, axis::null_type, op::circular_t>; // using double solves the errors
using A = axis::variable<float, axis::null_type, op::circular_t>;
auto a = A({
-2e+8,
-1e+8,
-1.,
});
BOOST_TEST_EQ(a.index(-1e8), 1);
BOOST_TEST_EQ(a.index(-2e8), 0);
BOOST_TEST_EQ(a.index(-4e8), 1); // test 'a.index(-4e8) == 1' ('0' == '1') failed
}
return boost::report_errors(); |
Your points 1 to 3 sound interesting, but I do not fully understand you yet.
But let's not get sidetracked. We should discuss eytzinger here, feel free to open issues for the other topics so that we can discuss them separately. |
The failure on Windows is caused by the new detail/prefetch.hpp code. It looks like I oversimplified the original implementation, this needs a better platform check. Edit: The original code seemed to be wrong?! Anyway, it works now. |
I just made another commit to this branch to fix the failure on Windows, please rebase your branch (or don't if you already fixed that in your branch). Then sync your local branch to your fork on GitHub. Then browse to your fork on the Github website and use the website interface to start a pull request onto the develop branch of boostorg/histogram. This should create another PR. I should be able to make changes to your PR and we can close this PR then. |
@HDembinski The point 1 intends to:
(there are two implementations of We can ignore this for now because it's not a big issue and it's too easy for users to add this layer of abstraction themselves. |
The pull request is created at #376 . (The design is very strange because a shallow type and a stateful type are occupying the same position and should be made interchangable.) I will wait for the checks and fix the syntax/semantic errors first. |
I still do not understand what your template alias achieves that the current implementation does not. The library was designed so you do not have to specify the template arguments if you are ok with the defaults. In C++14, you can use
To get the defaults. In C++17, you can drop the <> and let the deduction guides select the right type. What advantages does your idea have over this scheme? What kind of additional abstraction is needed? |
@HDembinski Here are some examples why using std::is_same_v<
boost::histogram::axis::variable<double, std::string, boost::use_default>,
boost::histogram::axis::variable<double, std::string, decltype(boost::histogram::axis::option::underflow | boost::histogram::axis::option::overflow)>
> is std::is_same_v<
boost::histogram::axis::variable_t<double, std::string, boost::use_default>,
boost::histogram::axis::variable_t<double, std::string, decltype(boost::histogram::axis::option::underflow | boost::histogram::axis::option::overflow)>
> is This causes a compile error: boost::histogram::axis::variable<double, std::string, boost::use_default> a;
boost::histogram::axis::variable<double, std::string, decltype(boost::histogram::axis::option::underflow | boost::histogram::axis::option::overflow)> b;
a = b; This does not cause a compile error: boost::histogram::axis::variable_t<double, std::string, boost::use_default> a;
boost::histogram::axis::variable_t<double, std::string, decltype(boost::histogram::axis::option::underflow | boost::histogram::axis::option::overflow)> b;
a = b; In templates, you can get the meta type, options type, allocator type conveniently if template users write
In debugger, you can not see what options boost::histogram::axis::variable<> a;
boost::histogram::axis::variable_t<> b; |
This PR is continued in #376 |
Replace standard search in axis::variable with faster eytzinger search.
Closes #369
@jhcarl0814 Please fork this branch to work on this feature. I made a start and added the benchmark.
To-do
histogram/detail/eytzinger_search.hpp
)axis/variable.hpp
with new search, calling implementation indetail/eytzinger_search.hpp