Skip to content

[BUG] Memory leak when IControllable, AbstractRecipeLogic setters don't change state #26

@solidDoWant

Description

@solidDoWant

Describe the bug
When IControllable::setWorkingEnabled, AbstractRecipeLogic::setHasNotEnoughEnergy, or AbstractRecipeLogic::setActive is called, SyncedTileEntityBase::writeCustomData is called unconditionally. This function stores update data in an instance-specific ArrayList<UpdateEntry> updateEntries. Entries in this list are only removed when SyncedTileEntityBase::getUpdatePacket is called.

Via profiling, I found that either getUpdatePacket isn't being called when old state == new state, or it isn't being called frequently enough. As a result, when any of the three bugged functions are called fast enough (this can happen when other mods call these functions hundreds/thousands of times per second), the lists grow without bound. This impacts performance in a couple of ways:

  • Tens of gigabytes of memory can be eaten in a matter of seconds (memory leak)
  • When updates do get processed, the next garbage collection cycle takes a long time because a ton of memory is released (causes freezing/lag spikes)
  • The JVM spends a lot of time managing array list memory, because the growth causes the underlying ArrayList implementation to copy memory as the internal elementData array is repeatedly resized (causing freezing/lag spikes)

There are several things that could be done to improve this (such as using a fixed-size queue instead), but there is one really simple solution: add a single conditional to the implementation of these functions that returns early if there is no change:

Example for setWorkingEnabled

@Override
public void setWorkingEnabled(boolean workingEnabled) {
    if (this.workingEnabled == workingEnabled)
        return;

    this.workingEnabled = workingEnabled;
    metaTileEntity.markDirty();
    World world = metaTileEntity.getWorld();
    if (world != null && !world.isRemote) {
        writeCustomData(1, buf -> {
            buf.writeBoolean(isActive);
            buf.writeBoolean(workingEnabled);
        });
    }
}

This should be a six LoC total change across three functions, and will prevent the problem from affecting GTNE itself or any addon mods. I built a patch for this into my addon mod and it immediately fixed the issue within my mod, but it still affects GTNE itself (as well as any other GTNE addons).

Versions
Forge: 1.12.2
GTCE: 1.18.8
Modpack: Nomi dev branch 4ca1459
Addons: GregTech Energistics master branch

Setup
Playing: Both
New world generated if applicable

Steps To Reproduce

  1. Build a factory, insert valid items into machines
  2. Let it run out of power (so machines have a valid recipe but not enough power to produce)
  3. Attach a profiler an watch SyncedTileEntityBase$UpdateEntry instances grow without bound, because setHasNotEnoughEnergy will be called every tick
  4. Expand your factory to 1-2k machines and watch your computer melt

Expected behavior
Memory usage should be bounded regardless of how many times these functions are called with no actual state change

Screenshots

Initial sample:
Image

A few seconds later:

Image

Additional context

Discussion on Discord here

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions