-
Notifications
You must be signed in to change notification settings - Fork 73
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 hacky version of calculate_max_depth
with simpler one.
#2320
Conversation
} | ||
auto implied_count = next_power_of_2(node_count); | ||
return clz_power_2(implied_count) + 1; | ||
return std::llround(std::ceil(std::log2(node_count))) + 1; |
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 am not so sure about using floating point math for this. How about,
return std::llround(std::ceil(std::log2(node_count))) + 1; | |
return 8*sizeof(node_count) - std::countl_zero(std::bit_ceil(node_count)); |
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.
Great! Even better!
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.
actually even better is return std::bit_width(std::bit_ceil(node_count))
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.
Interestingly this return std::countr_zero(std::bit_ceil(node_count)) + 1;
is slower than the countl
version.
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'm surprised there'd be much of a difference if you've got access to lzcnt
(either through -mlzcnt
or something like -march=skylake
) since they pretty much compile down to the same handful of instructions.
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.
You're right, no difference.
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.
actually even better is
return std::bit_width(std::bit_ceil(node_count))
using this!
Note:start |
In PR #2314, I asked why we were using overly complex code to do what was basically a log base 2.
Gnome mentioned that the original code was in
incremental_merkle_tree.hpp
.So I propose this simplification. The replacement provides exactly the same result as the previous implementation, as the following test program demonstrates: