Faster bucket search in ByteBufferHashTable

[puzpuzpuz]

Description

Adds hash code comparison for large enough keys to ByteBufferHashTable#findBucket(). Also, changes key comparison to use long/int/byte instead of byte-only comparison (thus, the comparison is now closer to HashTableUtils#memoryEquals() used in MemoryOpenHashTable). These changes are aimed to speed-up bucket search in ByteBufferHashTable, especially in high-collision cases.

Microbenchmarks

Environment: Ryzen 7900x, Ubuntu 24.04, OpenJDK 64-bit 17.0.17

Before:

Benchmark                                (keySize)  Mode  Cnt   Score   Error  Units
ByteBufferHashTableBenchmark.findBucket          8  avgt    5  19.965 ± 0.212  ns/op
ByteBufferHashTableBenchmark.findBucket         16  avgt    5  26.816 ± 1.282  ns/op
ByteBufferHashTableBenchmark.findBucket         32  avgt    5  36.174 ± 0.337  ns/op
ByteBufferHashTableBenchmark.findBucket         64  avgt    5  49.581 ± 0.482  ns/op
ByteBufferHashTableBenchmark.findBucket        128  avgt    5  72.990 ± 1.429  ns/op

After:

Benchmark                                (keySize)  Mode  Cnt   Score   Error  Units
ByteBufferHashTableBenchmark.findBucket          8  avgt    5   5.502 ± 0.338  ns/op
ByteBufferHashTableBenchmark.findBucket         16  avgt    5  11.830 ± 0.046  ns/op
ByteBufferHashTableBenchmark.findBucket         32  avgt    5  15.965 ± 0.135  ns/op
ByteBufferHashTableBenchmark.findBucket         64  avgt    5  20.522 ± 0.069  ns/op
ByteBufferHashTableBenchmark.findBucket        128  avgt    5  29.035 ± 1.806  ns/op

Release note

Speed-up bucket search in hash table used by GROUP BY

---

Key changed/added classes in this PR

• ByteBufferHashTable

---

This PR has:

• been self-reviewed.
  • using the concurrency checklist (Remove this item if the PR doesn't have any relation to concurrency.)
• added documentation for new or modified features or behaviors.
• a release note entry in the PR description.
• added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
• added or updated version, license, or notice information in licenses.yaml
• added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
• added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
• added integration tests.
• been tested in a test Druid cluster.

[jtuglu1]

Thanks! Can we include a performance test (see benchmarks folder)?

[puzpuzpuz]

Thanks! Can we include a performance test (see benchmarks folder)?

@jtuglu1 done in 85869a8 - the measurements on my box are in the benchmark description. I've also made hash code check mandatory (previously it was disabled for keys <= 8 bytes).

[puzpuzpuz]

Sorry, I accidentally broke the empty bucket check with the earlier commit - fixed it in 2d9520f. Also updated the benchmark results with the measurements obtained on the latest commit.

[puzpuzpuz]

More updates. I've noticed that the change introduced implicit endianess dependency since it now checks an int for the empty bit instead of the previous single byte check. This should be fixed in 952fab3 and 7ea3339. No more changes from my side.

[gianm]

The changes look good to me. Thank you for including a benchmark as well.

[jtuglu1]

Thank you! Left 2 small, non-blocking comments.

[jtuglu1]

nit: while we're here, can we name this mask? It's used in other places below (byte-level mask) and makes it easier to read potentially.

[puzpuzpuz]

Addressed in 5fb014f

[jtuglu1]

Maybe we can save a comparison by switching to a do/while loop since I believe length will always be ≥ 8. This likely will not show up in the benchmark, however. Unfortunately we cannot do something like [[likely]] in Java I don't think.

[puzpuzpuz]

I'd rather keep the method correct in the face of smaller keys, if this ever changes in the future. Also, if the keys are always >=8, the first branch will be always taken, so CPU's branch predictor should make it very cheap.

[puzpuzpuz]

@jtuglu1 @gianm thanks for the reviews!

[FrankChen021]

@puzpuzpuz Thanks for the change. I'm wondering how much does this change improve(like the CPU usage) in a real cluster?

[puzpuzpuz]

@puzpuzpuz Thanks for the change. I'm wondering how much does this change improve(like the CPU usage) in a real cluster?

This is a small change and unlikely it's a significant bottleneck in typical workloads, but I'm guessing here. BTW are there any public benchmarks in which Druid actively participates? If so, it's a good idea to check those to be able to make more educated optimizations (if any required/possible).