Add support for colliding name hashes to Js5Index

Annoyingly, the 876 and latest RuneScape 3 cache both have collisions.

Signed-off-by: Graham <gpe@openrs2.dev>

cache/src/main/java/dev/openrs2/cache/NamedEntryCollection.kt

@@ -1,9 +1,12 @@
 package dev.openrs2.cache
 
 import dev.openrs2.util.krHashCode
-import it.unimi.dsi.fastutil.ints.Int2IntMap
-import it.unimi.dsi.fastutil.ints.Int2IntOpenHashMap
 import it.unimi.dsi.fastutil.ints.Int2ObjectAVLTreeMap
+import it.unimi.dsi.fastutil.ints.Int2ObjectMap
+import it.unimi.dsi.fastutil.ints.Int2ObjectOpenHashMap
+import it.unimi.dsi.fastutil.ints.IntAVLTreeSet
+import it.unimi.dsi.fastutil.ints.IntSortedSet
+import it.unimi.dsi.fastutil.ints.IntSortedSets
 
 /**
  * A specialist collection type entirely designed for use by the [Js5Index]
@@ -34,17 +37,24 @@ import it.unimi.dsi.fastutil.ints.Int2ObjectAVLTreeMap
  * none at all, but allowing a mixture in this implementation makes mutating
  * the index simpler.
  *
- * This implementation does not allow multiple entries with the same name hash,
+ * This implementation permits multiple entries to have the same name hash. In
- * and it throws an exception if it detects a collision. This differs from the
+ * the event of a collision, the colliding entry with the lowest ID is returned
- * client's implementation, which does not throw an exception and allows the
+ * from methods that look up an entry by its name hash. This implementation
- * colliding entry with the lowest ID to be referred to by the name.
+ * still behaves correctly if one of the colliding entries is removed: it will
+ * always return the entry with the next lowest ID.
+ *
+ * This is an edge case that is unlikely to be hit in practice: the 550 cache
+ * has no collisions, and the 876 cache only has a single collision (the hash
+ * for the empty string).
  */
 public abstract class NamedEntryCollection<T : NamedEntry>(
     private val entryConstructor: (NamedEntryCollection<T>, Int) -> T
 ) : MutableIterable<T> {
     private var singleEntry: T? = null
     private var entries: Int2ObjectAVLTreeMap<T>? = null
-    private var nameHashTable: Int2IntMap? = null
+
+    // XXX(gpe): unfortunately fastutil doesn't have a multimap type
+    private var nameHashTable: Int2ObjectMap<IntSortedSet>? = null
 
     public val size: Int
         get() {
@@ -116,9 +126,9 @@ public abstract class NamedEntryCollection<T : NamedEntry>(
         }
 
         val nameHashTable = nameHashTable ?: return null
-        val id = nameHashTable[nameHash]
+        val ids = nameHashTable.getOrDefault(nameHash, IntSortedSets.EMPTY_SET)
-        return if (id != -1) {
+        return if (ids.isNotEmpty()) {
-            get(id) ?: throw IllegalStateException()
+            get(ids.firstInt()) ?: throw IllegalStateException()
         } else {
             null
         }
@@ -150,9 +160,8 @@ public abstract class NamedEntryCollection<T : NamedEntry>(
                 entries[singleEntry.id] = singleEntry
 
                 if (singleEntry.nameHash != -1) {
-                    val nameHashTable = Int2IntOpenHashMap()
+                    val nameHashTable = Int2ObjectOpenHashMap<IntSortedSet>()
-                    nameHashTable.defaultReturnValue(-1)
+                    nameHashTable[singleEntry.nameHash] = IntSortedSets.singleton(singleEntry.id)
-                    nameHashTable[singleEntry.nameHash] = singleEntry.id
                     this.nameHashTable = nameHashTable
                 }
             }
@@ -224,18 +233,31 @@ public abstract class NamedEntryCollection<T : NamedEntry>(
 
         var nameHashTable = nameHashTable
         if (nameHashTable != null && prevNameHash != -1) {
-            nameHashTable.remove(prevNameHash)
+            val set = nameHashTable.get(prevNameHash)
+            check(set != null && set.contains(id))
+
+            if (set.size > 1) {
+                set.remove(id)
+            } else {
+                nameHashTable.remove(prevNameHash)
+            }
         }
 
         if (newNameHash != -1) {
             if (nameHashTable == null) {
-                nameHashTable = Int2IntOpenHashMap()
+                nameHashTable = Int2ObjectOpenHashMap()
-                nameHashTable.defaultReturnValue(-1)
             }
 
-            val prevId = nameHashTable.put(newNameHash, id)
+            val set = nameHashTable[newNameHash]
-            check(prevId == -1) {
+            when {
-                "Name hash collision: $newNameHash is already used by entry $prevId"
+                set == null -> nameHashTable[newNameHash] = IntSortedSets.singleton(id)
+                set.size == 1 -> {
+                    val newSet = IntAVLTreeSet()
+                    newSet.add(set.firstInt())
+                    newSet.add(id)
+                    nameHashTable[newNameHash] = newSet
+                }
+                else -> set.add(id)
             }
 
             this.nameHashTable = nameHashTable
@@ -262,14 +284,7 @@ public abstract class NamedEntryCollection<T : NamedEntry>(
             return
         }
 
-        val nameHashTable = nameHashTable ?: return
+        rename(entry.id, entry.nameHash, -1)
-        if (entry.nameHash != -1) {
-            nameHashTable.remove(entry.nameHash)
-
-            if (nameHashTable.isEmpty()) {
-                this.nameHashTable = null
-            }
-        }
     }
 
     override fun iterator(): MutableIterator<T> {

cache/src/test/java/dev/openrs2/cache/NamedEntryCollectionTest.kt

@@ -736,11 +736,156 @@ object NamedEntryCollectionTest {
     @Test
     fun testNameHashCollision() {
         val collection = TestCollection()
-        collection.createOrGet("hello")
 
-        val entry = collection.createOrGet(1)
+        val entry0 = collection.createOrGet(0)
-        assertThrows<IllegalStateException> {
+        val entry1 = collection.createOrGet(1)
-            entry.setName("hello")
+        val entry2 = collection.createOrGet(2)
-        }
+
+        entry0.setName("hello")
+        entry1.setName("hello")
+        entry2.setName("hello")
+
+        assertEquals(entry0, collection["hello"])
+
+        entry0.clearName()
+        assertEquals(entry1, collection["hello"])
+
+        entry1.clearName()
+        assertEquals(entry2, collection["hello"])
+
+        entry2.clearName()
+        assertNull(collection["hello"])
+    }
+
+    @Test
+    fun testNameHashCollisionOppositeOrder() {
+        val collection = TestCollection()
+
+        val entry0 = collection.createOrGet(0)
+        val entry1 = collection.createOrGet(1)
+        val entry2 = collection.createOrGet(2)
+
+        entry2.setName("hello")
+        entry1.setName("hello")
+        entry0.setName("hello")
+
+        assertEquals(entry0, collection["hello"])
+
+        entry2.clearName()
+        assertEquals(entry0, collection["hello"])
+
+        entry1.clearName()
+        assertEquals(entry0, collection["hello"])
+
+        entry0.clearName()
+        assertNull(collection["hello"])
+    }
+
+    @Test
+    fun testNameHashCollisionRemove() {
+        val collection = TestCollection()
+
+        val entry0 = collection.createOrGet(0)
+        val entry1 = collection.createOrGet(1)
+        val entry2 = collection.createOrGet(2)
+
+        entry0.setName("hello")
+        entry1.setName("hello")
+        entry2.setName("hello")
+
+        assertEquals(entry0, collection["hello"])
+
+        entry0.remove()
+        assertEquals(entry1, collection["hello"])
+
+        entry1.remove()
+        assertEquals(entry2, collection["hello"])
+
+        entry2.remove()
+        assertNull(collection["hello"])
+    }
+
+    @Test
+    fun testNameHashCollisionOppositeOrderRemove() {
+        val collection = TestCollection()
+
+        val entry0 = collection.createOrGet(0)
+        val entry1 = collection.createOrGet(1)
+        val entry2 = collection.createOrGet(2)
+
+        entry2.setName("hello")
+        entry1.setName("hello")
+        entry0.setName("hello")
+
+        assertEquals(entry0, collection["hello"])
+
+        entry2.remove()
+        assertEquals(entry0, collection["hello"])
+
+        entry1.remove()
+        assertEquals(entry0, collection["hello"])
+
+        entry0.remove()
+        assertNull(collection["hello"])
+    }
+
+    @Test
+    fun testNameHashCollisionRename() {
+        val collection = TestCollection()
+
+        val entry0 = collection.createOrGet(0)
+        val entry1 = collection.createOrGet(1)
+        val entry2 = collection.createOrGet(2)
+
+        entry0.setName("hello")
+        entry1.setName("hello")
+        entry2.setName("hello")
+
+        assertEquals(entry0, collection["hello"])
+
+        entry0.setName("entry0")
+        assertEquals(entry0, collection["entry0"])
+        assertEquals(entry1, collection["hello"])
+
+        entry1.setName("entry1")
+        assertEquals(entry0, collection["entry0"])
+        assertEquals(entry1, collection["entry1"])
+        assertEquals(entry2, collection["hello"])
+
+        entry2.setName("entry2")
+        assertEquals(entry0, collection["entry0"])
+        assertEquals(entry1, collection["entry1"])
+        assertEquals(entry2, collection["entry2"])
+        assertNull(collection["hello"])
+    }
+
+    @Test
+    fun testNameHashCollisionOppositeOrderRename() {
+        val collection = TestCollection()
+
+        val entry0 = collection.createOrGet(0)
+        val entry1 = collection.createOrGet(1)
+        val entry2 = collection.createOrGet(2)
+
+        entry2.setName("hello")
+        entry1.setName("hello")
+        entry0.setName("hello")
+
+        assertEquals(entry0, collection["hello"])
+
+        entry2.setName("entry2")
+        assertEquals(entry2, collection["entry2"])
+        assertEquals(entry0, collection["hello"])
+
+        entry1.setName("entry1")
+        assertEquals(entry1, collection["entry1"])
+        assertEquals(entry2, collection["entry2"])
+        assertEquals(entry0, collection["hello"])
+
+        entry0.setName("entry0")
+        assertEquals(entry0, collection["entry0"])
+        assertEquals(entry1, collection["entry1"])
+        assertEquals(entry2, collection["entry2"])
+        assertNull(collection["hello"])
     }
 }