Fix ItemStack hashing and classified ingredient lookups - #238
Fix ItemStack hashing and classified ingredient lookups#238rubensworks wants to merge 5 commits into
Conversation
|
The companion benchmark PR referenced above is CyclopsMC/IntegratedDynamics#1722. It is independent of this change and can be merged on its own. Generated by Claude Code |
|
coverage: 31.112% (+0.09%) from 31.026% — fix/itemstack-hash-components into master-26-lts |
rubensworks
changed the title
|
Re-measured the storage terminal against the build carrying all three changes, so the numbers in the description no longer lag the branch. Loopback, one channel, medians of seven runs after a discarded warm-up, times in ms.
Within the run to run spread this matches the hash-only measurement quoted in the description, which is what I expected: the terminal open path is dominated by exact lookups, and the other two changes target item-only lookups and modifications. Neither made it worse. Generated by Claude Code |
|
The shapes in the description bracket a real storage network rather than describing one, so I added a
The modification cost tracks the share of instances carrying components, since only those pay for the hash:
In absolute terms a modification on the mixed shape costs 209 ns more; an item-only lookup on the same shape costs 220 microseconds less. Correcting something I wrote earlierI listed caching the hash inside The real redundancy for modifications is different: the get-then-put pairs in I have not implemented it; it is a wider API change than this PR should carry, and it is orthogonal to the correctness fix here. Happy to open it separately if you want it. Generated by Claude Code |
getItemStackHashCode hashed only count and item. Equality compares data components, so the hash was strictly less discriminating than equality: every stack of one item landed in the same bucket of any hash-based ingredient collection. Collapsed collections normalise the count to 1 before using a stack as a key, so the count term is constant there and the hash degenerated to a function of the item alone. With enough component variants per item the buckets treeify and every lookup turns into a tree walk doing full data component comparisons, which is what made large Integrated Dynamics storage networks scale quadratically. Profiling a 50k stack storage terminal open showed 61% of server thread samples inside treeified HashMap buckets and 65% inside IngredientInstanceWrapper.equals. Including components takes that open from 4686 ms to 168 ms of server thread time. The exclusion comment dated from NBT tags, which were expensive to hash. Component maps are not, and vanilla hashes them the same way in ItemStack.hashItemAndComponents. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Mxjin31W1Lmq5XK1CCe84v
ItemStackHelpersCommon is abstract on more than getItemStackHashCode, so an anonymous subclass does not compile. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Mxjin31W1Lmq5XK1CCe84v
A single-classified collection partitions its instances by a category type. When
a query's match condition covers that category, every match has to share the
query's classifier, so an absent classifier means an empty result. contains and
iterator already returned one directly, but getAll, keySet, containsKey,
countKey and count fell through to the unclassified path, which scans every
instance to produce a result that was already known to be empty.
That fallback made the scan the common case rather than the exception. An
IntegratedDynamics storage index keeps one classified map per priority level, so
looking up an item touches every level, and every level that does not happen to
hold that item scanned all of its entries.
Measured on the IntegratedDynamics index benchmarks, item-only lookups over
5000 instances spread across 200 positions and 4 priority levels:
index_lookup_item 0.240831 ms/op before
0.001157 ms/op after
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Mxjin31W1Lmq5XK1CCe84v
Stacks carrying no component patch, and stacks whose only component was set back to its default, are the cases most of a storage network consists of. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Mxjin31W1Lmq5XK1CCe84v
A component map hashes its prototype alongside its patch, and the prototype is the item's default components. The item is already in the hash, so hashing its defaults again distinguishes nothing while walking the whole default map. For a plain stack, which carries no patch at all, that walk was the entire cost of the hash, and plain stacks are what most of a storage network consists of. This stays consistent with equality because PatchedDataComponentMap keeps its patch sanitized: setting a component to its default removes it from the patch rather than storing it. Two stacks of one item therefore have equal components exactly when they have equal patches. Vanilla can only report an empty patch by building one, which is free for exactly the stacks this catches, so the common implementation does that and NeoForge overrides it with the direct check. Measured on the IntegratedDynamics index benchmarks, over 5000 plain stacks distinct by item and count: index_lookup_exact 0.002014 -> 0.000988 ms/op index_modification 0.001276 -> 0.000582 ms/op Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Mxjin31W1Lmq5XK1CCe84v
8130dbc to
c676887
Compare
|
|
A It is the same three changes. The five ingredient collection files are byte identical between the branches, so only I re-measured on 1.21.1 rather than assuming, and the result is not the same. On 1.21 the modification benchmarks improve where they regressed here:
The reason is that the 1.21 baseline is far worse to begin with: Also confirmed on 1.21.1 directly rather than assumed: This PR stays open. Merge whichever branch suits your upmerge direction; they are not meant to both land independently. Generated by Claude Code |
3 participants
Three related changes to how ingredient collections find things. The first is the fix; the other two exist because measuring the first exposed them.
1. The ItemStack hash ignored data components
IItemStackHelpers.getItemStackHashCodehashed only the count and the item, while equality compares components in full. Every stack of the same item therefore hashed alike, so the hash-based ingredient collections collapsed into one bucket per item type and each lookup in such a bucket became a linear scan doing full component comparisons.The exclusion made sense when stacks carried NBT tags, which were expensive to hash. Component maps are not.
JFR profile of an unmodified 50 000 stack storage terminal open, 3651 server thread samples:
IngredientInstanceWrapper.equalstoIIngredientMatcher.matchesExactlytoItemMatch.areItemStacksEqualtoDataComparator.compareHashMapbuckets (HashMap$TreeNode.find57.1%,getTreeNode43.3%)Java only treeifies a bucket at eight or more collisions, so a treeified bucket is direct evidence of pathological collisions rather than ordinary hashing cost.
IngredientCollectionPrototypeMap.getPrototypenormalizes the count to 1, which removed the only other varying term and left the hash a function of the item alone.2. Classified lookups scanned everything when the classifier was empty
A single-classified collection partitions instances by a category type. When a query's match condition covers that category, every match has to share the query's classifier, so an absent classifier means an empty result.
containsanditeratoralready returned one directly.getAll,keySet,containsKey,countKeyandcountfell through to the unclassified path, which scans every instance to produce a result already known to be empty.That made the scan the common case rather than the exception. An IntegratedDynamics storage index keeps one classified map per priority level, so an item lookup visits every level, and every level not holding that item scanned all of its entries. It is why item-only lookups were slow before any of this, and why they got slower still once hashing stopped being nearly free.
This is a pre-existing bug, independent of change 1.
3. Plain stacks were hashing their item's default components
A component map hashes its prototype alongside its patch, and the prototype is the item's defaults, which the item already in the hash stands for. Hashing it again distinguishes nothing. For a stack carrying no patch that walk was the entire cost of the hash, and plain stacks are what most of a storage network consists of. Profiling change 1 showed 54.5% of index modification samples inside
DataComponentMap$Builder$SimpleMap.hashCode, which is exactly that.This stays consistent with equality because
PatchedDataComponentMapkeeps its patch sanitized:setremoves an entry equal to the prototype's default rather than storing it, andapplyPatchandfromPatchdo the same. Two stacks of one item therefore have equal components exactly when they have equal patches. Vanilla can only report an empty patch by building one, which costs nothing for exactly the stacks this catches, so that is the common implementation and NeoForge overrides it withisComponentsPatchEmpty.Numbers
IntegratedDynamics index benchmarks,
PERFORMANCE_BENCHMARK_ENABLED=true ./gradlew runGameTestServer, ms per operation. Medians of six whole runs, except the hash-only column which is one run and is shown only to separate the three changes. Theplain,mixed,single_item,few_itemsandheavy_componentsshapes are added in CyclopsMC/IntegratedDynamics#1722.The
plainshape is 5000 stacks with no components at all;mixedgives one in ten a component, which is closer to a real modpack storage. Both end up faster or unchanged on lookups, andplainis unchanged on modification.The regressions, stated plainly
Modification on component-bearing stacks. 1.33x on the realistic mixed shape, about 2x on the spread shape, 4.9x with a large component payload. In absolute terms that is 166 ns, 640 ns and 1.65 microseconds per operation. Run to run spread on these rows is large, so the factors are not well determined; the direction is. On the plain shape, where a modification hashes no components, it is slightly faster than before.
Item-only lookup where one item has thousands of variants, 1.35x. That query has to return every variant, so classification cannot narrow it and each result costs a dearer hash.
Set against them, an item-only lookup on a normal network costs 220 microseconds less, and the collision-heavy shapes are hundreds to thousands of times cheaper. One avoided lookup pays for roughly a thousand modifications.
Effect on a storage terminal
Terminal open on a loopback dedicated server with IntegratedTerminals, medians of seven runs after a discarded warm-up. Times in ms. Scenario A is a first open, B a re-open.
Scaling stops being superlinear: 50x the stacks costs 31x the server work, against about 1000x before. A 50 000 stack storage where every heavy stack sits on one item used to trip the 60 s single-tick watchdog and now costs 336 ms of server time. Bytes on the wire are unchanged.
Caller audit
Every caller of
getItemStackHashCodeacross CyclopsCore, IntegratedDynamics, IntegratedTerminals and the CommonCapabilities API:ItemStackHelpersCommonIItemStackHelpersCraftingHelpersCommon:160, recipe cache keyequalsalready usedItemStack.isSameItemSameComponents, so the hash was less discriminating than the equality it keyed. Strictly improved.ValueObjectTypeItemStack:156(IntegratedDynamics)hashCodefor the ItemStack value type, more correct nowIngredientMatcherItemStack.hash(CommonCapabilities)None depend on component-blind hashing for correctness.
Tests
TestItemStackHelpersHashCode: equal stacks hash equal, different items and counts hash differently, components affect the hash, 1000 component variants of one item produce over 99% distinct hashes, plain stacks still spread over items and counts, a component set back to its default hashes as plain again, and the hash agrees withItemStack.isSameItemSameComponentsplus count over 100 samples.TestSingleClassifiedAbsentClassifier:getAll,keySet,containsKey,countKeyandcountreturn empty for an absent classifier and correct results for a present one, and a counting inner collection asserts that nothing is scanned in the absent case, so the optimization is pinned and not just its answer. A match condition outside the category still scans, which is also asserted../gradlew buildpasses on all three loaders.Notes for review
IngredientMapWrappedAdapter.iterator()callscollection.get(key)per key while iterating the key set, hashing every entry twice. Avoid redundant HashMap lookup in IngredientMapWrappedAdapter.iterator() #232 already fixes that; it applies cleanly on top of this.getItemStackHashCodeis byte identical onmaster-1.21-lts, andIngredientMapSingleClassifiedhas the same fall-through there, so both fixes apply. Given the upmerge direction you may want them there first.What was not verified
hasComponentPatchimplementation, which allocates a patch instance for stacks that carry one. That path was not benchmarked; only NeoForge was.