Sponge
Sponge copied to clipboard
Fix block entity memory leak
Fixed memory leak related to inability to unload block entities when the world is inactive (no players and forced chunks). When the world is inactive, updating and unloading entities (including block entities) takes only 300 ticks (15 seconds). Next, three special arrays for block entities are inflated and hold a huge number of block entities in memory. Now it is guaranteed that every world tick there will be cleanup of block entity arrays.
ServerLevel#tick()
:
boolean active = !this.players.isEmpty() || !this.getForcedChunks().isEmpty();
if (active) {
this.emptyTime = 0;
}
if (active || this.emptyTime++ < 300) {
...
this.tickBlockEntities();
...
}
Level#tickBlockEntities()
:
public final List<BlockEntity> blockEntityList = Lists.newArrayList();
public final List<BlockEntity> tickableBlockEntities = Lists.newArrayList();
protected final List<BlockEntity> blockEntitiesToUnload = Lists.newArrayList();
public void tickBlockEntities() {
...
if (!this.blockEntitiesToUnload.isEmpty()) {
this.tickableBlockEntities.removeAll(this.blockEntitiesToUnload);
this.blockEntityList.removeAll(this.blockEntitiesToUnload);
this.blockEntitiesToUnload.clear();
}
...
}
@Zidane
I don’t think that any conflicts can arise, block entity ticks are almost always called every tick of the world roughly speaking at the end of the ServerLevel#tick()
method, however, as I wrote above, block entity ticks are canceled after all players leave the world and there are no forced chunks, after the server counts 300 cycles (field ServerLevel#emptyTime
), block entity ticks are not executed, including the unloading code in the Level#tickBlockEntities()
header. And we want this data to be unloaded always, not only when block entity ticks are actually executed, therefore I added this code to the end of the ServerLevel#tick()
method, I think even the position does not matter, you can call it even in the header.
To be clear, these collections are emptied after 15 seconds in Vanilla and you're moving it to happen each tick?
To be clear, these collections are emptied after 15 seconds in Vanilla and you're moving it to happen each tick?
They are initially emptied every tick, but as soon as everyone exits the world, the world starts counting 300 ticks, during which it still performs cleaning and block entity ticks in general, but after that the world stops doing anything... I initially confused with this, too thought that 15 seconds is interval.
So, the problem is here that during the inactive state, if a chunk unloads the block entity list doesn't get cleared until the next "active" tick, right?
This is one way of doing it, but it adds an redundant check during the game loop which could be avoided. Could we perhaps mix into the this.emptyTime++ < 300
boolean check to do it then? That way, if the server is detected to be "active", this code will never execute - and performance on an inactive server is way less of a problem than on an active one.
(I was thinking about doing it on the ServerLevel
or ClientLevel
chunk unload methods, but unloading isn't necessarily done on the main thread IIRC and so there is the possibility of CMEs - so don't do that.)
It seemed to me that checking for emptiness is quite cheap, although there are no guarantees. Well, I added a check.
Do I need to add the ability to disable the fix in the optimization configuration category? By analogy, as it is done in #3476. Perhaps I also need to make a separate mixin in the package for optimizations?