diff --git a/loader-common/src/main/java/org/cyclops/cyclopscore/helper/ItemStackHelpersCommon.java b/loader-common/src/main/java/org/cyclops/cyclopscore/helper/ItemStackHelpersCommon.java index b435594025d..13cceda9636 100644 --- a/loader-common/src/main/java/org/cyclops/cyclopscore/helper/ItemStackHelpersCommon.java +++ b/loader-common/src/main/java/org/cyclops/cyclopscore/helper/ItemStackHelpersCommon.java @@ -109,12 +109,24 @@ public int getItemStackHashCode(ItemStack stack) { int result = 1; result = 37 * result + stack.getCount(); result = 37 * result + stack.getItem().hashCode(); - // Tags can be very large, and expensive to calculate, which is not needed for hashCodes. - // CompoundTag tagCompound = stack.getTag(); - // result = 37 * result + (tagCompound != null ? tagCompound.hashCode() : 0); + if (hasComponentPatch(stack)) { + result = 37 * result + stack.getComponents().hashCode(); + } // Not factoring in capability compatibility. Doing so would require either reflection (slow) // or an access transformer, it's highly unlikely that it'd be the only difference between // many ItemStacks in practice, and occasional hash code collisions are okay. return result; } + + /** + * If the given stack carries data components that differ from its item's defaults. + * + * Only those have to take part in {@link #getItemStackHashCode(ItemStack)}. + * + * @param stack A non-empty stack. + * @return If the stack has a non-empty component patch. + */ + protected boolean hasComponentPatch(ItemStack stack) { + return !stack.getComponentsPatch().isEmpty(); + } } diff --git a/loader-neoforge/src/main/java/org/cyclops/cyclopscore/helper/ItemStackHelpersNeoForge.java b/loader-neoforge/src/main/java/org/cyclops/cyclopscore/helper/ItemStackHelpersNeoForge.java index cdeab05161e..7f4127e705a 100644 --- a/loader-neoforge/src/main/java/org/cyclops/cyclopscore/helper/ItemStackHelpersNeoForge.java +++ b/loader-neoforge/src/main/java/org/cyclops/cyclopscore/helper/ItemStackHelpersNeoForge.java @@ -10,4 +10,9 @@ public class ItemStackHelpersNeoForge extends ItemStackHelpersCommon { public ItemStack getCraftingRemainingItem(ItemStack itemStack) { return itemStack.getCraftingRemainder().create(); } + + @Override + protected boolean hasComponentPatch(ItemStack stack) { + return !stack.isComponentsPatchEmpty(); + } } diff --git a/loader-neoforge/src/main/java/org/cyclops/cyclopscore/ingredient/collection/IngredientCollectionSingleClassified.java b/loader-neoforge/src/main/java/org/cyclops/cyclopscore/ingredient/collection/IngredientCollectionSingleClassified.java index 9aab85895d5..a492598169b 100644 --- a/loader-neoforge/src/main/java/org/cyclops/cyclopscore/ingredient/collection/IngredientCollectionSingleClassified.java +++ b/loader-neoforge/src/main/java/org/cyclops/cyclopscore/ingredient/collection/IngredientCollectionSingleClassified.java @@ -176,13 +176,17 @@ public int count(T instance, M matchCondition) { } if (appliesToClassifier(matchCondition)) { IIngredientCollectionMutable collection = this.classifiedCollections.get(getClassifier(instance)); - if (collection != null) { - if (Objects.equals(getCategoryType().getMatchCondition(), matchCondition)) { - return collection.size(); - } else { - M subMatchCondition = getComponent().getMatcher().withoutCondition(matchCondition, getCategoryType().getMatchCondition()); - return collection.count(instance, subMatchCondition); - } + if (collection == null) { + // The match condition requires the classifier to be equal, so an absent classifier + // means nothing can match. Falling through to the unclassified path here would scan + // every instance for a result that is known to be zero. + return 0; + } + if (Objects.equals(getCategoryType().getMatchCondition(), matchCondition)) { + return collection.size(); + } else { + M subMatchCondition = getComponent().getMatcher().withoutCondition(matchCondition, getCategoryType().getMatchCondition()); + return collection.count(instance, subMatchCondition); } } return super.count(instance, matchCondition); diff --git a/loader-neoforge/src/main/java/org/cyclops/cyclopscore/ingredient/collection/IngredientMapSingleClassified.java b/loader-neoforge/src/main/java/org/cyclops/cyclopscore/ingredient/collection/IngredientMapSingleClassified.java index 31e51a8f941..f82e643e7ef 100644 --- a/loader-neoforge/src/main/java/org/cyclops/cyclopscore/ingredient/collection/IngredientMapSingleClassified.java +++ b/loader-neoforge/src/main/java/org/cyclops/cyclopscore/ingredient/collection/IngredientMapSingleClassified.java @@ -8,6 +8,7 @@ import javax.annotation.Nullable; import java.util.Collection; +import java.util.Collections; import java.util.Iterator; import java.util.List; import java.util.Map; @@ -121,13 +122,17 @@ public int size() { public boolean containsKey(T instance, M matchCondition) { if (appliesToClassifier(matchCondition)) { IIngredientMapMutable map = this.classifiedMaps.get(getClassifier(instance)); - if (map != null) { - if (Objects.equals(getCategoryType().getMatchCondition(), matchCondition)) { - return true; - } else { - M subMatchCondition = getComponent().getMatcher().withoutCondition(matchCondition, getCategoryType().getMatchCondition()); - return map.containsKey(instance, subMatchCondition); - } + if (map == null) { + // The match condition requires the classifier to be equal, so an absent classifier + // means nothing can match. Falling through to the unclassified path here would scan + // every key for a result that is known to be empty. + return false; + } + if (Objects.equals(getCategoryType().getMatchCondition(), matchCondition)) { + return true; + } else { + M subMatchCondition = getComponent().getMatcher().withoutCondition(matchCondition, getCategoryType().getMatchCondition()); + return map.containsKey(instance, subMatchCondition); } } return super.containsKey(instance, matchCondition); @@ -137,13 +142,14 @@ public boolean containsKey(T instance, M matchCondition) { public int countKey(T instance, M matchCondition) { if (appliesToClassifier(matchCondition)) { IIngredientMapMutable map = this.classifiedMaps.get(getClassifier(instance)); - if (map != null) { - if (Objects.equals(getCategoryType().getMatchCondition(), matchCondition)) { - return map.size(); - } else { - M subMatchCondition = getComponent().getMatcher().withoutCondition(matchCondition, getCategoryType().getMatchCondition()); - return map.countKey(instance, subMatchCondition); - } + if (map == null) { + return 0; + } + if (Objects.equals(getCategoryType().getMatchCondition(), matchCondition)) { + return map.size(); + } else { + M subMatchCondition = getComponent().getMatcher().withoutCondition(matchCondition, getCategoryType().getMatchCondition()); + return map.countKey(instance, subMatchCondition); } } return super.countKey(instance, matchCondition); @@ -192,13 +198,14 @@ public Collection values() { public Collection getAll(T key, M matchCondition) { if (appliesToClassifier(matchCondition)) { IIngredientMapMutable map = this.classifiedMaps.get(getClassifier(key)); - if (map != null) { - if (Objects.equals(getCategoryType().getMatchCondition(), matchCondition)) { - return map.values(); - } else { - M subMatchCondition = getComponent().getMatcher().withoutCondition(matchCondition, getCategoryType().getMatchCondition()); - return map.getAll(key, subMatchCondition); - } + if (map == null) { + return Collections.emptyList(); + } + if (Objects.equals(getCategoryType().getMatchCondition(), matchCondition)) { + return map.values(); + } else { + M subMatchCondition = getComponent().getMatcher().withoutCondition(matchCondition, getCategoryType().getMatchCondition()); + return map.getAll(key, subMatchCondition); } } return super.getAll(key, matchCondition); @@ -208,13 +215,14 @@ public Collection getAll(T key, M matchCondition) { public IngredientSet keySet(T key, M matchCondition) { if (appliesToClassifier(matchCondition)) { IIngredientMapMutable map = this.classifiedMaps.get(getClassifier(key)); - if (map != null) { - if (Objects.equals(getCategoryType().getMatchCondition(), matchCondition)) { - return map.keySet(); - } else { - M subMatchCondition = getComponent().getMatcher().withoutCondition(matchCondition, getCategoryType().getMatchCondition()); - return map.keySet(key, subMatchCondition); - } + if (map == null) { + return new IngredientHashSet<>(getComponent()); + } + if (Objects.equals(getCategoryType().getMatchCondition(), matchCondition)) { + return map.keySet(); + } else { + M subMatchCondition = getComponent().getMatcher().withoutCondition(matchCondition, getCategoryType().getMatchCondition()); + return map.keySet(key, subMatchCondition); } } return super.keySet(key, matchCondition); diff --git a/loader-neoforge/src/test/java/org/cyclops/cyclopscore/helper/TestItemStackHelpersHashCode.java b/loader-neoforge/src/test/java/org/cyclops/cyclopscore/helper/TestItemStackHelpersHashCode.java new file mode 100644 index 00000000000..e7d7c9052da --- /dev/null +++ b/loader-neoforge/src/test/java/org/cyclops/cyclopscore/helper/TestItemStackHelpersHashCode.java @@ -0,0 +1,137 @@ +package org.cyclops.cyclopscore.helper; + +import net.minecraft.core.Holder; +import net.minecraft.core.MappedRegistry; +import net.minecraft.core.component.DataComponentMap; +import net.minecraft.core.component.DataComponents; +import net.minecraft.core.registries.BuiltInRegistries; +import net.minecraft.network.chat.Component; +import net.minecraft.world.item.Item; +import net.minecraft.world.item.ItemStack; +import org.cyclops.cyclopscore.inventory.ItemDummy; +import org.junit.jupiter.api.Test; + +import java.util.HashSet; +import java.util.Set; + +import static org.hamcrest.CoreMatchers.is; +import static org.hamcrest.CoreMatchers.not; +import static org.hamcrest.MatcherAssert.assertThat; + +/** + * Unit tests for the ItemStack hash code of {@link ItemStackHelpersCommon}. + * + * The hash has to take data components into account. Ingredient collections key on it, and a + * component-blind hash puts every stack of the same item in one bucket, which turns their + * lookups into scans over full component comparisons. + * + * @author rubensworks + */ +public class TestItemStackHelpersHashCode { + + static { + ((MappedRegistry) BuiltInRegistries.ITEM).unfreeze(true); + } + + private static final Item ITEM1 = new ItemDummy(); + private static final Item ITEM2 = new ItemDummy(); + + static { + ((Holder.Reference) ITEM1.builtInRegistryHolder()).bindComponents(DataComponentMap.EMPTY); + ((Holder.Reference) ITEM2.builtInRegistryHolder()).bindComponents(DataComponentMap.EMPTY); + } + + private static final IItemStackHelpers HELPERS = new ItemStackHelpersNeoForge(); + + private static int hash(ItemStack stack) { + return HELPERS.getItemStackHashCode(stack); + } + + private static ItemStack named(Item item, int count, String name) { + ItemStack stack = new ItemStack(item, count); + stack.set(DataComponents.CUSTOM_NAME, Component.literal(name)); + return stack; + } + + @Test + public void testEqualStacksHashEqual() { + assertThat(hash(new ItemStack(ITEM1)), is(hash(new ItemStack(ITEM1)))); + assertThat(hash(new ItemStack(ITEM1, 7)), is(hash(new ItemStack(ITEM1, 7)))); + assertThat(hash(named(ITEM1, 3, "a")), is(hash(named(ITEM1, 3, "a")))); + assertThat(hash(ItemStack.EMPTY), is(hash(ItemStack.EMPTY))); + } + + @Test + public void testDifferentItemsHashDifferently() { + assertThat(hash(new ItemStack(ITEM1)), is(not(hash(new ItemStack(ITEM2))))); + } + + @Test + public void testDifferentCountsHashDifferently() { + assertThat(hash(new ItemStack(ITEM1, 1)), is(not(hash(new ItemStack(ITEM1, 2))))); + } + + /** + * The regression this guards: stacks of one item differing only by components used to share a hash. + */ + @Test + public void testComponentsAffectHash() { + assertThat(hash(named(ITEM1, 1, "a")), is(not(hash(named(ITEM1, 1, "b"))))); + assertThat(hash(new ItemStack(ITEM1)), is(not(hash(named(ITEM1, 1, "a"))))); + } + + /** + * A single differing hash could be luck. Over a sample of same-item stacks the hash has to + * spread, or hash-based collections degrade to linear scans. + */ + @Test + public void testComponentVariantsSpreadOverManyBuckets() { + int samples = 1000; + Set hashes = new HashSet<>(); + for (int i = 0; i < samples; i++) { + hashes.add(hash(named(ITEM1, 1, "variant " + i))); + } + assertThat("Component variants of one item have to produce distinct hashes", + hashes.size() > samples * 0.99, is(true)); + } + + /** + * Stacks carrying no component patch skip the component hash, so this pins that they still + * spread over items and counts, and still agree with equality. + */ + @Test + public void testPlainStacksSpreadOverItemsAndCounts() { + assertThat(hash(new ItemStack(ITEM1, 1)), is(not(hash(new ItemStack(ITEM2, 1))))); + assertThat(hash(new ItemStack(ITEM1, 1)), is(not(hash(new ItemStack(ITEM1, 2))))); + assertThat(hash(new ItemStack(ITEM1, 5)), is(hash(new ItemStack(ITEM1, 5)))); + } + + /** + * A stack whose only component is set back to its default carries no patch any more, so it is + * equal to the plain stack and has to hash like one. + */ + @Test + public void testComponentSetBackToDefaultHashesAsPlain() { + ItemStack plain = new ItemStack(ITEM1); + ItemStack restored = new ItemStack(ITEM1); + restored.set(DataComponents.CUSTOM_NAME, Component.literal("a")); + assertThat(hash(restored), is(not(hash(plain)))); + restored.set(DataComponents.CUSTOM_NAME, null); + assertThat(ItemStack.isSameItemSameComponents(plain, restored), is(true)); + assertThat(hash(restored), is(hash(plain))); + } + + /** + * The hash may never distinguish two stacks that count as equal, or lookups miss. + */ + @Test + public void testHashIsConsistentWithComponentEquality() { + for (int i = 0; i < 100; i++) { + ItemStack a = named(ITEM1, 1 + (i % 5), "variant " + i); + ItemStack b = named(ITEM1, 1 + (i % 5), "variant " + i); + assertThat(ItemStack.isSameItemSameComponents(a, b), is(true)); + assertThat(a.getCount(), is(b.getCount())); + assertThat(hash(a), is(hash(b))); + } + } +} diff --git a/loader-neoforge/src/test/java/org/cyclops/cyclopscore/ingredient/collection/TestSingleClassifiedAbsentClassifier.java b/loader-neoforge/src/test/java/org/cyclops/cyclopscore/ingredient/collection/TestSingleClassifiedAbsentClassifier.java new file mode 100644 index 00000000000..bd5c11e431a --- /dev/null +++ b/loader-neoforge/src/test/java/org/cyclops/cyclopscore/ingredient/collection/TestSingleClassifiedAbsentClassifier.java @@ -0,0 +1,191 @@ +package org.cyclops.cyclopscore.ingredient.collection; + +import org.cyclops.cyclopscore.ingredient.ComplexStack; +import org.cyclops.cyclopscore.ingredient.IngredientComponentStubs; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +import java.util.Collection; +import java.util.Iterator; +import java.util.Map; +import java.util.concurrent.atomic.AtomicInteger; + +import static org.hamcrest.CoreMatchers.is; +import static org.hamcrest.MatcherAssert.assertThat; + +/** + * Queries on a single-classified collection whose classifier holds nothing. + * + * A match condition that covers the category type can only match instances sharing the query's + * classifier, so an absent classifier means an empty result. These lookups must say so directly + * rather than falling back on a scan over every classifier, which is both slower and grows with + * the size of the whole collection instead of with the size of one classifier. + * + * @author rubensworks + */ +public class TestSingleClassifiedAbsentClassifier { + + private static final int GROUP = ComplexStack.Match.GROUP; + private static final int GROUP_META = ComplexStack.Match.GROUP | ComplexStack.Match.META; + + private static final ComplexStack A01 = new ComplexStack(ComplexStack.Group.A, 0, 1, null); + private static final ComplexStack A12 = new ComplexStack(ComplexStack.Group.A, 1, 2, null); + private static final ComplexStack B01 = new ComplexStack(ComplexStack.Group.B, 0, 1, null); + /** + * Nothing of this group is ever added. + */ + private static final ComplexStack C01 = new ComplexStack(ComplexStack.Group.C, 0, 1, null); + + /** + * Counts every time an inner collection is asked to iterate, so that a fallback scan over all + * classifiers becomes visible rather than only slow. + */ + private AtomicInteger innerIterations; + + private IngredientCollectionSingleClassified> collection; + private IngredientMapSingleClassified map; + + @BeforeEach + public void before() { + this.innerIterations = new AtomicInteger(); + + this.collection = new IngredientCollectionSingleClassified<>(IngredientComponentStubs.COMPLEX, + () -> new CountingSet(this.innerIterations), + IngredientComponentStubs.COMPLEX.getCategoryTypes().get(0)); + this.collection.add(A01); + this.collection.add(A12); + this.collection.add(B01); + + this.map = new IngredientMapSingleClassified<>(IngredientComponentStubs.COMPLEX, + () -> new CountingMap(this.innerIterations), + IngredientComponentStubs.COMPLEX.getCategoryTypes().get(0)); + this.map.put(A01, "a01"); + this.map.put(A12, "a12"); + this.map.put(B01, "b01"); + + this.innerIterations.set(0); + } + + @Test + public void testCollectionCountAbsentClassifier() { + assertThat(collection.count(C01, GROUP), is(0)); + assertThat(collection.count(C01, GROUP_META), is(0)); + assertThat(innerIterations.get(), is(0)); + } + + @Test + public void testCollectionCountPresentClassifier() { + assertThat(collection.count(A01, GROUP), is(2)); + assertThat(collection.count(B01, GROUP), is(1)); + assertThat(collection.count(A01, GROUP_META), is(1)); + } + + @Test + public void testMapCountKeyAbsentClassifier() { + assertThat(map.countKey(C01, GROUP), is(0)); + assertThat(map.countKey(C01, GROUP_META), is(0)); + assertThat(innerIterations.get(), is(0)); + } + + @Test + public void testMapCountKeyPresentClassifier() { + assertThat(map.countKey(A01, GROUP), is(2)); + assertThat(map.countKey(B01, GROUP), is(1)); + assertThat(map.countKey(A01, GROUP_META), is(1)); + } + + @Test + public void testMapContainsKeyAbsentClassifier() { + assertThat(map.containsKey(C01, GROUP), is(false)); + assertThat(map.containsKey(C01, GROUP_META), is(false)); + assertThat(innerIterations.get(), is(0)); + } + + @Test + public void testMapContainsKeyPresentClassifier() { + assertThat(map.containsKey(A01, GROUP), is(true)); + assertThat(map.containsKey(A01, GROUP_META), is(true)); + } + + @Test + public void testMapGetAllAbsentClassifier() { + assertThat(map.getAll(C01, GROUP).isEmpty(), is(true)); + assertThat(map.getAll(C01, GROUP_META).isEmpty(), is(true)); + assertThat(innerIterations.get(), is(0)); + } + + @Test + public void testMapGetAllPresentClassifier() { + assertThat(map.getAll(A01, GROUP).size(), is(2)); + assertThat(map.getAll(B01, GROUP).size(), is(1)); + assertThat(map.getAll(A01, GROUP_META).size(), is(1)); + } + + @Test + public void testMapKeySetAbsentClassifier() { + assertThat(map.keySet(C01, GROUP).isEmpty(), is(true)); + assertThat(map.keySet(C01, GROUP_META).isEmpty(), is(true)); + assertThat(innerIterations.get(), is(0)); + } + + @Test + public void testMapKeySetPresentClassifier() { + assertThat(map.keySet(A01, GROUP).size(), is(2)); + assertThat(map.keySet(B01, GROUP).size(), is(1)); + assertThat(map.keySet(A01, GROUP_META).size(), is(1)); + } + + /** + * A match condition that does not cover the category type can not be answered by the + * classifier, so it still has to visit the inner collections. + */ + @Test + public void testMatchConditionOutsideClassifierStillScans() { + assertThat(map.countKey(C01, ComplexStack.Match.META), is(2)); + assertThat(innerIterations.get() > 0, is(true)); + } + + private static class CountingSet extends IngredientHashSet { + + private final AtomicInteger counter; + + public CountingSet(AtomicInteger counter) { + super(IngredientComponentStubs.COMPLEX); + this.counter = counter; + } + + @Override + public Iterator iterator() { + this.counter.incrementAndGet(); + return super.iterator(); + } + } + + private static class CountingMap extends IngredientHashMap { + + private final AtomicInteger counter; + + public CountingMap(AtomicInteger counter) { + super(IngredientComponentStubs.COMPLEX); + this.counter = counter; + } + + @Override + public Iterator> iterator() { + this.counter.incrementAndGet(); + return super.iterator(); + } + + @Override + public IngredientSet keySet() { + this.counter.incrementAndGet(); + return super.keySet(); + } + + @Override + public Collection values() { + this.counter.incrementAndGet(); + return super.values(); + } + } +}