From b0b54e4ef351f86f0fd856b664328a023908e234 Mon Sep 17 00:00:00 2001 From: Aikar Date: Wed, 27 May 2020 01:23:19 -0400 Subject: [PATCH] Optimize Villager Pathfinding - Massive Villager Improvement This change reimplements the entire BehaviorFindPosition method to get rid of all of the streams, and implement the logic in a more sane way. We keep vanilla behavior 100% the same with this change, just wrote more optimal, as we can abort iterating POI's as soon as we find a match.... One slight change is that Minecraft adds a random delay before a POI is attempted again. I've increased the amount of that delay based on the distance to said POI, so farther POI's will not be attempted as often. Additionally, we spiral out, so we favor local POI's before we ever favor farther POI's. We also try to pathfind 1 POI at a time instead of collecting multiple POI's then tossing them all to the pathfinder, so that once we get a match we can return before even looking at other POI's. This benefits us in that ideally, a villager will constantly find the near POI's and not even try to pathfind to the farther POI. Trying to pathfind to distant POI's is what causes significant lag. Other improvements here is to stop spamming the POI manager with empty nullables. Vanilla used them to represent if they needed to load POI data off disk or not. Well, we load POI data async on chunk load, so we have it, and we surely do not ever want to load POI data sync either for unloaded chunks! So this massively reduces object count in the POI hashmaps, resulting in less hash collions, and also less memory use. Additionally, unemployed villagers were using significant time due to major ineffeciency in the code rebuilding data that is static every single invocation for every POI type... So we cache that and only rebuild it if professions change, which should be never unless a plugin manipulates and adds custom professions, which it will handle by rebuilding. --- Spigot-Server-Patches/0003-MC-Dev-fixes.patch | 13 + Spigot-Server-Patches/0004-MC-Utils.patch | 12 + .../0251-Improve-BlockPosition-inlining.patch | 4 +- .../0533-Optimize-Villagers.patch | 272 ++++++++++++++++++ 4 files changed, 299 insertions(+), 2 deletions(-) create mode 100644 Spigot-Server-Patches/0533-Optimize-Villagers.patch diff --git a/Spigot-Server-Patches/0003-MC-Dev-fixes.patch b/Spigot-Server-Patches/0003-MC-Dev-fixes.patch index bf06b080ba..34bd17b8b7 100644 --- a/Spigot-Server-Patches/0003-MC-Dev-fixes.patch +++ b/Spigot-Server-Patches/0003-MC-Dev-fixes.patch @@ -49,6 +49,19 @@ index a3afe60b0d85cf90bf7a170dc0a0b61a796381a7..85f799a713db0c822d46b689010f9f6b } else { System.arraycopy(this.b, 0, au, 0, this.c); if (au.length > this.c) { +diff --git a/src/main/java/net/minecraft/server/BehaviorFindPosition.java b/src/main/java/net/minecraft/server/BehaviorFindPosition.java +index 37006ec1bbb8fa285257edacdf337591595852a2..35eb3a5a61145e94d5b0c77c0eb13bfa46fac23b 100644 +--- a/src/main/java/net/minecraft/server/BehaviorFindPosition.java ++++ b/src/main/java/net/minecraft/server/BehaviorFindPosition.java +@@ -53,7 +53,7 @@ public class BehaviorFindPosition extends Behavior { + villageplace.a(this.a.c(), (blockposition1) -> { + return blockposition1.equals(blockposition); + }, blockposition, 1); +- entitycreature.getBehaviorController().setMemory(this.b, (Object) GlobalPos.create(worldserver.getWorldProvider().getDimensionManager(), blockposition)); ++ entitycreature.getBehaviorController().setMemory(this.b, GlobalPos.create(worldserver.getWorldProvider().getDimensionManager(), blockposition)); + PacketDebug.c(worldserver, blockposition); + }); + } else if (this.f < 5) { diff --git a/src/main/java/net/minecraft/server/BiomeBase.java b/src/main/java/net/minecraft/server/BiomeBase.java index 960dce23072bbb5fad36760677f0fe2efb661552..253890e53702f9ba1c6628cc860a4ca10756626a 100644 --- a/src/main/java/net/minecraft/server/BiomeBase.java diff --git a/Spigot-Server-Patches/0004-MC-Utils.patch b/Spigot-Server-Patches/0004-MC-Utils.patch index 6ebbdd83d1..64fe070438 100644 --- a/Spigot-Server-Patches/0004-MC-Utils.patch +++ b/Spigot-Server-Patches/0004-MC-Utils.patch @@ -2283,6 +2283,18 @@ index 4f60b931a143ebf70a8469913ec445ff13da4d8d..3e90b57b6fd5dcb6cb1325861306e2ff public double a() { double d0 = this.b(); double d1 = this.c(); +diff --git a/src/main/java/net/minecraft/server/BaseBlockPosition.java b/src/main/java/net/minecraft/server/BaseBlockPosition.java +index a3b5793e4824718c8bf3d0a4f963de0ca94a738e..3f09c24e1cd1bba2809b70b1fa6e89773537d834 100644 +--- a/src/main/java/net/minecraft/server/BaseBlockPosition.java ++++ b/src/main/java/net/minecraft/server/BaseBlockPosition.java +@@ -80,6 +80,7 @@ public class BaseBlockPosition implements Comparable { + return this.distanceSquared(iposition.getX(), iposition.getY(), iposition.getZ(), true) < d0 * d0; + } + ++ public final double distanceSquared(BaseBlockPosition baseblockposition) { return m(baseblockposition); } // Paper - OBFHELPER + public double m(BaseBlockPosition baseblockposition) { + return this.distanceSquared((double) baseblockposition.getX(), (double) baseblockposition.getY(), (double) baseblockposition.getZ(), true); + } diff --git a/src/main/java/net/minecraft/server/BlockAccessAir.java b/src/main/java/net/minecraft/server/BlockAccessAir.java index eff6ebcd30b538cbaedaa031a46a59ea956253ba..30cbfc8eac20910aa55951e3dce63862f5a43c37 100644 --- a/src/main/java/net/minecraft/server/BlockAccessAir.java diff --git a/Spigot-Server-Patches/0251-Improve-BlockPosition-inlining.patch b/Spigot-Server-Patches/0251-Improve-BlockPosition-inlining.patch index 2d668ce57b..366f75128a 100644 --- a/Spigot-Server-Patches/0251-Improve-BlockPosition-inlining.patch +++ b/Spigot-Server-Patches/0251-Improve-BlockPosition-inlining.patch @@ -21,7 +21,7 @@ This is based upon conclusions drawn from inspecting the assenmbly generated byt They had 'callq' (invoke) instead of 'mov' (get from memory) instructions. diff --git a/src/main/java/net/minecraft/server/BaseBlockPosition.java b/src/main/java/net/minecraft/server/BaseBlockPosition.java -index 71089442c189336fc0061852a661581784a64013..8f47b6c9e70d10c444ab328dc3bf3a46499b14d5 100644 +index 7b05bb9edcd059a134cef12cc9fea570217bc601..a0450a7ddf21659c5636b3f298e6bf4f0a93fc4d 100644 --- a/src/main/java/net/minecraft/server/BaseBlockPosition.java +++ b/src/main/java/net/minecraft/server/BaseBlockPosition.java @@ -7,32 +7,30 @@ import javax.annotation.concurrent.Immutable; @@ -126,7 +126,7 @@ index 71089442c189336fc0061852a661581784a64013..8f47b6c9e70d10c444ab328dc3bf3a46 } public boolean a(IPosition iposition, double d0) { -@@ -106,9 +107,9 @@ public class BaseBlockPosition implements Comparable { +@@ -107,9 +108,9 @@ public class BaseBlockPosition implements Comparable { } public int n(BaseBlockPosition baseblockposition) { diff --git a/Spigot-Server-Patches/0533-Optimize-Villagers.patch b/Spigot-Server-Patches/0533-Optimize-Villagers.patch new file mode 100644 index 0000000000..d1e4ed9fea --- /dev/null +++ b/Spigot-Server-Patches/0533-Optimize-Villagers.patch @@ -0,0 +1,272 @@ +From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 +From: Aikar +Date: Tue, 26 May 2020 21:32:05 -0400 +Subject: [PATCH] Optimize Villagers + +This change reimplements the entire BehaviorFindPosition method to +get rid of all of the streams, and implement the logic in a more sane way. + +We keep vanilla behavior 100% the same with this change, just wrote more +optimal, as we can abort iterating POI's as soon as we find a match.... + +One slight change is that Minecraft adds a random delay before a POI is +attempted again. I've increased the amount of that delay based on the distance +to said POI, so farther POI's will not be attempted as often. + +Additionally, we spiral out, so we favor local POI's before we ever favor farther POI's. + +We also try to pathfind 1 POI at a time instead of collecting multiple POI's then tossing them +all to the pathfinder, so that once we get a match we can return before even looking at other +POI's. + +This benefits us in that ideally, a villager will constantly find the near POI's and +not even try to pathfind to the farther POI. Trying to pathfind to distant POI's is +what causes significant lag. + +Other improvements here is to stop spamming the POI manager with empty nullables. +Vanilla used them to represent if they needed to load POI data off disk or not. + +Well, we load POI data async on chunk load, so we have it, and we surely do not ever +want to load POI data sync either for unloaded chunks! + +So this massively reduces object count in the POI hashmaps, resulting in less hash collions, +and also less memory use. + +Additionally, unemployed villagers were using significant time due to major ineffeciency in +the code rebuilding data that is static every single invocation for every POI type... + +So we cache that and only rebuild it if professions change, which should be never unless +a plugin manipulates and adds custom professions, which it will handle by rebuilding. + +diff --git a/src/main/java/net/minecraft/server/BehaviorFindPosition.java b/src/main/java/net/minecraft/server/BehaviorFindPosition.java +index 35eb3a5a61145e94d5b0c77c0eb13bfa46fac23b..be85c20bdc7dc10e6389d4ad8738fd5866b83ebd 100644 +--- a/src/main/java/net/minecraft/server/BehaviorFindPosition.java ++++ b/src/main/java/net/minecraft/server/BehaviorFindPosition.java +@@ -29,8 +29,54 @@ public class BehaviorFindPosition extends Behavior { + + protected void a(WorldServer worldserver, EntityCreature entitycreature, long i) { + this.f = 0; +- this.d = worldserver.getTime() + (long) worldserver.getRandom().nextInt(20); ++ this.d = worldserver.getTime() + (long) java.util.concurrent.ThreadLocalRandom.current().nextInt(20); // Paper + VillagePlace villageplace = worldserver.B(); ++ ++ // Paper start - replace implementation completely ++ BlockPosition blockposition2 = new BlockPosition(entitycreature); ++ int dist = 48; ++ int requiredDist = dist * dist; ++ int cdist = Math.floorDiv(dist, 16); ++ Predicate predicate = this.a.c(); ++ int maxPoiAttempts = 4; ++ int poiAttempts = 0; ++ OUT: ++ for (ChunkCoordIntPair chunkcoordintpair : MCUtil.getSpiralOutChunks(blockposition2, cdist)) { ++ for (int i1 = 0; i1 < 16; i1++) { ++ java.util.Optional section = villageplace.getSection(SectionPosition.a(chunkcoordintpair, i1).v()); ++ if (section == null || !section.isPresent()) continue; ++ for (java.util.Map.Entry> e : section.get().getRecords().entrySet()) { ++ if (!predicate.test(e.getKey())) continue; ++ for (VillagePlaceRecord record : e.getValue()) { ++ if (!record.hasVacancy()) continue; ++ ++ BlockPosition pos = record.getPosition(); ++ long key = pos.asLong(); ++ if (this.e.containsKey(key)) { ++ continue; ++ } ++ double poiDist = pos.distanceSquared(blockposition2); ++ if (poiDist <= (double) requiredDist) { ++ this.e.put(key, (long) (this.d + Math.sqrt(poiDist) * 4)); // use dist instead of 40 to blacklist longer if farther distance ++ ++poiAttempts; ++ PathEntity pathentity = entitycreature.getNavigation().a(com.google.common.collect.ImmutableSet.of(pos), 8, false, this.a.d()); ++ ++ if (poiAttempts <= maxPoiAttempts && pathentity != null && pathentity.h()) { ++ record.decreaseVacancy(); ++ GlobalPos globalPos = GlobalPos.create(worldserver.getWorldProvider().getDimensionManager(), pos); ++ entitycreature.getBehaviorController().setMemory(this.b, globalPos); ++ break OUT; ++ } ++ if (poiAttempts > maxPoiAttempts) { ++ this.e.long2LongEntrySet().removeIf((entry) -> entry.getLongValue() < this.d); ++ break OUT; ++ } ++ } ++ } ++ } ++ } ++ } ++ /* + Predicate predicate = (blockposition) -> { + long j = blockposition.asLong(); + +@@ -61,6 +107,6 @@ public class BehaviorFindPosition extends Behavior { + return entry.getLongValue() < this.d; + }); + } +- ++ */ // Paper end + } + } +diff --git a/src/main/java/net/minecraft/server/RegionFileSection.java b/src/main/java/net/minecraft/server/RegionFileSection.java +index a6d8ef5eb44f3f851a3a1be4032ca21ab1d7f2b2..c3e8a0145d63843736d2060f978cdf38df359563 100644 +--- a/src/main/java/net/minecraft/server/RegionFileSection.java ++++ b/src/main/java/net/minecraft/server/RegionFileSection.java +@@ -51,29 +51,15 @@ public class RegionFileSection extends RegionFi + + @Nullable + protected Optional c(long i) { +- return (Optional) this.c.get(i); ++ return this.c.getOrDefault(i, Optional.empty()); // Paper + } + ++ protected final Optional getSection(long i) { return d(i); } // Paper - OBFHELPER + protected Optional d(long i) { +- SectionPosition sectionposition = SectionPosition.a(i); +- +- if (this.b(sectionposition)) { +- return Optional.empty(); +- } else { +- Optional optional = this.c(i); +- +- if (optional != null) { +- return optional; +- } else { +- this.b(sectionposition.u()); +- optional = this.c(i); +- if (optional == null) { +- throw (IllegalStateException) SystemUtils.c(new IllegalStateException()); +- } else { +- return optional; +- } +- } +- } ++ // Paper start - replace method - never load POI data sync, we load this in chunk load already, reduce ops ++ // If it's an unloaded chunk, well too bad. ++ return this.c(i); ++ // Paper end + } + + protected boolean b(SectionPosition sectionposition) { +@@ -117,7 +103,7 @@ public class RegionFileSection extends RegionFi + private void a(ChunkCoordIntPair chunkcoordintpair, DynamicOps dynamicops, @Nullable T t0) { + if (t0 == null) { + for (int i = 0; i < 16; ++i) { +- this.c.put(SectionPosition.a(chunkcoordintpair, i).v(), Optional.empty()); ++ //this.c.put(SectionPosition.a(chunkcoordintpair, i).v(), Optional.empty()); // Paper - NO!!! + } + } else { + Dynamic dynamic = new Dynamic(dynamicops, t0); +@@ -135,7 +121,7 @@ public class RegionFileSection extends RegionFi + }, dynamic2); + }); + +- this.c.put(i1, optional); ++ if (optional.isPresent()) this.c.put(i1, optional); // Paper - NO!!! + optional.ifPresent((minecraftserializable) -> { + this.b(i1); + if (flag) { +@@ -199,7 +185,7 @@ public class RegionFileSection extends RegionFi + if (optional != null && optional.isPresent()) { + this.d.add(i); + } else { +- RegionFileSection.LOGGER.warn("No data for position: {}", SectionPosition.a(i)); ++ //RegionFileSection.LOGGER.warn("No data for position: {}", SectionPosition.a(i)); // Paper - hush + } + } + +diff --git a/src/main/java/net/minecraft/server/VillagePlaceRecord.java b/src/main/java/net/minecraft/server/VillagePlaceRecord.java +index 1e9d7a3f902eb4571b93bb0e58cba966365f07b8..44535a3e2c3320aac472c5a7ee557fac7bab2530 100644 +--- a/src/main/java/net/minecraft/server/VillagePlaceRecord.java ++++ b/src/main/java/net/minecraft/server/VillagePlaceRecord.java +@@ -32,6 +32,7 @@ public class VillagePlaceRecord implements MinecraftSerializable { + return dynamicops.createMap(ImmutableMap.of(dynamicops.createString("pos"), this.a.a(dynamicops), dynamicops.createString("type"), dynamicops.createString(IRegistry.POINT_OF_INTEREST_TYPE.getKey(this.b).toString()), dynamicops.createString("free_tickets"), dynamicops.createInt(this.c))); + } + ++ protected final boolean decreaseVacancy() { return b(); } // Paper - OBFHELPER + protected boolean b() { + if (this.c <= 0) { + return false; +@@ -42,6 +43,7 @@ public class VillagePlaceRecord implements MinecraftSerializable { + } + } + ++ protected final boolean increaseVacancy() { return c(); } // Paper - OBFHELPER + protected boolean c() { + if (this.c >= this.b.b()) { + return false; +@@ -52,14 +54,17 @@ public class VillagePlaceRecord implements MinecraftSerializable { + } + } + ++ public final boolean hasVacancy() { return d(); } // Paper - OBFHELPER + public boolean d() { + return this.c > 0; + } + ++ public final boolean isOccupied() { return e(); } // Paper - OBFHELPER + public boolean e() { + return this.c != this.b.b(); + } + ++ public final BlockPosition getPosition() { return f(); } // Paper + public BlockPosition f() { + return this.a; + } +diff --git a/src/main/java/net/minecraft/server/VillagePlaceSection.java b/src/main/java/net/minecraft/server/VillagePlaceSection.java +index 3f2602dbe0995f8d01d4a1428d919405d711a205..436b064c3b277143075386fc9a71027fb5962681 100644 +--- a/src/main/java/net/minecraft/server/VillagePlaceSection.java ++++ b/src/main/java/net/minecraft/server/VillagePlaceSection.java +@@ -23,7 +23,7 @@ public class VillagePlaceSection implements MinecraftSerializable { + + private static final Logger LOGGER = LogManager.getLogger(); + private final Short2ObjectMap b = new Short2ObjectOpenHashMap(); +- private final Map> c = Maps.newHashMap(); ++ private final Map> c = Maps.newHashMap(); public final Map> getRecords() { return c; } // Paper - OBFHELPER + private final Runnable d; + private boolean e; + +diff --git a/src/main/java/net/minecraft/server/VillagePlaceType.java b/src/main/java/net/minecraft/server/VillagePlaceType.java +index ab3e054cd2f38756a5d802d4d981022318ab047d..c1f293fc98d3efb4665cfb9036f208b842fc8e36 100644 +--- a/src/main/java/net/minecraft/server/VillagePlaceType.java ++++ b/src/main/java/net/minecraft/server/VillagePlaceType.java +@@ -12,8 +12,14 @@ import java.util.stream.Stream; + + public class VillagePlaceType { + ++ static Set professionCache; // Paper + private static final Predicate v = (villageplacetype) -> { +- return ((Set) IRegistry.VILLAGER_PROFESSION.d().map(VillagerProfession::b).collect(Collectors.toSet())).contains(villageplacetype); ++ // Paper start ++ if (professionCache == null) { ++ professionCache = IRegistry.VILLAGER_PROFESSION.d().map(VillagerProfession::b).collect(Collectors.toSet()); ++ } ++ return professionCache.contains(villageplacetype); ++ // Paper end + }; + public static final Predicate a = (villageplacetype) -> { + return true; +@@ -89,11 +95,11 @@ public class VillagePlaceType { + } + + private static VillagePlaceType a(String s, Set set, int i, int j) { +- return a((VillagePlaceType) IRegistry.POINT_OF_INTEREST_TYPE.a(new MinecraftKey(s), (Object) (new VillagePlaceType(s, set, i, j)))); ++ return a((VillagePlaceType) IRegistry.POINT_OF_INTEREST_TYPE.a(new MinecraftKey(s), (new VillagePlaceType(s, set, i, j)))); // Paper - decompile error + } + + private static VillagePlaceType a(String s, Set set, int i, Predicate predicate, int j) { +- return a((VillagePlaceType) IRegistry.POINT_OF_INTEREST_TYPE.a(new MinecraftKey(s), (Object) (new VillagePlaceType(s, set, i, predicate, j)))); ++ return a((VillagePlaceType) IRegistry.POINT_OF_INTEREST_TYPE.a(new MinecraftKey(s), (new VillagePlaceType(s, set, i, predicate, j)))); // Paper - decompile error + } + + private static VillagePlaceType a(VillagePlaceType villageplacetype) { +diff --git a/src/main/java/net/minecraft/server/VillagerProfession.java b/src/main/java/net/minecraft/server/VillagerProfession.java +index c38296165b33698bc15fe49a2de0d0d19cfb910a..f9d7a16c79a4e3ffe8b6e7ed469236a93892f01d 100644 +--- a/src/main/java/net/minecraft/server/VillagerProfession.java ++++ b/src/main/java/net/minecraft/server/VillagerProfession.java +@@ -61,6 +61,7 @@ public class VillagerProfession { + } + + static VillagerProfession a(String s, VillagePlaceType villageplacetype, ImmutableSet immutableset, ImmutableSet immutableset1, @Nullable SoundEffect soundeffect) { ++ VillagePlaceType.professionCache = null; // Paper + return (VillagerProfession) IRegistry.a((IRegistry) IRegistry.VILLAGER_PROFESSION, new MinecraftKey(s), (Object) (new VillagerProfession(s, villageplacetype, immutableset, immutableset1, soundeffect))); + } + }