neoforged / Bus

Event firing and listening framework, based on the event bus concept
GNU Lesser General Public License v2.1
3 stars 8 forks source link

Trivial optimisations #1

Closed PaintNinja closed 11 months ago

PaintNinja commented 1 year ago

Upstream PR: https://github.com/MinecraftForge/EventBus/pull/52

alcatrazEscapee commented 11 months ago

So, the benchmarks on the original PR are:

Name Base (ns/op) New (ns/op) Change (%)
EventBusBenchmark.testClassLoaderCombined 38.549459 ±1.472707 37.653003 ± 0.907105 -2.32%
EventBusBenchmark.testModLauncherCombined 38.508817 ±1.478818 37.399871 ± 0.603555 -2.87%
EventBusBenchmarkNoLoader.testNoLoaderCombined 145.976518 ± 0.882545 149.555166 ± 2.168568 +2.45%

Notably, the first two are both within the margin of error stated and the final one has gotten slightly worse by more than the margin of error? This doesn't suggest that they are trivial optimizations and rather are trivial refactors with no concrete optimizing potential. Looking at the code, it appears to be:

Is there any theoretical basis for any of these to be actual optimizations? Because from what I remember from that time cpw looked at different loop structures there is no performance improvement to be expected from the idea of "let's replace all for( : ) with for(int i..)!", and so this would just be a refactor into a less clean + readable version. The practical benefit of replacing ArrayList<T> with T[] did not seem to materialize in the results. And the other changes seem like trivial code cleanup tasks, and not optimizations of any sort.

Technici4n commented 11 months ago

These are code style changes if anything.