Skip to content

Commit

Permalink
Fix memory leak in CustomPayloadS2CPacket
Browse files Browse the repository at this point in the history
This is technically a bug in vanilla, however vanilla is saved from this bug due to pure dumb luck: it extensively abuses unpooled heap buffers, so the buffer just gets picked up by the garbage collector and we have nothing to worry about. However, with pooled direct buffers (as with Krypton), the copy is a big problem since it will occupy precious direct buffer space. Worse still, the copy isn't even needed at all. Change it to a slice, which is what it should've been in the first place.

While we're at it, add a new mixin to allow the resource leak detection level to be changed via the normal system property.
  • Loading branch information
astei committed Mar 9, 2022
1 parent a1c142a commit bcde156
Show file tree
Hide file tree
Showing 4 changed files with 38 additions and 6 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
package me.steinborn.krypton.mixin.shared.bugfix;

import io.netty.buffer.ByteBuf;
import net.minecraft.network.PacketByteBuf;
import net.minecraft.network.packet.s2c.play.CustomPayloadS2CPacket;
import org.spongepowered.asm.mixin.Mixin;
import org.spongepowered.asm.mixin.injection.At;
import org.spongepowered.asm.mixin.injection.Redirect;

@Mixin(CustomPayloadS2CPacket.class)
public class CustomPayloadS2CPacketFixMemoryLeakMixin {

@Redirect(method = "getData", at = @At(value = "INVOKE", target = "Lnet/minecraft/network/PacketByteBuf;copy()Lio/netty/buffer/ByteBuf;"))
private ByteBuf getData$copyShouldBeSlice(PacketByteBuf instance) {
return instance.slice();
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
package me.steinborn.krypton.mixin.shared.debugaid;

import io.netty.util.ResourceLeakDetector;
import net.minecraft.SharedConstants;
import org.spongepowered.asm.mixin.Mixin;
import org.spongepowered.asm.mixin.injection.At;
import org.spongepowered.asm.mixin.injection.Redirect;

@Mixin(SharedConstants.class)
public class ResourceLeakDetectorDisableConditionalMixin {
@Redirect(method = "<clinit>", at = @At(value = "INVOKE", target = "Lio/netty/util/ResourceLeakDetector;setLevel(Lio/netty/util/ResourceLeakDetector$Level;)V"))
private static void clinit$resourceLeakDetectorDisableConditional(ResourceLeakDetector.Level level) {
if (System.getProperty("io.netty.leakDetection.level") == null) {
// Allow the user to override the leak detection level in the Minecraft server with the
// io.netty.leakDetection.level system property.
ResourceLeakDetector.setLevel(level);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,6 @@ public class KryptonSharedInitializer implements ModInitializer {
if (System.getProperty("io.netty.allocator.maxOrder") == null) {
System.setProperty("io.netty.allocator.maxOrder", "9");
}

// Disable the resource leak detector by default as it reduces performance. Allow the user to
// override this if desired.
if (System.getProperty("io.netty.leakDetection.level") == null) {
ResourceLeakDetector.setLevel(ResourceLeakDetector.Level.DISABLED);
}
}

@Override
Expand Down
2 changes: 2 additions & 0 deletions src/main/resources/krypton.mixins.json
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
],
"mixins": [
"server.fastchunkentityaccess.ServerWorldMixin",
"shared.bugfix.CustomPayloadS2CPacketFixMemoryLeakMixin",
"shared.debugaid.ResourceLeakDetectorDisableConditionalMixin",
"shared.fastchunkentityaccess.EntityTrackingSectionAccessor",
"shared.fastchunkentityaccess.SectionedEntityCacheMixin",
"shared.network.avoidwork.ThreadedAnvilChunkStorageMixin",
Expand Down

0 comments on commit bcde156

Please sign in to comment.