Skip to content

Freeze model locations into an ordered immutable set - #697

Closed
BonsUnleashed wants to merge 1 commit into
embeddedt:1.20from
BonsUnleashed:bons-furious/modernfix-bake-location-order
Closed

BonsUnleashed wants to merge 1 commit into
embeddedt:1.20from
BonsUnleashed:bons-furious/modernfix-bake-location-order

Conversation

@BonsUnleashed

@BonsUnleashed BonsUnleashed commented Oct 4, 2026 •

Copy link
Copy Markdown
Contributor

The emulated model-bake event builds a large ordered set of model locations, then several mods walk that set. The linked hash set keeps insertion order, but its iterator has to follow links through its hash table for every location.

This keeps the current linked set while the constructor gathers locations, then freezes it with ImmutableSet.copyOf. Guava preserves that first-insertion order and uses a compact array for iteration. The set exposed through ModernFix's existing model-map views has the same contents and lookup behavior, while repeated walks avoid following the linked table. Those views already reject modifications.

This follows the model-location work prepared for Bons and Furious 1.0.30. Changing the constructor directly allows a much smaller implementation.

compileJava passed against the current 1.20 branch using Java 21. A full-client ABBA comparison with six completed reloads per arm found a storage/construction tradeoff: the set and backing arrays used about 48% less space, but each constructor allocated about 16.8 MiB more. Combined construction and real handler dispatch took a median 4.14 seconds before and 4.28 seconds after; no overall reload speedup was established. Measurements, source and limitations.

@embeddedt

embeddedt commented Oct 4, 2026 via email

Copy link
Copy Markdown
Owner

@BonsUnleashed

Copy link
Copy Markdown
Contributor Author

Fair question. I haven't yet measured representative bake-event handlers or the retained heap with this change. The compile check alone doesn't answer either concern.

I'm setting up an A/B client model reload with the same mod set, including a retained-heap comparison. I'll post the setup and results here, including any regressions. Thanks for explaining the history behind the current structure.

@BonsUnleashed

Copy link
Copy Markdown
Contributor Author

The full-pack measurements show a space/time tradeoff, not a net reload speedup.

I ran baseline → this PR → this PR → baseline, with three completed resource reloads in each process. The pack has 512 enabled jars, and the event contains about 1.15 million actual model locations. These are the real mods' event handlers, not a repeated synthetic handler.

Medians across six reloads per side:

Work Baseline This PR
ModifyBakingResult handler dispatches 2.82 s 2.24 s
Construction and dispatch, both bake events 4.14 s 4.28 s
Whole resource reload 29.35 s 29.99 s

The faster iteration does help several handlers, but constructing the immutable copy offsets that saving. Not every handler was faster: Dimensional Doors' median rose from about 0.57 to 0.63 seconds. The raw results include every handler and the outliers.

The set and its backing arrays shrink from about 24.0 to 12.4 MiB, excluding shared model-location objects. However, construction allocates about 16.8 MiB more per event because it still builds the fastutil set first. That smaller set is live during the helper's lifetime; the whole-client heap samples were too noisy to establish a retained-heap saving.

Data, instrumentation and reproduction details include the hardware/configuration, all process results and measurement limits. I've updated the description to reflect this.

@embeddedt

Copy link
Copy Markdown
Owner

Thanks for getting that data. With a pack that size, I don't think the 12MB reduction justifies the code change, since it would have to be reverted immediately if the set ever needed to become mutable at runtime. I am also concerned that the time to rehash all the elements would be much worse on a weaker CPU, and therefore the resource reload time would worsen by more than 0.6 seconds.

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