Conversation
|
We seem to have many uses of |
clalancette
left a comment
There was a problem hiding this comment.
I left a question inline.
| // 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; |
There was a problem hiding this comment.
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.
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 std::size_t here and send this size_t over any attachments as opposed to the 16B Gid.
There was a problem hiding this comment.
@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?
Yadunund
left a comment
There was a problem hiding this comment.
This is looking good to me. A couple minor nitpicks to address after which this is ready to merge.
Co-authored-by: yadunund <yadunund@gmail.com> Signed-off-by: Mahmoud Mazouz <mazouz.mahmoud@outlook.com>
hash_gidis called on Zenoh subscriber callbacks and shows up when profiling as a hot function. The use ofstd::stringstreamseems to be relatively inefficient. A significant portion ofhash_gid's running time is spend allocating & deallocating thestd::stringstream.