Skip to content

Index candidate creative tabs before counting JEI items - #699

Closed
BonsUnleashed wants to merge 2 commits into
embeddedt:1.20from
BonsUnleashed:bons-furious/modernfix-represented-tabs-index
Closed

BonsUnleashed wants to merge 2 commits into
embeddedt:1.20from
BonsUnleashed:bons-furious/modernfix-represented-tabs-index

Conversation

@BonsUnleashed

@BonsUnleashed BonsUnleashed commented Oct 4, 2026 •

Copy link
Copy Markdown
Contributor

ModernFix's first JEI-backed creative search checks every JEI stack against every tab. This builds a temporary index of the tabs containing each item-stack hash, then runs the existing membership check only for possible matches.

Hash collisions still reach the original contains call. The index requires vanilla's exact item set and type-and-tag strategy. Tabs may inherit the stock collection getters; subclasses declaring collection-returning methods keep the original loop. That check does not depend on mapped method names. Empty stacks retain the full check, and tab/stack order is preserved. Nothing is cached across searches or reloads.

The equivalent change is prepared for Bons and Furious 1.0.30. This native revision passed compilation and reobfuscation, plus 321 helper cases in each of two mapping variants. In the actual loaded pack, the index handled 36,142 ingredients and produced exactly the original per-tab counts and 235 represented tabs. The original-loop comparison ran outside the timers.

In one matched client pair, the complete counting operation, including index construction, took 1.54 seconds before and 0.42 seconds after. Thread allocation increased from 50.8 to 54.1 MiB. This is one process per arm, so it is an exploratory result rather than a stable speedup estimate. Raw results, settings and reproduction sources include the limitations and the earlier fallback explanation.

@BonsUnleashed

Copy link
Copy Markdown
Contributor Author

The full-pack test caught an overly cautious fallback here: Mekanism's tabs inherit the standard item-list getters, but the original guard rejected every subclass. I've pushed a follow-up that accepts inherited getters while keeping the original loop for custom collection methods or unfamiliar sets.

The revised native build used the index during an ordinary creative search. An untimed comparison matched every original count and all represented tabs across 36,142 ingredients. The complete operation, including index construction, took 1.54 seconds in the baseline and 0.42 seconds with the revision; allocation rose from 50.8 to 54.1 MiB. That's one fresh process per arm on the same machine, so I wouldn't treat it as a settled speedup estimate.

The results and reproduction sources are here. The earlier measurements remain unchanged as evidence of the fallback, and the PR description now reflects the new check and its allocation cost.

@embeddedt

Copy link
Copy Markdown
Owner

Ok, so there's something here since you got a 4x speedup. There is too much word salad here and the code change is rather large for what it sounds like it does. It would be nice if you could concisely and clearly explain the root cause of the performance issue. Then I can decide whether this is the right approach to solve it

@BonsUnleashed

Copy link
Copy Markdown
Contributor Author

Short version:

getRepresentedTabs() is a nested loop. For every JEI stack it calls tab.getSearchTabDisplayItems().contains(stack) on every tab. In the pack I measured that is 36,142 stacks times 243 tabs, about 8.8 million lookups. Each lookup hashes the stack again, and that hash includes its NBT (CompoundTag.hashCode() walks the whole tag). Nearly all of them miss.

The fix is to hash each stack once and only ask the tabs that hold something with that hash.

The diff is large because of the fallbacks: tabs that return their own collection, sets that are not vanilla's type-and-tag set, empty stacks. Without them it is about ten lines: one map from stack to its tabs, built with the same hash strategy the tab sets use, then one lookup per JEI stack. If you'd rather have that version, say so and I'll swap it in.

And fair enough on #697 and #698. Thanks for looking at the numbers.

@embeddedt

Copy link
Copy Markdown
Owner

So really the issue you've flagged is that ItemStack.hashCode is expensive and not cached? If it was cached, we wouldn't have to bandage around that problem everywhere else, correct?

@BonsUnleashed

Copy link
Copy Markdown
Contributor Author

Yes, repeated content hashing is a large part of the cost here. More precisely, it is the ItemStackLinkedSet.TYPE_AND_TAG strategy hashing the item and its NBT, rather than ItemStack.hashCode() itself. A shared cache could avoid those repeated NBT walks in other callers too.

The complication is invalidation: stacks and their nested NBT can change in place. A persistent cache would need to catch those changes. This PR avoids that problem by keeping its index within one counting operation; it also skips most of the 8.8 million tab-membership checks.

I have not tested a shared hash cache, so the measurements here do not establish that this index is the better approach. My previous explanation should have separated the repeated hashing from the number of lookups.

@embeddedt

Copy link
Copy Markdown
Owner

It's a valid observation but we cannot add 120 lines of bespoke logic at every site that uses TYPE_OR_TAG. I'll look into whether this site can be optimized further in the future but as it stands the provided change is not mergeable.

@embeddedt embeddedt closed this Oct 13, 2026
@BonsUnleashed

Copy link
Copy Markdown
Contributor Author

Understood. The added complexity is too much for this call site. Thanks for reviewing it.

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.

2 participants