Re: [PR] HBASE-29889 Add XXH3 Hash Support to Bloom Filter using hash4j [hbase]
jinhyukify commented on PR #7782: URL: https://github.com/apache/hbase/pull/7782#issuecomment-4322270433 All failed test passed in my local. - `TestBufferedMutator.testMultiThread` - `TestPrefetchWithBucketCache.testPrefetchRunTriggersEvictions` - `TestZooKeeper.*` - `TestFromClientSide3.*` - `TestHRegionWithInMemoryFlush.*` - `TestEditsBehindDroppedTableTiming.*` -- 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]
Re: [PR] HBASE-29889 Add XXH3 Hash Support to Bloom Filter using hash4j [hbase]
jinhyukify commented on code in PR #7782:
URL: https://github.com/apache/hbase/pull/7782#discussion_r3040161760
##
hbase-server/src/main/java/org/apache/hadoop/hbase/util/BloomFilterUtil.java:
##
@@ -278,4 +279,31 @@ public static byte[] getBloomFilterParam(BloomType
bloomFilterType, Configuratio
}
return bloomParam;
}
+
+ /**
+ * Generate the two hash values needed for Bloom filter index generation.
Bloom filters require a
+ * (hash1, hash2) pair to derive multiple probe locations.
+ *
+ * If the hash implementation provides a 64-bit hash, we split the
64-bit value into two
+ * 32-bit hashes to avoid extra hashing cost.
+ * Otherwise, fall back to classic double hashing by rehashing with
hash1 as the seed.
+ *
+ * @param hash the hash function
+ * @param key the hash key
+ * @return a pair of hash values (hash1, hash2)
+ */
+ public static Pair getHashPair(Hash hash, HashKey key) {
Review Comment:
Good call. Done in 2164f5d313bb11cc11ceada76f27eefde3d506b5
--
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]
Re: [PR] HBASE-29889 Add XXH3 Hash Support to Bloom Filter using hash4j [hbase]
jinhyukify commented on PR #7782: URL: https://github.com/apache/hbase/pull/7782#issuecomment-4215399344 ``` GH:7782 does not apply to master. Rebase required? Wrong Branch? See https://yetus.apache.org/documentation/in-pr ogress/precommit-patchnames for help. ``` Let me rebase onto master once. -- 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]
Re: [PR] HBASE-29889 Add XXH3 Hash Support to Bloom Filter using hash4j [hbase]
jinhyukify commented on code in PR #7782:
URL: https://github.com/apache/hbase/pull/7782#discussion_r3040161760
##
hbase-server/src/main/java/org/apache/hadoop/hbase/util/BloomFilterUtil.java:
##
@@ -278,4 +279,31 @@ public static byte[] getBloomFilterParam(BloomType
bloomFilterType, Configuratio
}
return bloomParam;
}
+
+ /**
+ * Generate the two hash values needed for Bloom filter index generation.
Bloom filters require a
+ * (hash1, hash2) pair to derive multiple probe locations.
+ *
+ * If the hash implementation provides a 64-bit hash, we split the
64-bit value into two
+ * 32-bit hashes to avoid extra hashing cost.
+ * Otherwise, fall back to classic double hashing by rehashing with
hash1 as the seed.
+ *
+ * @param hash the hash function
+ * @param key the hash key
+ * @return a pair of hash values (hash1, hash2)
+ */
+ public static Pair getHashPair(Hash hash, HashKey key) {
Review Comment:
Good call. Done in 870c1fa5fbbed079a55a70dc5e328d0d167a22be
--
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]
Re: [PR] HBASE-29889 Add XXH3 Hash Support to Bloom Filter using hash4j [hbase]
Apache9 commented on code in PR #7782:
URL: https://github.com/apache/hbase/pull/7782#discussion_r3036942185
##
hbase-server/src/main/java/org/apache/hadoop/hbase/util/BloomFilterUtil.java:
##
@@ -278,4 +279,31 @@ public static byte[] getBloomFilterParam(BloomType
bloomFilterType, Configuratio
}
return bloomParam;
}
+
+ /**
+ * Generate the two hash values needed for Bloom filter index generation.
Bloom filters require a
+ * (hash1, hash2) pair to derive multiple probe locations.
+ *
+ * If the hash implementation provides a 64-bit hash, we split the
64-bit value into two
+ * 32-bit hashes to avoid extra hashing cost.
+ * Otherwise, fall back to classic double hashing by rehashing with
hash1 as the seed.
+ *
+ * @param hash the hash function
+ * @param key the hash key
+ * @return a pair of hash values (hash1, hash2)
+ */
+ public static Pair getHashPair(Hash hash, HashKey key) {
Review Comment:
Let's just return long here, and let upper layer do some shift magic to get
the two ints? I believe hashing is compute sensitive, so let's try to avoid
unnecessary boxing/unboxing operations.
##
hbase-server/src/main/java/org/apache/hadoop/hbase/util/BloomFilterUtil.java:
##
@@ -278,4 +279,31 @@ public static byte[] getBloomFilterParam(BloomType
bloomFilterType, Configuratio
}
return bloomParam;
}
+
+ /**
+ * Generate the two hash values needed for Bloom filter index generation.
Bloom filters require a
+ * (hash1, hash2) pair to derive multiple probe locations.
+ *
+ * If the hash implementation provides a 64-bit hash, we split the
64-bit value into two
+ * 32-bit hashes to avoid extra hashing cost.
+ * Otherwise, fall back to classic double hashing by rehashing with
hash1 as the seed.
+ *
+ * @param hash the hash function
+ * @param key the hash key
+ * @return a pair of hash values (hash1, hash2)
+ */
+ public static Pair getHashPair(Hash hash, HashKey key) {
+if (hash instanceof Hash64) {
Review Comment:
Is it possible to avoid instanceof here? Let Hash instance returns if it
supports 64 bits hash result?
--
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]
Re: [PR] HBASE-29889 Add XXH3 Hash Support to Bloom Filter using hash4j [hbase]
jinhyukify commented on PR #7782: URL: https://github.com/apache/hbase/pull/7782#issuecomment-4052161429 As I mentioned in the [original PR](https://github.com/apache/hbase/pull/7740#issuecomment-4052158574), I applied the same optimization to the existing hash implementations to read 4 bytes at once instead of assembling integers byte-by-byte. This optimization also brings noticeable performance improvements. The benchmark results are summarized below. For easier comparison, I also attached graphs in the Jira comment. Please take a look. 🙇♂️ | Length | Jenkins | **Jenkins After** | Δ% | Murmur | **Murmur After** | Δ% | Murmur3 | **Murmur3 After** | Δ% | |---:|---:|---:|---:|---:|---:|---:|---:|---:|---:| | 3 | 377M | **381M** | +1.2% | 598M | **599M** | +0.3% | 622M | **619M** | -0.5% | | 8 | 323M | **330M** | +2.2% | 315M | **417M** | +32.5% | 275M | **384M** | +39.6% | | 16 | 147M | **204M** | +39.0% | 246M | **339M** | +37.7% | 207M | **282M** | +35.8% | | 32 | 100M | **139M** | +38.5% | 145M | **208M** | +43.3% | 128M | **176M** | +37.2% | | 64 | 51M | **70M** | +38.2% | 85M | **113M** | +32.4% | 70M | **89M** | +28.0% | | 128 | 26M | **33M** | +27.7% | 44M | **56M** | +28.4% | 36M | **42M** | +15.9% | | 240 | 13M | **17M** | +33.6% | 23M | **29M** | +23.8% | 18M | **19M** | +8.0% | | 256 | 12M | **16M** | +31.4% | 22M | **27M** | +22.0% | 15M | **18M** | +23.6% | | 512 | 6M | **8M** | +26.5% | 10M | **12M** | +14.9% | 7M | **7M** | +2.0% | | 1024 | 3M | **4M** | +26.8% | 5M | **5M** | +10.5% | 3M | **4M** | +6.0% | | 2048 | 1M | **2M** | +27.3% | 2M | **2M** | +6.3% | 2M | **2M** | +5.5% | | 4096 | 709K | **934K** | +31.8% | 1M | **1M** | +10.1% | 802K | **833K** | +3.8% | | 16384 | 185K | **232K** | +25.6% | 254K | **262K** | +3.0% | 199K | **197K** | -0.8% | -- 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]
Re: [PR] HBASE-29889 Add XXH3 Hash Support to Bloom Filter using hash4j [hbase]
jinhyukify commented on code in PR #7782: URL: https://github.com/apache/hbase/pull/7782#discussion_r2840995708 ## hbase-shaded/pom.xml: ## @@ -537,6 +537,16 @@ keytab.txt + + +com.dynatrace.hash4j:hash4j + + META-INF/versions/25/** + + Review Comment: Got it. I’ve updated it here. https://github.com/apache/hbase/pull/7782/changes/6553589bcc138770f59babe2337d2cafb8ebbc0c -- 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]
Re: [PR] HBASE-29889 Add XXH3 Hash Support to Bloom Filter using hash4j [hbase]
Apache9 commented on code in PR #7782: URL: https://github.com/apache/hbase/pull/7782#discussion_r2839122149 ## hbase-shaded/pom.xml: ## @@ -537,6 +537,16 @@ keytab.txt + + +com.dynatrace.hash4j:hash4j + + META-INF/versions/25/** + + Review Comment: We can bump the shade plugin version, altough we only test on JDK17 official, as far as I know lots of our users have already run HBase with JDK21, so maybe there are users want to run HBase with JDK25. -- 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]
Re: [PR] HBASE-29889 Add XXH3 Hash Support to Bloom Filter using hash4j [hbase]
jinhyukify commented on PR #7782: URL: https://github.com/apache/hbase/pull/7782#issuecomment-3938643145 CI Failures on large-wave-1, large-wave-3 - `org.apache.hadoop.hbase.tool.TestCanaryTool.testWebUI` - `org.apache.hadoop.hbase.regionserver.TestClearRegionBlockCache.testClearBlockCache` - `org.apache.hadoop.hbase.regionserver.TestTags.testFlushAndCompactionwithCombinations` All tests pass on my local. -- 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]
Re: [PR] HBASE-29889 Add XXH3 Hash Support to Bloom Filter using hash4j [hbase]
jinhyukify commented on code in PR #7782: URL: https://github.com/apache/hbase/pull/7782#discussion_r2835954359 ## hbase-shaded/pom.xml: ## @@ -537,6 +537,16 @@ keytab.txt + + +com.dynatrace.hash4j:hash4j + + META-INF/versions/25/** + + Review Comment: Another possible fix is upgrading the `maven-shade-plugin` from 3.6.0 to 3.6.1, since ASM starts supporting Java 25 in 3.6.1. However, we’re not ready to adopt this jdk version. ## hbase-common/src/main/java/org/apache/hadoop/hbase/util/XXH3.java: ## @@ -0,0 +1,55 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.hadoop.hbase.util; + +import com.dynatrace.hash4j.hashing.Hasher64; +import com.dynatrace.hash4j.hashing.Hashing; +import org.apache.yetus.audience.InterfaceAudience; +import org.apache.yetus.audience.InterfaceStability; + +/** + * XXH3 64-bit hash implementation based on hash4j library. + * + * Provides high throughput, strong dispersion, and minimal memory usage, optimized for modern CPU + * architectures. + * @see https://github.com/Cyan4973/xxHash/blob/dev/doc/xxhash_spec.md#xxh3-algorithm-overview";>XXH3 + * Algorithm + */ [email protected] [email protected] +public class XXH3 extends Hash implements Hash64 { + private static final XXH3 _instance = new XXH3(); + + public static Hash getInstance() { +return _instance; + } + + @Override + public long hash64(HashKey hashKey, long seed) { +Hasher64 hasher = seed == 0L ? Hashing.xxh3_64() : Hashing.xxh3_64(seed); +return hasher.hashBytesToLong(hashKey, 0, hashKey.length(), HashKeyByteAccess.INSTANCE); Review Comment: I think tests for the hash function itself are outside the scope of this change. The actual verification was done separately, and all hash functions behave as expected. The test did on https://github.com/jinhyukify/hbase/commit/ed1c82119d9151cf73bd3dab67a3060fb06cce82 ## pom.xml: ## @@ -983,6 +983,8 @@ 1.10.1 1.1.10.4 1.5.7-2 + +0.29.0 Review Comment: Not fully sure if I should add this dependency here ## hbase-shaded/pom.xml: ## @@ -537,6 +537,16 @@ keytab.txt + + +com.dynatrace.hash4j:hash4j + + META-INF/versions/25/** + + Review Comment: fyi: we get the following errors without this ``` .m2/repository/com/dynatrace/hash4j/hash4j/0.29.0/hash4j-0.29.0.jar entry META-INF/versions/25/com/dynatrace/hash4j/hashing/FFMUtil$MemorySegmentByteAccess.class: java.lang.IllegalArgumentException: Unsupported class file major version 69 ``` -- 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]
