Hash an ingredient key once per index update - #241
Open
rubensworks wants to merge 2 commits into
Open
Conversation
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Mxjin31W1Lmq5XK1CCe84v (cherry picked from commit 1ae5111)
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Mxjin31W1Lmq5XK1CCe84v
Member
Author
|
Correcting a placeholder in the description: the companion IntegratedDynamics change is CyclopsMC/IntegratedDynamics#1728. Generated by Claude Code |
|
This was referenced Sep 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.



The 1.21 version of #239.
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
IngredientMapWrappedAdapterbuilds its own wrapper inside each ofget,putandremove, so one logical update hashed the same key twice. Storage networks then run every change twice over, once for its own channel and once for the wildcard channel, so it multiplies.IIngredientMapMutablegains acomputemethod withMap.computesemantics. The default implementation is the old read-then-write, so every existing implementation stays correct without changing.IngredientMapWrappedAdapterbuilds one wrapper and hands it to the backing map's owncompute.IngredientMapSingleClassifieddelegates to the classifier's sub-map while keeping its size counter and its empty-classifier cleanup.IngredientCollectionPrototypeMap.addandremoveuse it. Component types whose hash is cheap see no difference either way.The companion change on the IntegratedDynamics side is CyclopsMC/IntegratedDynamics#PENDING_ID_COMPUTE, which converts
IngredientPositionsIndex.addPositionandremovePosition.Numbers
These come from 26.1, not from 1.21. All four touched files are byte identical between
master-1.21-ltsandmaster-26-lts, so the change is literally the same code, but I did not re-run the benchmarks here. Say the word and I will; each full run on this branch takes about 33 minutes, so a paired set is a couple of hours.IntegratedDynamics index benchmarks on 26.1, ms per operation, medians of six runs each, alternating between the two configurations within one session so session drift falls on both equally. Both sides carry the fixes from #238.
Lookup benchmarks all land within 10% either way with no consistent direction, which is expected: they do not read-then-write, so there is nothing for this to collapse.
The win is concentrated where hashing genuinely dominates a modification: many component variants of one item, or a large component payload. It is much weaker on the realistic shapes, because #238 already skips the component hash entirely for stacks carrying no patch, so there is little hashing left to halve. The spread-shape row reads 11% slower, but that benchmark runs first in the sequence and absorbs JIT warm-up, and its two distributions overlap almost completely; I would not read a regression into it and cannot rule one out either.
I predicted this would be worth about a third of the remaining modification cost. That held for the hash-dominated shapes and overestimated the realistic ones. Reporting the miss rather than the prediction.
One caveat specific to this branch: on 1.21 the hash fix makes modifications faster rather than slower, because the baseline collides much harder here. That means the cost this change exists to reduce is a smaller share of the total on 1.21 than on 26.1, so the benefit here is probably smaller than the table above suggests.
Recommendation
Worth taking, but not urgently. It is a strict reduction in work, the API mirrors
Map.computeso it is not exotic, and the default keeps every other implementation correct untouched. But the realistic-mix gain is inside the measurement spread, so if you would rather not widenIIngredientMapMutablefor that, closing this is a defensible call.Tests
TestIngredientMapComputecovers insert, update, remove, a null remapping on an absent key, key pass-through, and size tracking across classifiers, overIngredientHashMap,IngredientTreeMapandIngredientMapSingleClassified. It also drives a sequence of computes against a second map doing the get-then-put by hand and asserts the two stay identical after every step. It is written against this branch's JUnit 4 setup, in a separate commit so the port is visible on its own../gradlew buildpasses on all loaders.Relationship to the 26.1 branch
#239 is the same change against
master-26-ltsand stays open. Merge whichever suits your upmerge direction.Notes for review
IngredientMapMultiClassifiedkeeps the default implementation. It is correct, just not collapsed; the positions index never uses it.Generated by Claude Code