`maxEntityCollisions` is labeled as a "optimization", but does not actually improve TPS significantly
magneticflux- opened this issue · 2 comments
The problem:
In the course of my analysis of tick cramming lag for Lithium (CaffeineMC/lithium-fabric#260), I found this Carpet setting which at first glance seemed to limit the n^2 nature of overlapping collision checks to a mere k*n where k is the current setting and n is the number of overlapping entities. Unfortunately, this is actually not the case, as I will explain.
The background:
The mixin that implements this feature (here) does prevent collision code from running after k entities have been processed. However, the code it prevents from running isn't actually the code that causes the most lag. This] is actually the code that causes the lag:
In my PR to Lithium, I identified three sources of lag: 1) the bounding box checks for every entity in the chunk section, 2) every collision check getting the entity's health from the DataTracker, and 3) every collision check getting the climb-ability of the block the entity is in. The crucial part about this is that all these expensive checks are done at the very beginning of the method when it queries for a list of all matching colliding entities. This means that the mixin doesn't affect the number of checks, and lag isn't reduced.
The solution:
The mixin should instead limit the initial search of matching colliding entities to the current setting. This is not possible with the current getOtherEntities method, so a new helper method should be created that bails out of the search after reaching the specified limit.
@gnembon, I'm able to get started on a PR for this right away. Just let me know if you have any comments, concerns, or if I should just get started.
Some data to back up my claims in the title:
360 chickens in one place, running /tick entities:
| maxEntityCollisions | mspt (chickens) |
|---|---|
| 0 (unlimited) | 14.59ms |
| 1 | 15.68ms |
| 20 | 15.20ms |
The effect of the setting is less than 5% of the total chicken tick time, so it's completely obscured by random variation in tick times.
I am aware of that - it used to work best when it was first implemented. The 'positive' (IMO) effect is that entities do not jet out unnaturally from crammed spaces, but the optimizations are not there. Feel free to replace the current simple limit with optimized entity search method if you would like that - that would be appreciated.