Repository navigation
Freeze model locations into an ordered immutable set - #697
BonsUnleashed wants to merge 1 commit into
Conversation
|
The current data structure choices in that code were arrived at over
months/years of tweaking and occasional edge case profiler reports. Have
you checked that this doesn't regress performance of representative model
bake event handlers and that the heap usage is roughly the same? Fastutil
tends to be close to optimal on heap usage since it doesn't need wrapper
objects per entry.
…On Sat., Oct. 3, 2026, 10:57 p.m. BonsUnleashed, ***@***.***> wrote:
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. I have
not timed a model reload with this upstream build.
------------------------------
You can view, comment on, or merge this pull request online at:
#697
Commit Summary
- 973731d
<973731d>
Freeze model locations into an ordered immutable set
File Changes
(1 file <https://github.com/embeddedt/ModernFix/pull/697/files>)
- *M*
src/main/java/org/embeddedt/modernfix/forge/dynresources/ModelBakeEventHelper.java
<https://github.com/embeddedt/ModernFix/pull/697/files#diff-02df0129126b703c9005f2b81f0a26c42b5aed69c7cee3b034644f38d42bd8b4>
(8)
Patch Links:
- https://github.com/embeddedt/ModernFix/pull/697.patch
- https://github.com/embeddedt/ModernFix/pull/697.diff
—
Reply to this email directly, view it on GitHub
<#697?email_source=notifications&email_token=AKHTVAFR37PNGA3JTE24IJT5SG4BTA5CNFSNUABEM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UF42DOMZRGQ3DQNRYGKTHEZLBONXW5KTTOVRHGY3SNFRGKZFFMV3GK3TUVRTG633UMVZF6Y3MNFRWW>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AKHTVAFGNHDFBISCJ5I2LLD5SG4BTAVCNFSNUABFKJSXA33TNF2G64TZHM2TQNBQGE3DSNJQHNEXG43VMU5TKNRZGU3TCNBTGM3KC5QC>
.
Triage notifications, keep track of coding agent tasks and review pull
requests on the go with GitHub Mobile for iOS
<https://github.com/notifications/mobile/ios/AKHTVAECXE4VYCSFUJOD5ZD5SG4BTA5CNFSNUABEM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UF42DOMZRGQ3DQNRYGKTHEZLBONXW5KTTOVRHGY3SNFRGKZFFMV3GK3TUVJTG633UMVZF62LPOM>
and Android
<https://github.com/notifications/mobile/android/AKHTVAB22B22QUTG5FNWZT35SG4BTA5CNFSNUABEM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UF42DOMZRGQ3DQNRYGKTHEZLBONXW5KTTOVRHGY3SNFRGKZFFMV3GK3TUVZTG633UMVZF6YLOMRZG62LE>.
Download it today!
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
|
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. |
|
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:
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. |
|
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. |
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.
compileJavapassed against the current1.20branch 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.