shangxinli commented on code in PR #1019:
URL: https://github.com/apache/iceberg-cpp/pull/1019#discussion_r4284534837
##########
src/iceberg/util/murmurhash3_internal.cc:
##########
@@ -2,443 +2,75 @@
// MurmurHash3 was written by Austin Appleby, and is placed in the public
// domain. The author hereby disclaims copyright to this source code.
-// Note - The x86 and x64 versions do _not_ produce the same results, as the
-// algorithms are optimized for their respective platforms. You can still
-// compile and run any of them on any platform, but your performance with the
-// non-native version will be less than optimal.
-
#include "iceberg/util/murmurhash3_internal.h"
-//-----------------------------------------------------------------------------
-// Platform-specific functions and macros
-
-// Microsoft Visual Studio
-
-#if defined(_MSC_VER)
-
-# define FORCE_INLINE __forceinline
-
-# include <cstdlib>
-
-# define ROTL32(x, y) _rotl(x, y)
-# define ROTL64(x, y) _rotl64(x, y)
-
-# define BIG_CONSTANT(x) (x)
+#include <bit>
-// Other compilers
+#include "iceberg/util/endian.h"
-#else // defined(_MSC_VER)
-
-# define FORCE_INLINE inline __attribute__((always_inline))
-
-inline uint32_t rotl32(uint32_t x, int8_t r) { return (x << r) | (x >> (32 -
r)); }
-
-inline uint64_t rotl64(uint64_t x, int8_t r) { return (x << r) | (x >> (64 -
r)); }
-
-# define ROTL32(x, y) rotl32(x, y)
-# define ROTL64(x, y) rotl64(x, y)
-
-# define BIG_CONSTANT(x) (x##LLU)
-
-#endif // !defined(_MSC_VER)
-
-// NOLINTBEGIN
+// Adapted from the SMHasher MurmurHash3 reference implementation:
+//
https://github.com/aappleby/smhasher/blob/07bb4de10a63e8cc2e1724865454eba635742383/src/MurmurHash3.cpp
namespace iceberg {
namespace {
-//-----------------------------------------------------------------------------
-// Block read - if your platform needs to do endian-swapping or can only
-// handle aligned reads, do the conversion here
-FORCE_INLINE uint32_t getblock32(const uint32_t* p, int i) { return p[i]; }
-
-FORCE_INLINE uint64_t getblock64(const uint64_t* p, int i) { return p[i]; }
-
-//-----------------------------------------------------------------------------
-// Finalization mix - force all bits of a hash block to avalanche
-
-FORCE_INLINE uint32_t fmix32(uint32_t h) {
- h ^= h >> 16;
- h *= 0x85ebca6b;
- h ^= h >> 13;
- h *= 0xc2b2ae35;
- h ^= h >> 16;
-
- return h;
+uint32_t MixBlock(uint32_t block) {
+ constexpr uint32_t kC1 = 0xcc9e2d51;
+ constexpr uint32_t kC2 = 0x1b873593;
+ block *= kC1;
+ block = std::rotl(block, 15);
+ return block * kC2;
}
-//----------
-
-FORCE_INLINE uint64_t fmix64(uint64_t k) {
- k ^= k >> 33;
- k *= BIG_CONSTANT(0xff51afd7ed558ccd);
- k ^= k >> 33;
- k *= BIG_CONSTANT(0xc4ceb9fe1a85ec53);
- k ^= k >> 33;
+uint32_t MixHash(uint32_t hash, uint32_t block) {
+ hash ^= MixBlock(block);
+ return std::rotl(hash, 13) * 5 + 0xe6546b64;
+}
- return k;
+uint32_t Finalize(uint32_t hash, size_t len) {
+ hash ^= static_cast<uint32_t>(len);
+ hash ^= hash >> 16;
+ hash *= 0x85ebca6b;
+ hash ^= hash >> 13;
+ hash *= 0xc2b2ae35;
+ hash ^= hash >> 16;
+ return hash;
}
} // namespace
-//-----------------------------------------------------------------------------
-
-void MurmurHash3_x86_32(const void* key, int len, uint32_t seed, void* out) {
- const uint8_t* data = (const uint8_t*)key;
- const int nblocks = len / 4;
-
- uint32_t h1 = seed;
-
- const uint32_t c1 = 0xcc9e2d51;
- const uint32_t c2 = 0x1b873593;
-
- //----------
- // body
+uint32_t MurmurHash3_x86_32(const void* key, size_t len, uint32_t seed) {
+ // Process the full size_t length; only the final 32-bit length mix may
truncate it.
+ const auto* data = static_cast<const uint8_t*>(key);
+ const size_t nblocks = len / sizeof(uint32_t);
+ uint32_t hash = seed;
- const uint32_t* blocks = (const uint32_t*)(data + nblocks * 4);
-
- for (int i = -nblocks; i; i++) {
- uint32_t k1 = getblock32(blocks, i);
-
- k1 *= c1;
- k1 = ROTL32(k1, 15);
- k1 *= c2;
-
- h1 ^= k1;
- h1 = ROTL32(h1, 13);
- h1 = h1 * 5 + 0xe6546b64;
+ for (size_t i = 0; i < nblocks; ++i) {
+ hash = MixHash(hash, ReadLittleEndian<uint32_t>(data));
Review Comment:
Checked the new path against the SMHasher reference and it's bit-identical,
and _Long matches Guava's hashLong. No CI leg is big-endian though, so the fix
this PR is named for stays unverified - the new tests only have teeth on the
sanitizer job.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
