Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix block entity memory leak #3689

Merged
merged 4 commits into from Aug 22, 2022

Conversation

Lignium
Copy link
Contributor

@Lignium Lignium commented May 17, 2022

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();
    }
    ...
}

@Lignium
Copy link
Contributor Author

Lignium commented May 17, 2022

@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.

@Zidane
Copy link
Member

Zidane commented May 17, 2022

To be clear, these collections are emptied after 15 seconds in Vanilla and you're moving it to happen each tick?

@Lignium
Copy link
Contributor Author

Lignium commented May 17, 2022

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.

@dualspiral
Copy link
Contributor

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.)

@Lignium
Copy link
Contributor Author

Lignium commented May 17, 2022

It seemed to me that checking for emptiness is quite cheap, although there are no guarantees. Well, I added a check.

@Lignium Lignium requested a review from dualspiral May 18, 2022 15:09
@Lignium
Copy link
Contributor Author

Lignium commented Jul 1, 2022

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?

@Lignium Lignium force-pushed the fix/block-entity-memory-leak branch from 18d701a to 5694a25 Compare August 20, 2022 05:49
@Zidane Zidane merged commit 3bf6cb2 into SpongePowered:api-8 Aug 22, 2022
@Lignium Lignium deleted the fix/block-entity-memory-leak branch August 23, 2022 14:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants