From 4fdedf298bfefc97af2d3a7416c6d190a06a1971 Mon Sep 17 00:00:00 2001 From: Graham Date: Tue, 1 Sep 2020 20:28:30 +0100 Subject: [PATCH] Add support for colliding name hashes to Js5Index Annoyingly, the 876 and latest RuneScape 3 cache both have collisions. Signed-off-by: Graham --- .../dev/openrs2/cache/NamedEntryCollection.kt | 69 +++++--- .../openrs2/cache/NamedEntryCollectionTest.kt | 155 +++++++++++++++++- 2 files changed, 192 insertions(+), 32 deletions(-) diff --git a/cache/src/main/java/dev/openrs2/cache/NamedEntryCollection.kt b/cache/src/main/java/dev/openrs2/cache/NamedEntryCollection.kt index 15b29f72..d1cbcde5 100644 --- a/cache/src/main/java/dev/openrs2/cache/NamedEntryCollection.kt +++ b/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, - * and it throws an exception if it detects a collision. This differs from the - * client's implementation, which does not throw an exception and allows the - * colliding entry with the lowest ID to be referred to by the name. + * This implementation permits multiple entries to have the same name hash. In + * the event of a collision, the colliding entry with the lowest ID is returned + * from methods that look up an entry by its name hash. This implementation + * 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( private val entryConstructor: (NamedEntryCollection, Int) -> T ) : MutableIterable { private var singleEntry: T? = null private var entries: Int2ObjectAVLTreeMap? = null - private var nameHashTable: Int2IntMap? = null + + // XXX(gpe): unfortunately fastutil doesn't have a multimap type + private var nameHashTable: Int2ObjectMap? = null public val size: Int get() { @@ -116,9 +126,9 @@ public abstract class NamedEntryCollection( } val nameHashTable = nameHashTable ?: return null - val id = nameHashTable[nameHash] - return if (id != -1) { - get(id) ?: throw IllegalStateException() + val ids = nameHashTable.getOrDefault(nameHash, IntSortedSets.EMPTY_SET) + return if (ids.isNotEmpty()) { + get(ids.firstInt()) ?: throw IllegalStateException() } else { null } @@ -150,9 +160,8 @@ public abstract class NamedEntryCollection( entries[singleEntry.id] = singleEntry if (singleEntry.nameHash != -1) { - val nameHashTable = Int2IntOpenHashMap() - nameHashTable.defaultReturnValue(-1) - nameHashTable[singleEntry.nameHash] = singleEntry.id + val nameHashTable = Int2ObjectOpenHashMap() + nameHashTable[singleEntry.nameHash] = IntSortedSets.singleton(singleEntry.id) this.nameHashTable = nameHashTable } } @@ -224,18 +233,31 @@ public abstract class NamedEntryCollection( 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.defaultReturnValue(-1) + nameHashTable = Int2ObjectOpenHashMap() } - val prevId = nameHashTable.put(newNameHash, id) - check(prevId == -1) { - "Name hash collision: $newNameHash is already used by entry $prevId" + val set = nameHashTable[newNameHash] + when { + 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( return } - val nameHashTable = nameHashTable ?: return - if (entry.nameHash != -1) { - nameHashTable.remove(entry.nameHash) - - if (nameHashTable.isEmpty()) { - this.nameHashTable = null - } - } + rename(entry.id, entry.nameHash, -1) } override fun iterator(): MutableIterator { diff --git a/cache/src/test/java/dev/openrs2/cache/NamedEntryCollectionTest.kt b/cache/src/test/java/dev/openrs2/cache/NamedEntryCollectionTest.kt index f4d2fb26..ed4d8a3b 100644 --- a/cache/src/test/java/dev/openrs2/cache/NamedEntryCollectionTest.kt +++ b/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) - assertThrows { - entry.setName("hello") - } + 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.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"]) } }