Repository navigation
Remember the last shared cube per definition - #698
BonsUnleashed wants to merge 1 commit into
Conversation
|
Please share at least some basic numbers on the real-world performance
improvement (not a contrived test). This applies to all current & future
performance PRs.
…On Sat., Oct. 3, 2026, 10:57 p.m. BonsUnleashed, ***@***.***> wrote:
With compact entity models enabled, repeated bakes of a cube definition
construct and hash the same boxed list before finding the cube ModernFix
already shares. This remembers the last result on the definition and checks
the constructor arguments before building that list again.
The shortcut lives inside the existing constructor wrapper. The rest of
bake, including other wrappers around it, still runs. A change to any
constructor argument goes through the existing global cache. Floating-point
comparisons use the same bit-based equality as boxed floats, and visible
faces are copied so an in-place change is noticed. The remembered value is
published as one immutable object.
The equivalent optimization is prepared for Bons and Furious 1.0.30. It
applies only when ModernFix's existing cube-sharing mixin is enabled.
compileJava passed on Java 21. A direct-source harness matched boxed-key
equality in 14,000 checks covering every float field, signed zero, NaNs,
infinities, shared cube identity and in-place face-set changes. I have not
run an in-game model-rendering test of this build.
------------------------------
You can view, comment on, or merge this pull request online at:
#698
Commit Summary
- f19b269
<f19b269>
Remember the last shared cube per definition
File Changes
(2 files <https://github.com/embeddedt/ModernFix/pull/698/files>)
- *M*
src/main/java/org/embeddedt/modernfix/common/mixin/perf/compact_entity_models/CubeDefinitionMixin.java
<https://github.com/embeddedt/ModernFix/pull/698/files#diff-69e053b4f7dc02fc8056e9313fc3568aa1a2066da20fa36ba8bc2bb44ed1fe10>
(9)
- *A* src/main/java/org/embeddedt/modernfix/util/CubeBakeMemo.java
<https://github.com/embeddedt/ModernFix/pull/698/files#diff-8ba2af07259a8768029d6a5625235baf7776d15d5676155d10cc364d6af543a7>
(59)
Patch Links:
- https://github.com/embeddedt/ModernFix/pull/698.patch
- https://github.com/embeddedt/ModernFix/pull/698.diff
—
Reply to this email directly, view it on GitHub
<#698?email_source=notifications&email_token=AKHTVACB42PN37IHVFWQV2D5SG4CBA5CNFSNUABEM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UF42DOMZRGQ3DSMZTGGTHEZLBONXW5KTTOVRHGY3SNFRGKZFFMV3GK3TUVRTG633UMVZF6Y3MNFRWW>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AKHTVACKVBIU7B76472XRAD5SG4CBAVCNFSNUABFKJSXA33TNF2G64TZHM2TQNBQGE3DSNJQHNEXG43VMU5TKNRZGU3TCNJRGI32C5QC>
.
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/AKHTVABOEKGJZ4UFDCWS6DT5SG4CBA5CNFSNUABEM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UF42DOMZRGQ3DSMZTGGTHEZLBONXW5KTTOVRHGY3SNFRGKZFFMV3GK3TUVJTG633UMVZF62LPOM>
and Android
<https://github.com/notifications/mobile/android/AKHTVAH4XNMHK3CEYQDTABL5SG4CBA5CNFSNUABEM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UF42DOMZRGQ3DSMZTGGTHEZLBONXW5KTTOVRHGY3SNFRGKZFFMV3GK3TUVZTG633UMVZF6YLOMRZG62LE>.
Download it today!
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
|
Understood. The 14,000 checks establish correctness; they don't establish a real-world speedup. I don't have client reload measurements for this PR yet. I'll add those measurements for these ModernFix changes, with the mod set and test procedure, and use that requirement for future performance PRs too. |
|
I now have results from actual client loads and resource reloads. They show that the memo is used, but do not establish an overall speedup. With compact entity models explicitly enabled in both baseline and candidate, each of the candidate's three reloads made about 92,000 CubeDefinition calls. About 66,000, or 72%, returned through the new per-definition memo; the remainder used the existing shared-cache path. No repeated bake loop was added to create those hits. Whole resource reloads had a median of 29.46 seconds for this PR versus 29.35 seconds for baseline, with overlapping ranges. This is one candidate process with three reloads, compared with two baseline processes and six reloads, so it is a small sample. During the 30-second equipped-player rendering window there were no CubeDefinition calls at all. That scene gives no evidence of an FPS benefit. The measurements and test code include the exact setup, activation counts, timings and limits. The description now distinguishes these results from the earlier correctness checks. On this evidence I can say the shortcut handles real work, but I can't claim the real-world performance improvement you asked for. |
|
So... this adds 68 lines of complexity and fragility (memoization is easy to get wrong), for no real-world improvement. Sorry but this is not mergeable. |
With compact entity models enabled, repeated bakes of a cube definition construct and hash the same boxed list before finding the cube ModernFix already shares. This remembers the last result on the definition and checks the constructor arguments before building that list again.
The shortcut lives inside the existing constructor wrapper. The rest of
bake, including other wrappers around it, still runs. A change to any constructor argument goes through the existing global cache. Floating-point comparisons use the same bit-based equality as boxed floats, and visible faces are copied so an in-place change is noticed. The remembered value is published as one immutable object.The equivalent optimization is prepared for Bons and Furious 1.0.30. It applies only when ModernFix's existing cube-sharing mixin is enabled.
compileJavapassed on Java 21. A direct-source harness matched boxed-key equality in 14,000 checks covering every float field, signed zero, NaNs, infinities, shared cube identity and in-place face-set changes. In a real full-client run, the memo handled about 72% of CubeDefinition calls during each of three resource reloads. Whole reload timings overlapped baseline, and the equipped-player rendering window made no CubeDefinition calls, so no reload or FPS gain is established. This was an activation/timing test, not a visual comparison. Measurements, source and limitations.