From c64f2161509fa92833fdeb646ab3c88310150ba8 Mon Sep 17 00:00:00 2001 From: Enrico Olivelli Date: Sun, 7 Dec 2025 17:47:12 +0100 Subject: [PATCH] CNDB-16243: Improve ChunkCache Key in order to keep O(1) access time Improve hashCode of the Key to prevent collisions Implement Comparable in case the ConcurrentHashMap falls back in to Tree mode --- .../apache/cassandra/cache/ChunkCache.java | 31 +- .../cassandra/cache/ChunkCacheKeyTest.java | 438 ++++++++++++++++++ 2 files changed, 461 insertions(+), 8 deletions(-) create mode 100644 test/unit/org/apache/cassandra/cache/ChunkCacheKeyTest.java diff --git a/src/java/org/apache/cassandra/cache/ChunkCache.java b/src/java/org/apache/cassandra/cache/ChunkCache.java index 6def74cc21b2..66d71f8510a5 100644 --- a/src/java/org/apache/cassandra/cache/ChunkCache.java +++ b/src/java/org/apache/cassandra/cache/ChunkCache.java @@ -297,12 +297,12 @@ public void invalidateFileNow(File file) synchronousCache.invalidateAll(Iterables.filter(cache.asMap().keySet(), x -> (x.readerId & mask) == fileId)); } - static class Key + static class Key implements Comparable { final long readerId; final long position; - private Key(long readerId, long position) + Key(long readerId, long position) { super(); this.position = position; @@ -312,11 +312,15 @@ private Key(long readerId, long position) @Override public int hashCode() { - final int prime = 31; - int result = 1; - result = prime * result + Long.hashCode(readerId); - result = prime * result + Long.hashCode(position); - return result; + // Mix readerId and position into a single long using a large prime multiplier + // This constant is a mixing constant derived from the Golden Ratio + long mixed = (Long.rotateLeft(readerId, 16) + Long.rotateLeft(position, 16)) * 0x9E3779B97F4A7C15L; + + // Spread the bits (XOR-shift) to ensure high bits affect low bits + mixed ^= (mixed >>> 32); + mixed ^= (mixed >>> 16); + + return (int) mixed; } @Override @@ -324,13 +328,24 @@ public boolean equals(Object obj) { if (this == obj) return true; - if (obj == null) + if (obj == null || getClass() != obj.getClass()) return false; Key other = (Key) obj; return (position == other.position) && readerId == other.readerId; } + + @Override + public int compareTo(Key other) { + // Compare readerId first + int cmp = Long.compare(this.readerId, other.readerId); + if (cmp != 0) { + return cmp; + } + // Then compare position + return Long.compare(this.position, other.position); + } } /** diff --git a/test/unit/org/apache/cassandra/cache/ChunkCacheKeyTest.java b/test/unit/org/apache/cassandra/cache/ChunkCacheKeyTest.java new file mode 100644 index 000000000000..11436ad2b948 --- /dev/null +++ b/test/unit/org/apache/cassandra/cache/ChunkCacheKeyTest.java @@ -0,0 +1,438 @@ +/* + * 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.cassandra.cache; + +import java.lang.reflect.Constructor; +import java.util.Arrays; +import java.util.HashMap; +import java.util.HashSet; +import java.util.Set; +import java.util.TreeSet; + +import org.junit.Test; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotEquals; +import static org.junit.Assert.assertTrue; + +/** + * Unit tests for ChunkCache.Key class + */ +public class ChunkCacheKeyTest +{ + /** + * Helper method to create a Key instance using reflection since the constructor is package-private + */ + private ChunkCache.Key createKey(long readerId, long position) throws Exception + { + return new ChunkCache.Key(readerId, position); + } + + @Test + public void testKeyConstruction() throws Exception + { + ChunkCache.Key key = createKey(123L, 456L); + assertEquals(123L, key.readerId); + assertEquals(456L, key.position); + } + + @Test + public void testKeyConstructionWithZeroValues() throws Exception + { + ChunkCache.Key key = createKey(0L, 0L); + assertEquals(0L, key.readerId); + assertEquals(0L, key.position); + } + + @Test + public void testKeyConstructionWithNegativeValues() throws Exception + { + ChunkCache.Key key = createKey(-1L, -1L); + assertEquals(-1L, key.readerId); + assertEquals(-1L, key.position); + } + + @Test + public void testKeyConstructionWithMaxValues() throws Exception + { + ChunkCache.Key key = createKey(Long.MAX_VALUE, Long.MAX_VALUE); + assertEquals(Long.MAX_VALUE, key.readerId); + assertEquals(Long.MAX_VALUE, key.position); + } + + @Test + public void testKeyConstructionWithMinValues() throws Exception + { + ChunkCache.Key key = createKey(Long.MIN_VALUE, Long.MIN_VALUE); + assertEquals(Long.MIN_VALUE, key.readerId); + assertEquals(Long.MIN_VALUE, key.position); + } + + @Test + public void testEqualsReflexive() throws Exception + { + ChunkCache.Key key = createKey(100L, 200L); + assertTrue(key.equals(key)); + } + + @Test + public void testEqualsSymmetric() throws Exception + { + ChunkCache.Key key1 = createKey(100L, 200L); + ChunkCache.Key key2 = createKey(100L, 200L); + assertTrue(key1.equals(key2)); + assertTrue(key2.equals(key1)); + } + + @Test + public void testEqualsTransitive() throws Exception + { + ChunkCache.Key key1 = createKey(100L, 200L); + ChunkCache.Key key2 = createKey(100L, 200L); + ChunkCache.Key key3 = createKey(100L, 200L); + assertTrue(key1.equals(key2)); + assertTrue(key2.equals(key3)); + assertTrue(key1.equals(key3)); + } + + @Test + public void testEqualsConsistent() throws Exception + { + ChunkCache.Key key1 = createKey(100L, 200L); + ChunkCache.Key key2 = createKey(100L, 200L); + // Multiple invocations should return the same result + for (int i = 0; i < 10; i++) + { + assertTrue(key1.equals(key2)); + } + } + + @Test + public void testEqualsWithNull() throws Exception + { + ChunkCache.Key key = createKey(100L, 200L); + assertFalse(key.equals(null)); + } + + @Test + public void testEqualsWithDifferentReaderId() throws Exception + { + ChunkCache.Key key1 = createKey(100L, 200L); + ChunkCache.Key key2 = createKey(101L, 200L); + assertFalse(key1.equals(key2)); + assertFalse(key2.equals(key1)); + } + + @Test + public void testEqualsWithDifferentPosition() throws Exception + { + ChunkCache.Key key1 = createKey(100L, 200L); + ChunkCache.Key key2 = createKey(100L, 201L); + assertFalse(key1.equals(key2)); + assertFalse(key2.equals(key1)); + } + + @Test + public void testEqualsWithBothDifferent() throws Exception + { + ChunkCache.Key key1 = createKey(100L, 200L); + ChunkCache.Key key2 = createKey(101L, 201L); + assertFalse(key1.equals(key2)); + assertFalse(key2.equals(key1)); + } + + @Test + public void testHashCodeConsistency() throws Exception + { + ChunkCache.Key key = createKey(100L, 200L); + int hash1 = key.hashCode(); + int hash2 = key.hashCode(); + assertEquals(hash1, hash2); + } + + @Test + public void testHashCodeEqualityContract() throws Exception + { + ChunkCache.Key key1 = createKey(100L, 200L); + ChunkCache.Key key2 = createKey(100L, 200L); + // If two objects are equal, their hash codes must be equal + assertTrue(key1.equals(key2)); + assertEquals(key1.hashCode(), key2.hashCode()); + } + + @Test + public void testHashCodeDifferentKeys() throws Exception + { + ChunkCache.Key key1 = createKey(100L, 200L); + ChunkCache.Key key2 = createKey(101L, 200L); + ChunkCache.Key key3 = createKey(100L, 201L); + + // Different keys should ideally have different hash codes (though not guaranteed) + // We just verify they're computed without error + int hash1 = key1.hashCode(); + int hash2 = key2.hashCode(); + int hash3 = key3.hashCode(); + + // At least one should be different + assertTrue(hash1 != hash2 || hash1 != hash3); + } + + @Test + public void testHashCodeDistribution() throws Exception + { + // Test that hash codes are reasonably distributed + Set hashes = new HashSet<>(); + for (long i = 0; i < 1000; i++) + { + ChunkCache.Key key = createKey(i, i * 2); + hashes.add(key.hashCode()); + } + + // We should have a good distribution - at least 90% unique hashes + assertTrue("Hash distribution is poor: " + hashes.size() + " unique hashes out of 1000", + hashes.size() > 900); + } + + @Test + public void testHashCodeWithZeroValues() throws Exception + { + ChunkCache.Key key = createKey(0L, 0L); + // Should not throw and should produce a valid hash + int hash = key.hashCode(); + // Verify consistency + assertEquals(hash, key.hashCode()); + } + + @Test + public void testHashCodeWithMaxValues() throws Exception + { + ChunkCache.Key key = createKey(Long.MAX_VALUE, Long.MAX_VALUE); + int hash = key.hashCode(); + assertEquals(hash, key.hashCode()); + } + + @Test + public void testHashCodeWithMinValues() throws Exception + { + ChunkCache.Key key = createKey(Long.MIN_VALUE, Long.MIN_VALUE); + int hash = key.hashCode(); + assertEquals(hash, key.hashCode()); + } + + @Test + public void testCompareToReflexive() throws Exception + { + ChunkCache.Key key = createKey(100L, 200L); + assertEquals(0, key.compareTo(key)); + } + + @Test + public void testCompareToSymmetric() throws Exception + { + ChunkCache.Key key1 = createKey(100L, 200L); + ChunkCache.Key key2 = createKey(100L, 200L); + assertEquals(0, key1.compareTo(key2)); + assertEquals(0, key2.compareTo(key1)); + } + + @Test + public void testCompareToTransitive() throws Exception + { + ChunkCache.Key key1 = createKey(100L, 200L); + ChunkCache.Key key2 = createKey(200L, 300L); + ChunkCache.Key key3 = createKey(300L, 400L); + + assertTrue(key1.compareTo(key2) < 0); + assertTrue(key2.compareTo(key3) < 0); + assertTrue(key1.compareTo(key3) < 0); + } + + @Test + public void testCompareToConsistentWithEquals() throws Exception + { + ChunkCache.Key key1 = createKey(100L, 200L); + ChunkCache.Key key2 = createKey(100L, 200L); + + assertTrue(key1.equals(key2)); + assertEquals(0, key1.compareTo(key2)); + } + + @Test + public void testCompareToByReaderId() throws Exception + { + ChunkCache.Key key1 = createKey(100L, 200L); + ChunkCache.Key key2 = createKey(200L, 200L); + + assertTrue(key1.compareTo(key2) < 0); + assertTrue(key2.compareTo(key1) > 0); + } + + @Test + public void testCompareToByPosition() throws Exception + { + // When readerId is the same, position determines order + ChunkCache.Key key1 = createKey(100L, 200L); + ChunkCache.Key key2 = createKey(100L, 300L); + + assertTrue(key1.compareTo(key2) < 0); + assertTrue(key2.compareTo(key1) > 0); + } + + @Test + public void testCompareToReaderIdTakesPrecedence() throws Exception + { + // readerId comparison takes precedence over position + ChunkCache.Key key1 = createKey(100L, 500L); + ChunkCache.Key key2 = createKey(200L, 100L); + + // key1 < key2 because readerId 100 < 200, even though position 500 > 100 + assertTrue(key1.compareTo(key2) < 0); + assertTrue(key2.compareTo(key1) > 0); + } + + @Test + public void testCompareToWithZeroValues() throws Exception + { + ChunkCache.Key key1 = createKey(0L, 0L); + ChunkCache.Key key2 = createKey(0L, 0L); + assertEquals(0, key1.compareTo(key2)); + } + + @Test + public void testCompareToWithNegativeValues() throws Exception + { + ChunkCache.Key key1 = createKey(-100L, -200L); + ChunkCache.Key key2 = createKey(-50L, -100L); + + assertTrue(key1.compareTo(key2) < 0); + assertTrue(key2.compareTo(key1) > 0); + } + + @Test + public void testCompareToWithMaxValues() throws Exception + { + ChunkCache.Key key1 = createKey(Long.MAX_VALUE, Long.MAX_VALUE); + ChunkCache.Key key2 = createKey(Long.MAX_VALUE - 1, Long.MAX_VALUE); + + assertTrue(key1.compareTo(key2) > 0); + assertTrue(key2.compareTo(key1) < 0); + } + + @Test + public void testCompareToWithMinValues() throws Exception + { + ChunkCache.Key key1 = createKey(Long.MIN_VALUE, Long.MIN_VALUE); + ChunkCache.Key key2 = createKey(Long.MIN_VALUE + 1, Long.MIN_VALUE); + + assertTrue(key1.compareTo(key2) < 0); + assertTrue(key2.compareTo(key1) > 0); + } + + @Test + public void testCompareToOrdering() throws Exception + { + ChunkCache.Key[] keys = new ChunkCache.Key[]{ + createKey(300L, 100L), + createKey(100L, 300L), + createKey(200L, 200L), + createKey(100L, 100L), + createKey(100L, 200L) + }; + + Arrays.sort(keys); + + // Expected order after sorting: + // (100, 100), (100, 200), (100, 300), (200, 200), (300, 100) + assertEquals(100L, keys[0].readerId); + assertEquals(100L, keys[0].position); + + assertEquals(100L, keys[1].readerId); + assertEquals(200L, keys[1].position); + + assertEquals(100L, keys[2].readerId); + assertEquals(300L, keys[2].position); + + assertEquals(200L, keys[3].readerId); + assertEquals(200L, keys[3].position); + + assertEquals(300L, keys[4].readerId); + assertEquals(100L, keys[4].position); + } + + @Test + public void testKeyInHashMap() throws Exception + { + // Test that Key works correctly as a HashMap key + HashMap map = new HashMap<>(); + + ChunkCache.Key key1 = createKey(100L, 200L); + ChunkCache.Key key2 = createKey(100L, 200L); + ChunkCache.Key key3 = createKey(101L, 200L); + + map.put(key1, "value1"); + + // key2 should retrieve the same value as key1 since they're equal + assertEquals("value1", map.get(key2)); + + // key3 should not retrieve anything + assertEquals(null, map.get(key3)); + + // Adding with key2 should replace the value + map.put(key2, "value2"); + assertEquals(1, map.size()); + assertEquals("value2", map.get(key1)); + } + + @Test + public void testKeyInTreeSet() throws Exception + { + // Test that Key works correctly in a TreeSet (uses compareTo) + TreeSet set = new TreeSet<>(); + + ChunkCache.Key key1 = createKey(100L, 200L); + ChunkCache.Key key2 = createKey(100L, 200L); + ChunkCache.Key key3 = createKey(200L, 100L); + + set.add(key1); + set.add(key2); + set.add(key3); + + // Should only have 2 elements (key1 and key2 are equal) + assertEquals(2, set.size()); + assertTrue(set.contains(key1)); + assertTrue(set.contains(key2)); + assertTrue(set.contains(key3)); + } + + @Test + public void testEqualsDifferentTypes() throws Exception + { + ChunkCache.Key key = createKey(100L, 200L); + Object other = new Object(); + assertFalse(key.equals(other)); + } + + @Test + public void testEqualsWithString() throws Exception + { + ChunkCache.Key key = createKey(100L, 200L); + assertFalse(key.equals("not a key")); + } +}