-
Notifications
You must be signed in to change notification settings - Fork 59
Replace hash_gid
with FNV-1a
#422
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
base: rolling
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -234,8 +234,7 @@ std::string qos_to_keyexpr(const rmw_qos_profile_t & qos); | |
std::optional<rmw_qos_profile_t> keyexpr_to_qos(const std::string & keyexpr); | ||
} // namespace liveliness | ||
|
||
///============================================================================= | ||
size_t hash_gid(const std::array<uint8_t, RMW_GID_STORAGE_SIZE> gid); | ||
using Gid = std::array<uint8_t, RMW_GID_STORAGE_SIZE>; | ||
} // namespace rmw_zenoh_cpp | ||
|
||
///============================================================================= | ||
|
@@ -270,6 +269,29 @@ struct equal_to<rmw_zenoh_cpp::liveliness::ConstEntityPtr> | |
return lhs->keyexpr_hash() == rhs->keyexpr_hash(); | ||
} | ||
}; | ||
|
||
template<> | ||
struct hash<rmw_zenoh_cpp::Gid> | ||
{ | ||
std::size_t operator()(const rmw_zenoh_cpp::Gid & gid) const noexcept | ||
{ | ||
// This function implemented FNV-1a 64-bit as the GID is small enough | ||
// (e.g. as of commit db4d05e of ros2/rmw RMW_GID_STORAGE_SIZE is set to 16) | ||
// and FNV is known to be efficient in such cases. | ||
// | ||
// See https://github.com/ros2/rmw/blob/db4d05e/rmw/include/rmw/types.h#L44 | ||
// and https://datatracker.ietf.org/doc/html/draft-eastlake-fnv-17.html | ||
static constexpr uint64_t FNV_OFFSET_BASIS_64 = 0x00000100000001b3; | ||
static constexpr uint64_t FNV_PRIME_64 = 0xcbf29ce484222325; | ||
Comment on lines
+278
to
+285
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So we are already using xxhash3 in liveliness_utils to take a string and hash it into a 128-bit quantity. Do we need to hash again? In other words, could we just use the 128-bit GID directly and use that as the key? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We talked about this today and one optimization to make is to rely on FNA-1a to store the hash of the liveliness string as a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Yadunund After taking a closer look at this, things are more complicated than they seem. FNV only outperforms XXH3 for very small keys (see https://github.com/Cyan4973/xxHash/wiki/Performance-comparison#benchmarks-concentrating-on-small-data). So it's better to simply move to a 64-bit version of XXH3, as long as we agree that GID can be 8 bytes instead of 16. What do you think? |
||
|
||
uint64_t hash = FNV_OFFSET_BASIS_64; | ||
for (const uint8_t & byte : gid) { | ||
hash ^= byte; | ||
hash *= FNV_PRIME_64; | ||
} | ||
return static_cast<std::size_t>(hash); | ||
} | ||
}; | ||
} // namespace std | ||
|
||
#endif // DETAIL__LIVELINESS_UTILS_HPP_ |
Uh oh!
There was an error while loading. Please reload this page.