Skip to content

Hash an ingredient key once per index update - #239

Open
rubensworks wants to merge 1 commit into
master-26-ltsfrom
perf/ingredient-map-compute
Open

Hash an ingredient key once per index update#239
rubensworks wants to merge 1 commit into
master-26-ltsfrom
perf/ingredient-map-compute

Conversation

@rubensworks

@rubensworks rubensworks commented Sep 6, 2026

Copy link
Copy Markdown
Member

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 IngredientMapWrappedAdapter builds its own wrapper inside each of get, put and remove, 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.

IIngredientMapMutable gains a compute method with Map.compute semantics. The default implementation is the old read-then-write, so every existing implementation stays correct without changing. IngredientMapWrappedAdapter builds one wrapper and hands it to the backing map's own compute. IngredientMapSingleClassified delegates to the classifier's sub-map while keeping its size counter and its empty-classifier cleanup.

IngredientCollectionPrototypeMap.add and remove use it. Component types whose hash is cheap see no difference either way.

The companion change on the IntegratedDynamics side is CyclopsMC/IntegratedDynamics#1725, which converts IngredientPositionsIndex.addPosition and removePosition.

Numbers

IntegratedDynamics index benchmarks, ms per operation, medians of six runs each, alternating between the two configurations within one session so that session drift falls on both equally. Both sides carry the fixes from #238, since this only pays off once hashing costs something.

benchmark without compute with compute factor
index_modification_single_item 0.001162 0.000688 41% faster
index_modification_few_items 0.001084 0.000726 33% faster
index_modification_heavy_components 0.002510 0.002077 17% faster
index_modification_mixed 0.000713 0.000667 6% faster
index_modification_plain 0.000530 0.000507 4% faster
index_modification 0.001246 0.001381 11% slower

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.

What that means, honestly

The win is concentrated where hashing genuinely dominates a modification: many component variants of one item, or a large component payload. Those are the shapes a storage network hurts on.

It is much weaker on the realistic shapes. On mixed, which is one instance in ten carrying a component, the median improves 6% and the distributions overlap (without: 0.000649 to 0.000763; with: 0.000593 to 0.000902). On plain it is 4% and likewise inside the spread. The reason is that #238 already skips the component hash entirely for stacks carrying no patch, so on those shapes there is little hashing left to halve.

index_modification on the spread shape reads 11% slower, but that benchmark runs first in the sequence and absorbs JIT warm-up. Its two distributions overlap almost completely (without: 0.001088 to 0.001568; with: 0.000974 to 0.002894, the top value being a clear outlier). I would not read a regression into it, but I am not able to 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.

Recommendation

Worth taking, but not urgently. It is a strict reduction in work, the API mirrors Map.compute so it is not exotic, the default keeps every other implementation correct untouched, and it helps most exactly where large storage networks hurt. But the realistic-mix gain is inside the measurement spread, so if you would rather not widen IIngredientMapMutable for that, closing this is a defensible call.

Tests

TestIngredientMapCompute covers insert, update, remove, a null remapping on an absent key, key pass-through, and size tracking across classifiers, over IngredientHashMap, IngredientTreeMap and IngredientMapSingleClassified. 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.

./gradlew build passes on all three loaders.

Notes for review

  • IngredientMapMultiClassified keeps the default implementation. It is correct, just not collapsed; the positions index never uses it.
  • This is independent of Fix ItemStack hashing and classified ingredient lookups #238 and merges cleanly with it. I trial-merged both onto master locally and nothing conflicted.

@coveralls

coveralls commented Sep 6, 2026

Copy link
Copy Markdown

Coverage Status

coverage: 31.101% (+0.08%) from 31.026% — perf/ingredient-map-compute into master-26-lts

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
@rubensworks
rubensworks force-pushed the perf/ingredient-map-compute branch from f7f3264 to 1ae5111 Compare September 6, 2026 18:15
@sonarqubecloud

sonarqubecloud Bot commented Sep 6, 2026

Copy link
Copy Markdown

@rubensworks
rubensworks marked this pull request as ready for review September 6, 2026 18:20

Copy link
Copy Markdown
Member Author

A master-1.21-lts version of this now exists as #241, since the plan is to upmerge rather than maintain both.

All four touched files are byte identical between the branches, so it is literally the same patch; only the test needed adjusting for JUnit 4. I did not re-run the benchmarks on 1.21, and said so there rather than reusing these numbers silently.

Worth noting for both: on 1.21 the hash fix in #240 makes modifications faster rather than slower, because that baseline collides much harder. The cost this change exists to reduce is therefore a smaller share of the total there, so its benefit on 1.21 is probably smaller than the table in this PR suggests.

This PR stays open. Merge whichever branch suits your upmerge direction.


Generated by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants