From 9aa5204e1e0b72967b285fd1e939ef9245741c92 Mon Sep 17 00:00:00 2001 From: MrPowerGamerBR Date: Thu, 23 Nov 2023 18:23:18 -0300 Subject: [PATCH] Cache last shouldTickBlocksAt result when ticking block entities --- README.md | 6 ++ ...dTickBlocksAt-result-when-ticking-bl.patch | 58 +++++++++++++++++++ ...Track-how-much-MSPT-each-world-used.patch} | 0 ...atch => 0019-Parallel-world-ticking.patch} | 16 ++--- 4 files changed, 72 insertions(+), 8 deletions(-) create mode 100644 patches/server/0017-Cache-last-shouldTickBlocksAt-result-when-ticking-bl.patch rename patches/server/{0017-Track-how-much-MSPT-each-world-used.patch => 0018-Track-how-much-MSPT-each-world-used.patch} (100%) rename patches/server/{0018-Parallel-world-ticking.patch => 0019-Parallel-world-ticking.patch} (99%) diff --git a/README.md b/README.md index 99edfce..7af742f 100644 --- a/README.md +++ b/README.md @@ -65,6 +65,12 @@ SparklyPaper's config file is `sparklypaper.yml`, the file is, by default, place * First, we change the original `HashMap` to fastutil's `Object2ObjectOpenHashMap`, because fastutil's `containsKey` throughput is better. * Then, we add a `isEmpty()` check before attempting to check if the map contains something. This seems stupid, but it does seem that it improves the performance a bit, and it makes sense, `containsKey(...)` does not attempt to check the map size before attempting to check if the map contains the key. * We also create a `canSee` method tailored for `ChunkMap#updatePlayer()`, a method without the equals check (the `updatePlayer()` already checks if the entity is the same entity) and we cache the `isVisibleByDefault()` result between runs (this also seems a bit overkill because `isVisibleByDefault()` is just a method that returns a boolean, but because this is a hot path, we need all optimizations that we can get!). +* Cache last `shouldTickBlocksAt` result when ticking block entities + * The "shouldTickBlocksAt` call is expensive, it requires pulling chunk holder info from an map for each block entity (even if the block entities are on the same chunk!) every single time + * So here's a quick and dirty optimization: We cache the last `shouldTickBlocksAt` result and, if the last chunk position is the same as our cached value, we use the last cached `shouldTickBlocksAt` result! + * We could use a map for caching, but here's why this is way better than using a map: The block entity ticking list is sorted by chunks! Well, sort of... It is sorted by chunk when the chunk has been loaded *because* all loaded block entities are appended to the list, however, this also means that newly placed blocks will be appended to the end of the list until the chunk unloads and loads again. + * But here's the thing: We don't care if this is bad, the small performance hit of when a player placed new block entities is so small ('tis just a long comparsion after all), that the huge performance boost from already placed block entities is way bigger. + * Most block entities are things that players placed to be there for a long time anyway (like hoppers, etc), and the block entities will be automatically sorted after the chunk is unloaded and loaded again, so it ain't that bad. * Check how much MSPT (milliseconds per tick) each world is using in `/mspt` * Useful to figure out which worlds are lagging your server. ![Per World MSPT](docs/per-world-mspt.png) diff --git a/patches/server/0017-Cache-last-shouldTickBlocksAt-result-when-ticking-bl.patch b/patches/server/0017-Cache-last-shouldTickBlocksAt-result-when-ticking-bl.patch new file mode 100644 index 0000000..ce36880 --- /dev/null +++ b/patches/server/0017-Cache-last-shouldTickBlocksAt-result-when-ticking-bl.patch @@ -0,0 +1,58 @@ +From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 +From: MrPowerGamerBR +Date: Thu, 23 Nov 2023 18:05:00 -0300 +Subject: [PATCH] Cache last shouldTickBlocksAt result when ticking block + entities + +The "shouldTickBlocksAt" call is expensive, it requires pulling chunk holder info from an map for each block entity (even if the block entities are on the same chunk!) every single time + +So here's a quick and dirty optimization: We cache the last "shouldTickBlocksAt" result and, if the last chunk position is the same as our cached value, we use the last cached "shouldTickBlocksAt" result! + +We could use a map for caching, but here's why this is way better than using a map: The block entity ticking list is sorted by chunks! Well, sort of... It is sorted by chunk when the chunk has been loaded, newly placed blocks will be appended to the end of the list until the chunk unloads and loads again + +But here's the thing: We don't care if this is bad, the small performance hit of when a player placed new block entities is so small ('tis just a integer comparsion after all), that the huge performance boost from already placed block entities is way bigger + +Most block entities are things that players placed to be there for a long time anyway (like hoppers, etc) + +diff --git a/src/main/java/net/minecraft/world/level/Level.java b/src/main/java/net/minecraft/world/level/Level.java +index b9e0822638a3979bd43392efdb595153e6f34675..95bc1fab201f5869bb811e25e51ad9c94e955e6e 100644 +--- a/src/main/java/net/minecraft/world/level/Level.java ++++ b/src/main/java/net/minecraft/world/level/Level.java +@@ -1272,6 +1272,10 @@ public abstract class Level implements LevelAccessor, AutoCloseable { + int tilesThisCycle = 0; + var toRemove = new it.unimi.dsi.fastutil.objects.ObjectOpenCustomHashSet(net.minecraft.Util.identityStrategy()); // Paper - use removeAll + toRemove.add(null); ++ // SparklyPaper start - cache last shouldTickBlocksAt result when ticking block entities ++ var shouldTickBlocksAtLastResult = -1; // -1 = undefined ++ var shouldTickBlocksAtChunkPos = 0L; ++ // SparklyPaper end + for (tileTickPosition = 0; tileTickPosition < this.blockEntityTickers.size(); tileTickPosition++) { // Paper - Disable tick limiters + this.tileTickPosition = (this.tileTickPosition < this.blockEntityTickers.size()) ? this.tileTickPosition : 0; + TickingBlockEntity tickingblockentity = (TickingBlockEntity) this.blockEntityTickers.get(this.tileTickPosition); +@@ -1288,13 +1292,25 @@ public abstract class Level implements LevelAccessor, AutoCloseable { + tilesThisCycle--; + toRemove.add(tickingblockentity); // Paper - use removeAll + // Spigot end +- } else if (this.shouldTickBlocksAt(tickingblockentity.getPos())) { ++ // } else if (this.shouldTickBlocksAt(tickingblockentity.getPos())) { // SparklyPaper - cache last shouldTickBlocksAt result when ticking block entities ++ } else { ++ long chunkPos = ChunkPos.asLong(tickingblockentity.getPos()); ++ boolean shouldTick; ++ if (shouldTickBlocksAtLastResult != -1 && shouldTickBlocksAtChunkPos == chunkPos) { ++ shouldTick = shouldTickBlocksAtLastResult == 1; ++ } else { ++ shouldTick = this.shouldTickBlocksAt(chunkPos); ++ shouldTickBlocksAtLastResult = shouldTick ? 1 : 0; ++ shouldTickBlocksAtChunkPos = chunkPos; ++ } ++ if (shouldTick) { + tickingblockentity.tick(); + // Paper start - execute chunk tasks during tick + if ((this.tileTickPosition & 7) == 0) { + MinecraftServer.getServer().executeMidTickTasks(); + } + // Paper end - execute chunk tasks during tick ++ } // SparklyPaper end + } + } + this.blockEntityTickers.removeAll(toRemove); diff --git a/patches/server/0017-Track-how-much-MSPT-each-world-used.patch b/patches/server/0018-Track-how-much-MSPT-each-world-used.patch similarity index 100% rename from patches/server/0017-Track-how-much-MSPT-each-world-used.patch rename to patches/server/0018-Track-how-much-MSPT-each-world-used.patch diff --git a/patches/server/0018-Parallel-world-ticking.patch b/patches/server/0019-Parallel-world-ticking.patch similarity index 99% rename from patches/server/0018-Parallel-world-ticking.patch rename to patches/server/0019-Parallel-world-ticking.patch index f4f22f2..884fdff 100644 --- a/patches/server/0018-Parallel-world-ticking.patch +++ b/patches/server/0019-Parallel-world-ticking.patch @@ -1322,7 +1322,7 @@ index 45243249a561440512ef2a620c60b02e159c80e2..849b9b4336d2ac99324dacf6ad8a2e34 continue; } diff --git a/src/main/java/net/minecraft/world/level/Level.java b/src/main/java/net/minecraft/world/level/Level.java -index b9e0822638a3979bd43392efdb595153e6f34675..28caabc36e7af64d3f546fc0e3c9c06bc5c8b78d 100644 +index 95bc1fab201f5869bb811e25e51ad9c94e955e6e..09ab461affbc20f0748dadd1c80552642201e9cf 100644 --- a/src/main/java/net/minecraft/world/level/Level.java +++ b/src/main/java/net/minecraft/world/level/Level.java @@ -15,6 +15,8 @@ import java.util.function.Consumer; @@ -1351,7 +1351,7 @@ index b9e0822638a3979bd43392efdb595153e6f34675..28caabc36e7af64d3f546fc0e3c9c06b // CraftBukkit start - tree generation if (this.captureTreeGeneration) { // Paper start -@@ -1292,7 +1295,7 @@ public abstract class Level implements LevelAccessor, AutoCloseable { +@@ -1307,7 +1310,7 @@ public abstract class Level implements LevelAccessor, AutoCloseable { tickingblockentity.tick(); // Paper start - execute chunk tasks during tick if ((this.tileTickPosition & 7) == 0) { @@ -1359,8 +1359,8 @@ index b9e0822638a3979bd43392efdb595153e6f34675..28caabc36e7af64d3f546fc0e3c9c06b + // MinecraftServer.getServer().executeMidTickTasks(); // SparklyPaper - parallel world ticking (only run mid-tick at the end of each tick / fixes concurrency bugs related to executeMidTickTasks) } // Paper end - execute chunk tasks during tick - } -@@ -1309,7 +1312,7 @@ public abstract class Level implements LevelAccessor, AutoCloseable { + } // SparklyPaper end +@@ -1325,7 +1328,7 @@ public abstract class Level implements LevelAccessor, AutoCloseable { public void guardEntityTick(Consumer tickConsumer, T entity) { try { tickConsumer.accept(entity); @@ -1369,7 +1369,7 @@ index b9e0822638a3979bd43392efdb595153e6f34675..28caabc36e7af64d3f546fc0e3c9c06b } catch (Throwable throwable) { if (throwable instanceof ThreadDeath) throw throwable; // Paper // Paper start - Prevent tile entity and entity crashes -@@ -1409,6 +1412,7 @@ public abstract class Level implements LevelAccessor, AutoCloseable { +@@ -1425,6 +1428,7 @@ public abstract class Level implements LevelAccessor, AutoCloseable { @Nullable public BlockEntity getBlockEntity(BlockPos blockposition, boolean validate) { @@ -1377,7 +1377,7 @@ index b9e0822638a3979bd43392efdb595153e6f34675..28caabc36e7af64d3f546fc0e3c9c06b // Paper start - Optimize capturedTileEntities lookup net.minecraft.world.level.block.entity.BlockEntity blockEntity; if (!this.capturedTileEntities.isEmpty() && (blockEntity = this.capturedTileEntities.get(blockposition)) != null) { -@@ -1416,10 +1420,11 @@ public abstract class Level implements LevelAccessor, AutoCloseable { +@@ -1432,10 +1436,11 @@ public abstract class Level implements LevelAccessor, AutoCloseable { } // Paper end // CraftBukkit end @@ -1390,7 +1390,7 @@ index b9e0822638a3979bd43392efdb595153e6f34675..28caabc36e7af64d3f546fc0e3c9c06b BlockPos blockposition = blockEntity.getBlockPos(); if (!this.isOutsideBuildHeight(blockposition)) { -@@ -1505,6 +1510,7 @@ public abstract class Level implements LevelAccessor, AutoCloseable { +@@ -1521,6 +1526,7 @@ public abstract class Level implements LevelAccessor, AutoCloseable { @Override public List getEntities(@Nullable Entity except, AABB box, Predicate predicate) { @@ -1398,7 +1398,7 @@ index b9e0822638a3979bd43392efdb595153e6f34675..28caabc36e7af64d3f546fc0e3c9c06b this.getProfiler().incrementCounter("getEntities"); List list = Lists.newArrayList(); ((ServerLevel)this).getEntityLookup().getEntities(except, box, list, predicate); // Paper - optimise this call -@@ -1769,8 +1775,7 @@ public abstract class Level implements LevelAccessor, AutoCloseable { +@@ -1785,8 +1791,7 @@ public abstract class Level implements LevelAccessor, AutoCloseable { } public final BlockPos.MutableBlockPos getRandomBlockPosition(int x, int y, int z, int l, BlockPos.MutableBlockPos out) { // Paper end