Avoid redundant HashMap lookup in IngredientMapWrappedAdapter.iterator() - #232
Open
BlaiseBaptist wants to merge 1 commit into
Conversation
iterator() called collection.get(key) for every key already yielded by collection.keySet().iterator(), a wasted second hash lookup for each element. On a large ingredient map with many distinct data-component variants of the same item, that lookup falls into treeified hash buckets and pays for a full equals() chain (DataComparator -> PatchedDataComponentMap.equals -> component-by-component comparison) per element, per collection iteration. In Integrated Terminals, this iterator is on the hot path for rendering/diffing a storage terminal's ingredient view. On a large, diverse network, iterating it while a slot is actively selected (e.g. while dragging an item) can pin the client render thread at 100%+ CPU for tens of seconds, appearing as a full freeze. Iterating entrySet() directly yields the same (key, value) pairs without the redundant lookup.
|
|
|
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
2 participants
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.
What
IngredientMapWrappedAdapter.iterator()iteratescollection.keySet()and then callscollection.get(key)for every key it already has in hand from the iterator - a redundant second hash lookup per element. This PR switches it to iteratecollection.entrySet()directly, which yields the same(key, value)pairs without the extra lookup. Behavior is unchanged.Why this matters
On a large ingredient map containing many distinct data-component/NBT variants of the same item (enchanted books, AE2 storage cells, backpacks, etc.), those variants collide into the same hash bucket (item hash codes intentionally exclude components/NBT for performance elsewhere). Once a bucket treeifies, the redundant
get()triggers a fullequals()chain per element:DataComparator.compare->PatchedDataComponentMap.equals-> component-by-component comparison. That cost is paid twice per element on every iteration because of the redundant lookup.I ran into this via Integrated Terminals, where this iterator sits on the hot path of
TerminalStorageTabIngredientComponentClientfor building/diffing a storage terminal's ingredient view. On a large, diverse storage network (Sophisticated Storage + AE2 + Refined Storage combined), dragging an item out of a terminal (which keeps a slot actively selected while network-change packets stream in) triggered repeated full rebuilds of this view, pinning the client render thread at 300%+ CPU for a sustained freeze (confirmed viajstackthread dump - happy to share the full stack if useful).This looks related in spirit to CyclopsMC/IntegratedTerminals#139 and #209, though it's a distinct code path from both - it's the "unnecessary duplicate lookup" issue independent of the update-batching/Sophisticated-Storage-specific work done for those.
Testing
Disclosure
Root-cause analysis, the fix, and this PR description were produced with AI assistance (Claude Code), based on a real client-side freeze I hit and profiled myself (jstack thread dump referenced above). I reviewed and verified the change before submitting.