From 4098309e79424762bc83d552660fe54a4658741b Mon Sep 17 00:00:00 2001 From: MrPowerGamerBR Date: Fri, 17 Nov 2023 19:13:56 -0300 Subject: [PATCH] Refactor MapItem update skip, add synchornized to CraftMapColorCache's "isCached" function --- README.md | 6 ++++-- ...ate-if-the-map-does-not-have-the-Cra.patch | 19 ++++++----------- ...issues-when-using-imageToBytes-in-mu.patch | 21 +++++++++++++++++++ ...Track-how-much-MSPT-each-world-used.patch} | 0 ...atch => 0009-Parallel-world-ticking.patch} | 0 5 files changed, 31 insertions(+), 15 deletions(-) create mode 100644 patches/server/0007-Fix-concurrency-issues-when-using-imageToBytes-in-mu.patch rename patches/server/{0007-Track-how-much-MSPT-each-world-used.patch => 0008-Track-how-much-MSPT-each-world-used.patch} (100%) rename patches/server/{0008-Parallel-world-ticking.patch => 0009-Parallel-world-ticking.patch} (100%) diff --git a/README.md b/README.md index a724e60..694bd91 100644 --- a/README.md +++ b/README.md @@ -21,8 +21,10 @@ SparklyPaper's config file is `sparklypaper.yml`, the file is, by default, place * Skip `distanceToSqr` call in `ServerEntity#sendChanges` if the delta movement hasn't changed * The `distanceToSqr` call is a bit expensive, so avoiding it is pretty nice, around ~15% calls are skipped with this check. Currently, we only check if both Vec3 objects have the same identity, that means, if they are literally the same object. (that works because Minecraft's code reuses the Vec3 object when caching the current delta movement) * Skip `MapItem#update()` if the map does not have the CraftMapRenderer present - * By default, custom maps, even those with custom renderers, are still fetching world data to update the map data. With this change, "image in map" maps can avoid these hefty updates, without requiring the map to be locked, which some old map plugins may not do. - * This has the disadvantage that the vanilla map data will never be updated while the CraftMapRenderer is not present, so if you readd the default renderer, the server will need to update the map data, but that's not a huuuge problem for us. + * By default, maps, even those with custom renderers, fetch the world data to update the map data. With this change, "image in map" maps can avoid these hefty updates, without requiring the map to be locked, which some old map plugins may not do. + * This has the disadvantage that the vanilla map data will never be updated while the CraftMapRenderer is not present, so if you readd the default renderer, the server will need to update the map data, but that's not a huuuge problem, after all, it is a very rare circumstance that you may need the map data to always be up-to-date when you have a custom renderer on the map. +* SparklyPaper - fix concurrency issues when using `imageToBytes` in multiple threads + * Useful if one of your plugins is parallelizng map creation on server startup * 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/0006-Skip-MapItem-update-if-the-map-does-not-have-the-Cra.patch b/patches/server/0006-Skip-MapItem-update-if-the-map-does-not-have-the-Cra.patch index 7b81e73..4760d5c 100644 --- a/patches/server/0006-Skip-MapItem-update-if-the-map-does-not-have-the-Cra.patch +++ b/patches/server/0006-Skip-MapItem-update-if-the-map-does-not-have-the-Cra.patch @@ -9,22 +9,15 @@ Optimizes "image in map" maps, without requiring the map to be locked, which som This has the disadvantage that the vanilla map data will never be updated while the CraftMapRenderer is not present, but that's not a huuuge problem for us diff --git a/src/main/java/net/minecraft/world/item/MapItem.java b/src/main/java/net/minecraft/world/item/MapItem.java -index c368b437597edf7e165326727ae778a69c3fcc83..c54498c662fef50ffb16441e1480f4e048ff99d6 100644 +index c368b437597edf7e165326727ae778a69c3fcc83..190716ea7857c7ad5427a527ad8d6b5a241a00b6 100644 --- a/src/main/java/net/minecraft/world/item/MapItem.java +++ b/src/main/java/net/minecraft/world/item/MapItem.java -@@ -333,7 +333,14 @@ public class MapItem extends ComplexItem { +@@ -332,7 +332,7 @@ public class MapItem extends ComplexItem { + worldmap.tickCarriedBy(entityhuman, stack); } - if (!worldmap.locked && (selected || entity instanceof Player && ((Player) entity).getOffhandItem() == stack)) { -- this.update(world, entity, worldmap); -+ // SparklyPaper - don't update maps if they don't have the CraftMapRenderer in the render list -+ if (!world.sparklyPaperConfig.getSkipMapItemDataUpdatesIfMapDoesNotHaveCraftMapRenderer() || worldmap.mapView.getRenderers().stream().anyMatch(mapRenderer -> mapRenderer.getClass() == org.bukkit.craftbukkit.map.CraftMapRenderer.class)) { -+ this.update(world, entity, worldmap); -+ } else { -+ if (entity instanceof Player) -+ worldmap.getHoldingPlayer((Player) entity); // Required to add the player to the carriedBy list, which causes a map update -+ } -+ // SparklyPaper end +- if (!worldmap.locked && (selected || entity instanceof Player && ((Player) entity).getOffhandItem() == stack)) { ++ if (!worldmap.locked && (!world.sparklyPaperConfig.getSkipMapItemDataUpdatesIfMapDoesNotHaveCraftMapRenderer() || worldmap.mapView.getRenderers().stream().anyMatch(mapRenderer -> mapRenderer.getClass() == org.bukkit.craftbukkit.map.CraftMapRenderer.class)) && (selected || entity instanceof Player && ((Player) entity).getOffhandItem() == stack)) { // SparklyPaper - don't update maps if they don't have the CraftMapRenderer in the render list + this.update(world, entity, worldmap); } - } diff --git a/patches/server/0007-Fix-concurrency-issues-when-using-imageToBytes-in-mu.patch b/patches/server/0007-Fix-concurrency-issues-when-using-imageToBytes-in-mu.patch new file mode 100644 index 0000000..f98d2aa --- /dev/null +++ b/patches/server/0007-Fix-concurrency-issues-when-using-imageToBytes-in-mu.patch @@ -0,0 +1,21 @@ +From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 +From: MrPowerGamerBR +Date: Fri, 17 Nov 2023 19:08:08 -0300 +Subject: [PATCH] Fix concurrency issues when using "imageToBytes" in multiple + threads + +Useful if one of your plugins is parallelizng map creation on server startup + +diff --git a/src/main/java/org/bukkit/craftbukkit/map/CraftMapColorCache.java b/src/main/java/org/bukkit/craftbukkit/map/CraftMapColorCache.java +index 8149b9c51b78eb5c689b7218a2ca3aab60e73bcf..c983d8d7e79d55c9757add8ac1093a0a9d98e5b3 100644 +--- a/src/main/java/org/bukkit/craftbukkit/map/CraftMapColorCache.java ++++ b/src/main/java/org/bukkit/craftbukkit/map/CraftMapColorCache.java +@@ -145,7 +145,7 @@ public class CraftMapColorCache implements MapPalette.MapColorCache { + } + + @Override +- public boolean isCached() { ++ public synchronized boolean isCached() { // SparklyPaper - fix concurrency issues when using "imageToBytes" in multiple threads + return this.cached || (!this.running.get() && this.initCache().isDone()); + } + diff --git a/patches/server/0007-Track-how-much-MSPT-each-world-used.patch b/patches/server/0008-Track-how-much-MSPT-each-world-used.patch similarity index 100% rename from patches/server/0007-Track-how-much-MSPT-each-world-used.patch rename to patches/server/0008-Track-how-much-MSPT-each-world-used.patch diff --git a/patches/server/0008-Parallel-world-ticking.patch b/patches/server/0009-Parallel-world-ticking.patch similarity index 100% rename from patches/server/0008-Parallel-world-ticking.patch rename to patches/server/0009-Parallel-world-ticking.patch