From c5c909c9afdcdc3b426cd6495fc4036107aeb6e8 Mon Sep 17 00:00:00 2001 From: Aikar Date: Tue, 9 Jun 2020 21:13:51 -0400 Subject: [PATCH] Improve Plugin Ticket Management Now supports async chunk access even though doing that is bad and shouldn't be done anyways since we force you back to main, itll now just delay the ticket add to main the same way. Now only add the ticket if the plugin CAUSED the chunk load, so no longer adds ticket if the chunk was already loaded. Additionally, cap chunk ticket limits to 1 second (Effectively ignoring chunk-gc config unless the config is lower than 20 ticks) Fixes #3533 --- ...-Plugin-Tickets-to-API-Chunk-Methods.patch | 122 ++++++++++++++++++ ...d-PluginTickets-to-API-Chunk-Methods.patch | 65 ---------- 2 files changed, 122 insertions(+), 65 deletions(-) create mode 100644 Spigot-Server-Patches/0539-Add-Plugin-Tickets-to-API-Chunk-Methods.patch delete mode 100644 Spigot-Server-Patches/0539-Add-PluginTickets-to-API-Chunk-Methods.patch diff --git a/Spigot-Server-Patches/0539-Add-Plugin-Tickets-to-API-Chunk-Methods.patch b/Spigot-Server-Patches/0539-Add-Plugin-Tickets-to-API-Chunk-Methods.patch new file mode 100644 index 000000000..63d7a34e4 --- /dev/null +++ b/Spigot-Server-Patches/0539-Add-Plugin-Tickets-to-API-Chunk-Methods.patch @@ -0,0 +1,122 @@ +From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 +From: Aikar +Date: Tue, 9 Jun 2020 03:33:03 -0400 +Subject: [PATCH] Add Plugin Tickets to API Chunk Methods + +Like previous versions, plugins loading chunks kept them loaded until +they garbage collected to avoid constant spamming of chunk loads + +This adds tickets to a few more places so that they can be unloaded. + +Additionally, this drops their ticket level to BORDER so they wont be ticking +so they will just sit inactive instead. + +Using .loadChunk to keep a chunk ticking was a horrible idea for upstream +when we have TWO methods that are able to do that already in the API. + +Also reduce their collection count down to a maximum of 1 second. Barely +anyone knows what chunk-gc is in bukkit.yml as its less relevant now, and +since this wasn't spigot behavior, this is safe to mostly ignore (unless someone +wants it to collect even faster, they can restore that setting back to 1 instead of 20+) + +Not adding it to .getType() though to keep behavior consistent with vanilla for performance reasons. + +diff --git a/src/main/java/org/bukkit/craftbukkit/CraftServer.java b/src/main/java/org/bukkit/craftbukkit/CraftServer.java +index 894917c8891a951edb251f019529d0f7bec7037d..568aefdf698facfc36edacc9332e83196c0d80bc 100644 +--- a/src/main/java/org/bukkit/craftbukkit/CraftServer.java ++++ b/src/main/java/org/bukkit/craftbukkit/CraftServer.java +@@ -318,7 +318,7 @@ public final class CraftServer implements Server { + ambientSpawn = configuration.getInt("spawn-limits.ambient"); + console.autosavePeriod = configuration.getInt("ticks-per.autosave"); + warningState = WarningState.value(configuration.getString("settings.deprecated-verbose")); +- TicketType.PLUGIN.loadPeriod = configuration.getInt("chunk-gc.period-in-ticks"); ++ TicketType.PLUGIN.loadPeriod = Math.min(20, configuration.getInt("chunk-gc.period-in-ticks")); // Paper - cap plugin loads to 1 second + minimumAPI = configuration.getString("settings.minimum-api"); + loadIcon(); + } +@@ -796,7 +796,7 @@ public final class CraftServer implements Server { + waterAnimalSpawn = configuration.getInt("spawn-limits.water-animals"); + ambientSpawn = configuration.getInt("spawn-limits.ambient"); + warningState = WarningState.value(configuration.getString("settings.deprecated-verbose")); +- TicketType.PLUGIN.loadPeriod = configuration.getInt("chunk-gc.period-in-ticks"); ++ TicketType.PLUGIN.loadPeriod = Math.min(20, configuration.getInt("chunk-gc.period-in-ticks")); // Paper - cap plugin loads to 1 second + minimumAPI = configuration.getString("settings.minimum-api"); + printSaveWarning = false; + console.autosavePeriod = configuration.getInt("ticks-per.autosave"); +diff --git a/src/main/java/org/bukkit/craftbukkit/CraftWorld.java b/src/main/java/org/bukkit/craftbukkit/CraftWorld.java +index d3c5b7d1904a6cbd65db406639ed2ba90ec9fd2a..f4a1be34f08551692c1612cffea04fdcf71f2cd4 100644 +--- a/src/main/java/org/bukkit/craftbukkit/CraftWorld.java ++++ b/src/main/java/org/bukkit/craftbukkit/CraftWorld.java +@@ -399,9 +399,22 @@ public class CraftWorld implements World { + + @Override + public Chunk getChunkAt(int x, int z) { +- return this.world.getChunkProvider().getChunkAt(x, z, true).bukkitChunk; ++ // Paper start - add ticket to hold chunk for a little while longer if plugin accesses it ++ net.minecraft.server.Chunk chunk = world.getChunkProvider().getChunkAtIfLoadedImmediately(x, z); ++ if (chunk == null) { ++ addTicket(x, z); ++ chunk = this.world.getChunkProvider().getChunkAt(x, z, true); ++ } ++ return chunk.bukkitChunk; ++ // Paper end + } + ++ // Paper start ++ private void addTicket(int x, int z) { ++ MCUtil.MAIN_EXECUTOR.execute(() -> world.getChunkProvider().addTicket(TicketType.PLUGIN, new ChunkCoordIntPair(x, z), 0, Unit.INSTANCE)); // Paper ++ } ++ // Paper end ++ + @Override + public Chunk getChunkAt(Block block) { + Preconditions.checkArgument(block != null, "null block"); +@@ -476,7 +489,7 @@ public class CraftWorld implements World { + org.spigotmc.AsyncCatcher.catchOp("chunk unload"); // Spigot + net.minecraft.server.IChunkAccess chunk = world.getChunkProvider().getChunkAtIfLoadedImmediately(x, z); // Paper + if (chunk != null) { +- world.getChunkProvider().removeTicket(TicketType.PLUGIN, chunk.getPos(), 1, Unit.INSTANCE); ++ world.getChunkProvider().removeTicket(TicketType.PLUGIN, chunk.getPos(), 0, Unit.INSTANCE); // Paper + } + + return true; +@@ -553,10 +566,12 @@ public class CraftWorld implements World { + org.spigotmc.AsyncCatcher.catchOp("chunk load"); // Spigot + // Paper start - Optimize this method + ChunkCoordIntPair chunkPos = new ChunkCoordIntPair(x, z); ++ IChunkAccess immediate = world.getChunkProvider().getChunkAtIfLoadedImmediately(x, z); // Paper ++ if (immediate != null) return true; // Paper + + if (!generate) { + +- IChunkAccess immediate = world.getChunkProvider().getChunkAtImmediately(x, z); ++ //IChunkAccess immediate = world.getChunkProvider().getChunkAtImmediately(x, z); // Paper + if (immediate == null) { + immediate = world.getChunkProvider().playerChunkMap.getUnloadingChunk(x, z); + } +@@ -564,7 +579,7 @@ public class CraftWorld implements World { + if (!(immediate instanceof ProtoChunkExtension) && !(immediate instanceof net.minecraft.server.Chunk)) { + return false; // not full status + } +- world.getChunkProvider().addTicket(TicketType.PLUGIN, chunkPos, 1, Unit.INSTANCE); ++ world.getChunkProvider().addTicket(TicketType.PLUGIN, chunkPos, 0, Unit.INSTANCE); // Paper + world.getChunkAt(x, z); // make sure we're at ticket level 32 or lower + return true; + } +@@ -591,7 +606,7 @@ public class CraftWorld implements World { + // we do this so we do not re-read the chunk data on disk + } + +- world.getChunkProvider().addTicket(TicketType.PLUGIN, chunkPos, 1, Unit.INSTANCE); ++ world.getChunkProvider().addTicket(TicketType.PLUGIN, chunkPos, 0, Unit.INSTANCE); // Paper + world.getChunkProvider().getChunkAt(x, z, ChunkStatus.FULL, true); + return true; + // Paper end +@@ -2490,6 +2505,7 @@ public class CraftWorld implements World { + } + return this.world.getChunkProvider().getChunkAtAsynchronously(x, z, gen, urgent).thenComposeAsync((either) -> { + net.minecraft.server.Chunk chunk = (net.minecraft.server.Chunk) either.left().orElse(null); ++ if (chunk != null) addTicket(x, z); // Paper + return CompletableFuture.completedFuture(chunk == null ? null : chunk.getBukkitChunk()); + }, MinecraftServer.getServer()); + } diff --git a/Spigot-Server-Patches/0539-Add-PluginTickets-to-API-Chunk-Methods.patch b/Spigot-Server-Patches/0539-Add-PluginTickets-to-API-Chunk-Methods.patch deleted file mode 100644 index fee5cafa9..000000000 --- a/Spigot-Server-Patches/0539-Add-PluginTickets-to-API-Chunk-Methods.patch +++ /dev/null @@ -1,65 +0,0 @@ -From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 -From: Aikar -Date: Tue, 9 Jun 2020 03:33:03 -0400 -Subject: [PATCH] Add PluginTickets to API Chunk Methods - -Like previous versions, plugins loading chunks kept them loaded until -they garbage collected to avoid constant spamming of chunk loads - -This adds tickets to a few more places so that they can be unloaded. - -Additionally, this drops their ticket level to BORDER so they wont be ticking -so they will just sit inactive instead. - -Using .loadChunk to keep a chunk ticking was a horrible idea for upstream -when we have TWO methods that are able to do that already in the API. - -Not adding it to .getType() though to keep behavior consistent with vanilla. - -diff --git a/src/main/java/org/bukkit/craftbukkit/CraftWorld.java b/src/main/java/org/bukkit/craftbukkit/CraftWorld.java -index d3c5b7d1904a6cbd65db406639ed2ba90ec9fd2a..314700041068c79898b555538e8f327693cf9898 100644 ---- a/src/main/java/org/bukkit/craftbukkit/CraftWorld.java -+++ b/src/main/java/org/bukkit/craftbukkit/CraftWorld.java -@@ -399,6 +399,7 @@ public class CraftWorld implements World { - - @Override - public Chunk getChunkAt(int x, int z) { -+ world.getChunkProvider().addTicket(TicketType.PLUGIN, new ChunkCoordIntPair(x, z), 0, Unit.INSTANCE); // Paper - return this.world.getChunkProvider().getChunkAt(x, z, true).bukkitChunk; - } - -@@ -476,7 +477,7 @@ public class CraftWorld implements World { - org.spigotmc.AsyncCatcher.catchOp("chunk unload"); // Spigot - net.minecraft.server.IChunkAccess chunk = world.getChunkProvider().getChunkAtIfLoadedImmediately(x, z); // Paper - if (chunk != null) { -- world.getChunkProvider().removeTicket(TicketType.PLUGIN, chunk.getPos(), 1, Unit.INSTANCE); -+ world.getChunkProvider().removeTicket(TicketType.PLUGIN, chunk.getPos(), 0, Unit.INSTANCE); // Paper - } - - return true; -@@ -564,7 +565,7 @@ public class CraftWorld implements World { - if (!(immediate instanceof ProtoChunkExtension) && !(immediate instanceof net.minecraft.server.Chunk)) { - return false; // not full status - } -- world.getChunkProvider().addTicket(TicketType.PLUGIN, chunkPos, 1, Unit.INSTANCE); -+ world.getChunkProvider().addTicket(TicketType.PLUGIN, chunkPos, 0, Unit.INSTANCE); // Paper - world.getChunkAt(x, z); // make sure we're at ticket level 32 or lower - return true; - } -@@ -591,7 +592,7 @@ public class CraftWorld implements World { - // we do this so we do not re-read the chunk data on disk - } - -- world.getChunkProvider().addTicket(TicketType.PLUGIN, chunkPos, 1, Unit.INSTANCE); -+ world.getChunkProvider().addTicket(TicketType.PLUGIN, chunkPos, 0, Unit.INSTANCE); // Paper - world.getChunkProvider().getChunkAt(x, z, ChunkStatus.FULL, true); - return true; - // Paper end -@@ -2477,6 +2478,7 @@ public class CraftWorld implements World { - if (err != null) { - future.completeExceptionally(err); - } else { -+ world.getChunkProvider().addTicket(TicketType.PLUGIN, new ChunkCoordIntPair(x, z), 0, Unit.INSTANCE); // Paper - future.complete(chunk); - } - });