From b83b14337e44ca934ead18523d13a569888d0895 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 6 Sep 2026 15:53:18 +0000 Subject: [PATCH 1/2] Hash an ingredient key once per index update An ingredient map hashes its key on every operation, and for ItemStack that hash walks a data component map. The update paths all read then write, and each call builds its own wrapper, so the same key was hashed twice for one logical update. Storage networks then run each change twice over, once for its own channel and once for the wildcard channel, so this multiplies. IIngredientMapMutable gains a compute method with Map.compute semantics. The default is the old read-then-write, so nothing has to change to stay correct. The wrapped adapter builds one wrapper and hands it to the backing map's own compute, and the classified map delegates to the classifier's sub-map while keeping its size and its empty-classifier cleanup. IngredientCollectionPrototypeMap add and remove use it. Callers that hold a cheap-to-hash component type see no difference. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Mxjin31W1Lmq5XK1CCe84v (cherry picked from commit 1ae5111d17e7743a0b3718c0d42d8575802f46d4) --- .../collection/IIngredientMapMutable.java | 28 ++++ .../IngredientCollectionPrototypeMap.java | 39 ++--- .../IngredientMapSingleClassified.java | 24 +++ .../IngredientMapWrappedAdapter.java | 9 ++ .../collection/TestIngredientMapCompute.java | 138 ++++++++++++++++++ 5 files changed, 220 insertions(+), 18 deletions(-) create mode 100644 loader-neoforge/src/test/java/org/cyclops/cyclopscore/ingredient/collection/TestIngredientMapCompute.java diff --git a/loader-neoforge/src/main/java/org/cyclops/cyclopscore/ingredient/collection/IIngredientMapMutable.java b/loader-neoforge/src/main/java/org/cyclops/cyclopscore/ingredient/collection/IIngredientMapMutable.java index 9da53deb1e8..ba2e04661bc 100644 --- a/loader-neoforge/src/main/java/org/cyclops/cyclopscore/ingredient/collection/IIngredientMapMutable.java +++ b/loader-neoforge/src/main/java/org/cyclops/cyclopscore/ingredient/collection/IIngredientMapMutable.java @@ -5,6 +5,7 @@ import javax.annotation.Nullable; import java.util.Map; import java.util.Objects; +import java.util.function.BiFunction; /** * A mutable mapping from ingredient component instances to values of any type. @@ -90,6 +91,33 @@ default public int removeAll(Iterable instances, M matchCondition) return removed; } + /** + * Compute a new value for the given key instance, following {@link Map#compute} semantics. + * + * The remapping function is passed the key and the currently mapped value, or null when the key + * is absent. Returning null removes the mapping, returning anything else stores it. + * + * Implementations back onto a hash of the key instance, which for some component types is + * expensive. Doing this in one call rather than a get followed by a put lets them hash once. + * + * @param key An instance key. + * @param remappingFunction A function computing the new value from the key and the old value. + * @return The new value associated with the key, or null if none. + */ + @Nullable + default V compute(T key, BiFunction remappingFunction) { + V oldValue = get(key); + V newValue = remappingFunction.apply(key, oldValue); + if (newValue == null) { + if (oldValue != null) { + remove(key); + } + } else { + put(key, newValue); + } + return newValue; + } + /** * Add all entries from the given map to this map. * @param map A map, that will not be changed, only read. diff --git a/loader-neoforge/src/main/java/org/cyclops/cyclopscore/ingredient/collection/IngredientCollectionPrototypeMap.java b/loader-neoforge/src/main/java/org/cyclops/cyclopscore/ingredient/collection/IngredientCollectionPrototypeMap.java index db61a9db58c..1235020a448 100644 --- a/loader-neoforge/src/main/java/org/cyclops/cyclopscore/ingredient/collection/IngredientCollectionPrototypeMap.java +++ b/loader-neoforge/src/main/java/org/cyclops/cyclopscore/ingredient/collection/IngredientCollectionPrototypeMap.java @@ -52,14 +52,14 @@ public IngredientCollectionPrototypeMap(IngredientComponent component, boo public boolean add(T instance) { IIngredientMatcher matcher = getComponent().getMatcher(); T prototype = getPrototype(instance); - Long value = ingredients.get(prototype); - long existingValue = value == null ? 0 : value; - long newValue = Math.addExact(existingValue, matcher.getQuantity(instance)); - if (newValue != 0) { - ingredients.put(prototype, newValue); - } else { - ingredients.remove(prototype); - } + long quantity = matcher.getQuantity(instance); + // Computed in one call so the prototype is hashed once rather than once to read and once + // to write. Hashing a prototype is the dominant cost here for component-bearing instances. + ingredients.compute(prototype, (key, value) -> { + long existingValue = value == null ? 0 : value; + long newValue = Math.addExact(existingValue, quantity); + return newValue == 0 ? null : newValue; + }); return true; } @@ -67,17 +67,20 @@ public boolean add(T instance) { public boolean remove(T instance) { IIngredientMatcher matcher = getComponent().getMatcher(); T prototype = getPrototype(instance); - Long value = ingredients.get(prototype); - long existingValue = value == null ? 0 : value; long currentValue = matcher.getQuantity(instance); - if (currentValue == existingValue) { - ingredients.remove(prototype); - return true; - } else if (currentValue < existingValue || isNegativeQuantities()) { - ingredients.put(prototype, existingValue - currentValue); - return true; - } - return false; + boolean[] removed = new boolean[1]; + ingredients.compute(prototype, (key, value) -> { + long existingValue = value == null ? 0 : value; + if (currentValue == existingValue) { + removed[0] = true; + return null; + } else if (currentValue < existingValue || isNegativeQuantities()) { + removed[0] = true; + return existingValue - currentValue; + } + return value; + }); + return removed[0]; } @Override 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..809959eeb71 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 @@ -12,6 +12,7 @@ import java.util.List; import java.util.Map; import java.util.Objects; +import java.util.function.BiFunction; import java.util.function.Supplier; /** @@ -112,6 +113,29 @@ public V remove(T key) { return null; } + @Nullable + @Override + public V compute(T key, BiFunction remappingFunction) { + C classifier = getClassifier(key); + IIngredientMapMutable map = this.classifiedMaps.get(classifier); + if (map == null) { + // Nothing is stored under this classifier, so only an insertion can come out of this + V newValue = remappingFunction.apply(key, null); + if (newValue != null) { + getOrCreateClassifiedCollection(classifier).put(key, newValue); + this.size++; + } + return newValue; + } + int sizeBefore = map.size(); + V newValue = map.compute(key, remappingFunction); + this.size += map.size() - sizeBefore; + if (map.isEmpty()) { + this.classifiedMaps.remove(classifier); + } + return newValue; + } + @Override public int size() { return this.size; diff --git a/loader-neoforge/src/main/java/org/cyclops/cyclopscore/ingredient/collection/IngredientMapWrappedAdapter.java b/loader-neoforge/src/main/java/org/cyclops/cyclopscore/ingredient/collection/IngredientMapWrappedAdapter.java index 49e0474364b..2bb85241b0c 100644 --- a/loader-neoforge/src/main/java/org/cyclops/cyclopscore/ingredient/collection/IngredientMapWrappedAdapter.java +++ b/loader-neoforge/src/main/java/org/cyclops/cyclopscore/ingredient/collection/IngredientMapWrappedAdapter.java @@ -9,6 +9,7 @@ import java.util.Collection; import java.util.Iterator; import java.util.Map; +import java.util.function.BiFunction; /** * An abstract ingredient map adapter. @@ -68,6 +69,14 @@ public V get(T key) { return this.collection.get(wrap(key)); } + @Nullable + @Override + public V compute(T key, BiFunction remappingFunction) { + // One wrapper, so the key is hashed once instead of once for the get and once for the put + return this.collection.compute(wrap(key), + (wrapper, value) -> remappingFunction.apply(wrapper.getInstance(), value)); + } + @Override public IngredientSet keySet() { return new IngredientSet<>(this.getComponent(), this.collection.keySet()); diff --git a/loader-neoforge/src/test/java/org/cyclops/cyclopscore/ingredient/collection/TestIngredientMapCompute.java b/loader-neoforge/src/test/java/org/cyclops/cyclopscore/ingredient/collection/TestIngredientMapCompute.java new file mode 100644 index 00000000000..77aab6c01e9 --- /dev/null +++ b/loader-neoforge/src/test/java/org/cyclops/cyclopscore/ingredient/collection/TestIngredientMapCompute.java @@ -0,0 +1,138 @@ +package org.cyclops.cyclopscore.ingredient.collection; + +import org.cyclops.cyclopscore.ingredient.ComplexStack; +import org.cyclops.cyclopscore.ingredient.IngredientComponentStubs; +import org.junit.jupiter.api.TestInstance; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; + +import java.util.function.Supplier; +import java.util.stream.Stream; + +import static org.hamcrest.CoreMatchers.is; +import static org.hamcrest.CoreMatchers.nullValue; +import static org.hamcrest.MatcherAssert.assertThat; + +/** + * {@link IIngredientMapMutable#compute} across the map implementations. + * + * It exists so that a read followed by a write can hash the key once instead of twice, so the + * results have to stay identical to doing both separately. + * + * @author rubensworks + */ +@TestInstance(TestInstance.Lifecycle.PER_CLASS) +public class TestIngredientMapCompute { + + 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); + + public Stream maps() { + return Stream.>>of( + () -> new IngredientHashMap<>(IngredientComponentStubs.COMPLEX), + () -> new IngredientTreeMap<>(IngredientComponentStubs.COMPLEX), + () -> new IngredientMapSingleClassified<>(IngredientComponentStubs.COMPLEX, + () -> new IngredientHashMap<>(IngredientComponentStubs.COMPLEX), + IngredientComponentStubs.COMPLEX.getCategoryTypes().get(0)) + ).map(Arguments::of); + } + + @ParameterizedTest + @MethodSource("maps") + public void testInsertsWhenAbsent(Supplier> factory) { + IIngredientMapMutable map = factory.get(); + assertThat(map.compute(A01, (key, value) -> { + assertThat(value, is(nullValue())); + return "first"; + }), is("first")); + assertThat(map.get(A01), is("first")); + assertThat(map.size(), is(1)); + } + + @ParameterizedTest + @MethodSource("maps") + public void testUpdatesWhenPresent(Supplier> factory) { + IIngredientMapMutable map = factory.get(); + map.put(A01, "first"); + assertThat(map.compute(A01, (key, value) -> value + "+second"), is("first+second")); + assertThat(map.get(A01), is("first+second")); + assertThat(map.size(), is(1)); + } + + @ParameterizedTest + @MethodSource("maps") + public void testRemovesOnNull(Supplier> factory) { + IIngredientMapMutable map = factory.get(); + map.put(A01, "first"); + map.put(A12, "other"); + assertThat(map.compute(A01, (key, value) -> null), is(nullValue())); + assertThat(map.get(A01), is(nullValue())); + assertThat(map.get(A12), is("other")); + assertThat(map.size(), is(1)); + } + + @ParameterizedTest + @MethodSource("maps") + public void testNullOnAbsentIsNoOp(Supplier> factory) { + IIngredientMapMutable map = factory.get(); + assertThat(map.compute(A01, (key, value) -> null), is(nullValue())); + assertThat(map.size(), is(0)); + assertThat(map.isEmpty(), is(true)); + } + + @ParameterizedTest + @MethodSource("maps") + public void testKeyIsPassedThrough(Supplier> factory) { + IIngredientMapMutable map = factory.get(); + map.compute(A01, (key, value) -> { + assertThat(key, is(A01)); + return "v"; + }); + } + + /** + * The classified map keeps its own size and drops classifiers that run empty, so computing + * away the last entry of a classifier has to clean up just as a remove would. + */ + @ParameterizedTest + @MethodSource("maps") + public void testSizeTracksAcrossClassifiers(Supplier> factory) { + IIngredientMapMutable map = factory.get(); + map.compute(A01, (key, value) -> "a"); + map.compute(B01, (key, value) -> "b"); + assertThat(map.size(), is(2)); + map.compute(B01, (key, value) -> null); + assertThat(map.size(), is(1)); + assertThat(map.get(A01), is("a")); + assertThat(map.get(B01), is(nullValue())); + map.compute(B01, (key, value) -> "b again"); + assertThat(map.size(), is(2)); + assertThat(map.get(B01), is("b again")); + } + + @ParameterizedTest + @MethodSource("maps") + public void testMatchesGetThenPut(Supplier> factory) { + IIngredientMapMutable computed = factory.get(); + IIngredientMapMutable manual = factory.get(); + ComplexStack[] keys = {A01, A12, B01, A01, B01, A12}; + for (int i = 0; i < keys.length; i++) { + ComplexStack key = keys[i]; + int step = i; + computed.compute(key, (k, value) -> step % 3 == 2 ? null : (value == null ? "v" + step : value + "v" + step)); + String oldValue = manual.get(key); + String newValue = step % 3 == 2 ? null : (oldValue == null ? "v" + step : oldValue + "v" + step); + if (newValue == null) { + manual.remove(key); + } else { + manual.put(key, newValue); + } + assertThat(computed.size(), is(manual.size())); + for (ComplexStack probe : new ComplexStack[]{A01, A12, B01}) { + assertThat(computed.get(probe), is(manual.get(probe))); + } + } + } +} From 1329bf41a9bd42f9fce49e435e0d47403cbb59ba Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 6 Sep 2026 18:47:23 +0000 Subject: [PATCH 2/2] Adjust the compute test for the 1.21 test setup This branch uses JUnit 4, so the parameterized test follows the Parameterized runner pattern the other collection tests here use. The assertions are unchanged. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Mxjin31W1Lmq5XK1CCe84v --- .../collection/TestIngredientMapCompute.java | 89 ++++++++++--------- 1 file changed, 46 insertions(+), 43 deletions(-) diff --git a/loader-neoforge/src/test/java/org/cyclops/cyclopscore/ingredient/collection/TestIngredientMapCompute.java b/loader-neoforge/src/test/java/org/cyclops/cyclopscore/ingredient/collection/TestIngredientMapCompute.java index 77aab6c01e9..ee69508e99d 100644 --- a/loader-neoforge/src/test/java/org/cyclops/cyclopscore/ingredient/collection/TestIngredientMapCompute.java +++ b/loader-neoforge/src/test/java/org/cyclops/cyclopscore/ingredient/collection/TestIngredientMapCompute.java @@ -2,13 +2,13 @@ import org.cyclops.cyclopscore.ingredient.ComplexStack; import org.cyclops.cyclopscore.ingredient.IngredientComponentStubs; -import org.junit.jupiter.api.TestInstance; -import org.junit.jupiter.params.ParameterizedTest; -import org.junit.jupiter.params.provider.Arguments; -import org.junit.jupiter.params.provider.MethodSource; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.Parameterized; +import java.util.Arrays; +import java.util.Collection; import java.util.function.Supplier; -import java.util.stream.Stream; import static org.hamcrest.CoreMatchers.is; import static org.hamcrest.CoreMatchers.nullValue; @@ -22,27 +22,36 @@ * * @author rubensworks */ -@TestInstance(TestInstance.Lifecycle.PER_CLASS) +@RunWith(Parameterized.class) public class TestIngredientMapCompute { 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); - public Stream maps() { - return Stream.>>of( - () -> new IngredientHashMap<>(IngredientComponentStubs.COMPLEX), - () -> new IngredientTreeMap<>(IngredientComponentStubs.COMPLEX), - () -> new IngredientMapSingleClassified<>(IngredientComponentStubs.COMPLEX, - () -> new IngredientHashMap<>(IngredientComponentStubs.COMPLEX), - IngredientComponentStubs.COMPLEX.getCategoryTypes().get(0)) - ).map(Arguments::of); + @Parameterized.Parameters + public static Collection data() { + return Arrays.asList(new Object[][]{ + {(Supplier>) + () -> new IngredientHashMap<>(IngredientComponentStubs.COMPLEX)}, + {(Supplier>) + () -> new IngredientTreeMap<>(IngredientComponentStubs.COMPLEX)}, + {(Supplier>) + () -> new IngredientMapSingleClassified<>(IngredientComponentStubs.COMPLEX, + () -> new IngredientHashMap<>(IngredientComponentStubs.COMPLEX), + IngredientComponentStubs.COMPLEX.getCategoryTypes().get(0))}, + }); + } + + private final Supplier> factory; + + public TestIngredientMapCompute(Supplier> factory) { + this.factory = factory; } - @ParameterizedTest - @MethodSource("maps") - public void testInsertsWhenAbsent(Supplier> factory) { - IIngredientMapMutable map = factory.get(); + @Test + public void testInsertsWhenAbsent() { + IIngredientMapMutable map = this.factory.get(); assertThat(map.compute(A01, (key, value) -> { assertThat(value, is(nullValue())); return "first"; @@ -51,20 +60,18 @@ public void testInsertsWhenAbsent(Supplier> factory) { - IIngredientMapMutable map = factory.get(); + @Test + public void testUpdatesWhenPresent() { + IIngredientMapMutable map = this.factory.get(); map.put(A01, "first"); assertThat(map.compute(A01, (key, value) -> value + "+second"), is("first+second")); assertThat(map.get(A01), is("first+second")); assertThat(map.size(), is(1)); } - @ParameterizedTest - @MethodSource("maps") - public void testRemovesOnNull(Supplier> factory) { - IIngredientMapMutable map = factory.get(); + @Test + public void testRemovesOnNull() { + IIngredientMapMutable map = this.factory.get(); map.put(A01, "first"); map.put(A12, "other"); assertThat(map.compute(A01, (key, value) -> null), is(nullValue())); @@ -73,19 +80,17 @@ public void testRemovesOnNull(Supplier> factory) { - IIngredientMapMutable map = factory.get(); + @Test + public void testNullOnAbsentIsNoOp() { + IIngredientMapMutable map = this.factory.get(); assertThat(map.compute(A01, (key, value) -> null), is(nullValue())); assertThat(map.size(), is(0)); assertThat(map.isEmpty(), is(true)); } - @ParameterizedTest - @MethodSource("maps") - public void testKeyIsPassedThrough(Supplier> factory) { - IIngredientMapMutable map = factory.get(); + @Test + public void testKeyIsPassedThrough() { + IIngredientMapMutable map = this.factory.get(); map.compute(A01, (key, value) -> { assertThat(key, is(A01)); return "v"; @@ -96,10 +101,9 @@ public void testKeyIsPassedThrough(Supplier> factory) { - IIngredientMapMutable map = factory.get(); + @Test + public void testSizeTracksAcrossClassifiers() { + IIngredientMapMutable map = this.factory.get(); map.compute(A01, (key, value) -> "a"); map.compute(B01, (key, value) -> "b"); assertThat(map.size(), is(2)); @@ -112,11 +116,10 @@ public void testSizeTracksAcrossClassifiers(Supplier> factory) { - IIngredientMapMutable computed = factory.get(); - IIngredientMapMutable manual = factory.get(); + @Test + public void testMatchesGetThenPut() { + IIngredientMapMutable computed = this.factory.get(); + IIngredientMapMutable manual = this.factory.get(); ComplexStack[] keys = {A01, A12, B01, A01, B01, A12}; for (int i = 0; i < keys.length; i++) { ComplexStack key = keys[i];