FoliaLib
FoliaLib copied to clipboard
TechnicallyCoded
Performance Issue: Unnecessary "sanity check" in `SpigotImplementation#runAtEntityTimer`
We noticed a performance issue in SpigotImplementation#runAtEntityTimer, where the "sanity check" logic is heavy.
This check is redundant on modern Paper versions, where the Folia schedulers are available, and are generally preferred for entity-context task scheduling.
We prepared a patch in mcMMO, which uses this library, and it resulted in a significant performance improvement: https://github.com/CraftYourTown/mcMMO/commit/2cffd64b127678411e20f0b8f9a3e3b87a649ee8
Before:
After:
Just wanted to document this here for visibility. Unfortunately, the library's goal of supporting 1.8.8 and other less than desirable versions prevents me from immediately jumping to upstreaming this change, but I'm happy to participate in helping find a solution 😄
Hey, I could let the developer turn off this check if they know what they are doing. I leave it on by default since it matches the Folia behavior. My goal is to ensure consistent behavior. In fact, if you were to add a flag in the main FoliaLib class (the same way disabling alerts for 0-length ticks is done), I would happily accept this. It would avoid making you maintain a fork.
Is the sanity check truly heavy though? The Spark report may be misleading here. Anyways, I will apply this performance hack to mcMMO and some other changes which may resolve performance issues relating to this.
It's not the first time I've seen this type of spark report. It does seem to include some pretty costly checks weirdly enough.
The "sanity check" is a few field and boolean lookups; in my own instance of this it looked purely like a case of "55k tasks and running on a platform which doesn't support the async profiler just makes isValid a fun safepoint to mash up sampling"; The folia scheduler will perform better here due to the fact that it's not sorting 55k tasks prior to running them
or, at least, the "sanity check" I saw was mostly it just calling CraftEntity#isValid
@electronicboy I seem to remember isValid() and, within isValid(), the call to this.entity.isChunkLoaded() showing up in the profiler a fair few times, and under multiple different plugins. Otherwise yes, I agree the other checks are booleans which cost virtually nothing. Probably since hash tables being fast != free, but I am sure that 55k tasks would not help here either.
In the case of mcMMO specifically, the real long-term fix is likely to attach a scheduler per player instead of tracking the thousands of non-player entities. (I am pretty sure most mcmmo tasks revolve around players). The only problem here is, given the size of the codebase and mishmash of dev styles, it would require a rather intensive rework of mcmmo in ways that I suspect nossr would fear could affect functionality.