Skip to content

Conversation

@PaintNinja
Copy link
Contributor

Supersedes #66.

This allows for phase tracking to only apply to the requested phases, rather than an 'all or nothing' like previously.

Will run the benchmarks later when I have time.

@PaintNinja PaintNinja added the enhancement New feature or request label Mar 22, 2025
@PaintNinja
Copy link
Contributor Author

Finally got a moment to run the benchmarks. Bare in mind that my hardware has changed, so these results are not directly comparable to other benchmarks I've shown in previous PRs.

Before:

Benchmark                                              Mode  Cnt     Score    Error  Units
BenchmarkModLauncher.Posting.Static.postStatic         avgt    5    10.791 ±  0.019  ns/op
BenchmarkModLauncher.Posting.Static.postStaticDozen    avgt    5    78.311 ±  0.368  ns/op
BenchmarkModLauncher.Posting.Static.postStaticHundred  avgt    5  1019.963 ± 41.705  ns/op
BenchmarkNoLoader.Posting.Static.postStatic            avgt    5    19.522 ±  0.102  ns/op
BenchmarkNoLoader.Posting.Static.postStaticDozen       avgt    5    93.828 ±  4.059  ns/op
BenchmarkNoLoader.Posting.Static.postStaticHundred     avgt    5  1738.962 ± 18.299  ns/op

After:

Benchmark                                              Mode  Cnt     Score    Error  Units
BenchmarkModLauncher.Posting.Static.postStatic         avgt    5     8.419 ±  0.012  ns/op
BenchmarkModLauncher.Posting.Static.postStaticDozen    avgt    5    78.679 ±  0.297  ns/op
BenchmarkModLauncher.Posting.Static.postStaticHundred  avgt    5   981.408 ± 84.251  ns/op
BenchmarkNoLoader.Posting.Static.postStatic            avgt    5    15.278 ±  0.025  ns/op
BenchmarkNoLoader.Posting.Static.postStaticDozen       avgt    5    89.780 ±  1.534  ns/op
BenchmarkNoLoader.Posting.Static.postStaticHundred     avgt    5  1654.612 ± 57.834  ns/op

So roughly 5%-20% faster when phase tracking is disabled or there are no registered listeners for the tracked phases.

@LexManos
Copy link
Member

Not a fan of the default implementation being an exception throw. But guess that does allow for somewhat binary compatibility with older code that doesn't implement those methods.

This looks fine. The performance metrics are dubious at best. But I can see the logic.
Tho it would break any consumer that attempts to perform logic based on phase. Not sure if we care/that exists.

@PaintNinja
Copy link
Contributor Author

The default phase selection remains unchanged for back-compat. Disabling phase tracking disables all phases, and enabling phase tracking enables all phases, same as before.

Perf-wise you're welcome to run the benchmarks and compare yourself. The main benefit is for single listeners as obviously the difference going from posting 2 listeners to 1 listener is larger than going from 12 to 11 or 100 to 99.

If you're happy with this, please approve through GH and I'll merge.

@PaintNinja PaintNinja merged commit 1cac642 into MinecraftForge:6.2.x Apr 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants