From 6ef9abeec2fc9be9d2467c1950cfe9d5bf4260e7 Mon Sep 17 00:00:00 2001 From: egg82 Date: Sat, 11 Aug 2018 06:46:46 -0600 Subject: [PATCH] Use ConcurrentHashMap in JsonList & Optimize (#471) (#1309) * [CI-SKIP] add .editorconfig for base code style settings * * Created patch 0349 (fixes #471) * * Made requested modifications * * Made requested modifications (x2) * * Made recommended changes (x3) * * Moved ConcurrentMap return values to Map as no functions specific to ConcurrentMap were used (backing map is still ConcurrentMap) * Removed ConcurrentMap import --- ...49-Use-ConcurrentHashMap-in-JsonList.patch | 114 ++++++++++++++++++ 1 file changed, 114 insertions(+) create mode 100644 Spigot-Server-Patches/0349-Use-ConcurrentHashMap-in-JsonList.patch diff --git a/Spigot-Server-Patches/0349-Use-ConcurrentHashMap-in-JsonList.patch b/Spigot-Server-Patches/0349-Use-ConcurrentHashMap-in-JsonList.patch new file mode 100644 index 0000000000..608df19f2d --- /dev/null +++ b/Spigot-Server-Patches/0349-Use-ConcurrentHashMap-in-JsonList.patch @@ -0,0 +1,114 @@ +From d5626a994903ac7997f1fd6e1efa7ede3d180588 Mon Sep 17 00:00:00 2001 +From: egg82 +Date: Tue, 7 Aug 2018 01:24:23 -0600 +Subject: [PATCH] Use ConcurrentHashMap in JsonList + +This is specifically aimed at fixing #471 + +Using a ConcurrentHashMap because thread safety +The performance benefit of Map over ConcurrentMap is negligabe at best in this scenaio, as most operations will be get and not add or remove +Even without considering the use-case the benefits are still negligable + +Original ideas for the system included an expiration policy and/or handler +The simpler solution was to use a computeIfPresent in the get method +This will simultaneously have an O(1) lookup time and automatically expire any values +Since the get method (nor other similar methods) don't seem to have a critical need to flush the map to disk at any of these points further processing is simply wasteful +Meaning the original function expired values unrelated to the current value without actually having any explicit need to + +The h method was heavily modified to be much more efficient in its processing +Also instead of being called on every get, it's now called just before a save +This will eliminate stale values being flushed to disk + +Modified isEmpty to use the isEmpty() method instead of the slightly confusing size() < 1 +The point of this is readability, but does have a side-benefit of a small microptimization + +Finally, added a couple obfhelpers for the modified code + +diff --git a/src/main/java/net/minecraft/server/JsonList.java b/src/main/java/net/minecraft/server/JsonList.java +index 0859c7eb2..93111cc24 100644 +--- a/src/main/java/net/minecraft/server/JsonList.java ++++ b/src/main/java/net/minecraft/server/JsonList.java +@@ -26,6 +26,7 @@ import java.util.Collection; + import java.util.Iterator; + import java.util.List; + import java.util.Map; ++ + import org.apache.commons.io.IOUtils; + import org.apache.logging.log4j.LogManager; + import org.apache.logging.log4j.Logger; +@@ -35,7 +36,8 @@ public class JsonList> { + protected static final Logger a = LogManager.getLogger(); + protected final Gson b; + private final File c; +- private final Map d = Maps.newHashMap(); ++ // Paper - replace HashMap is ConcurrentHashMap ++ private final Map d = Maps.newConcurrentMap(); private final Map getBackingMap() { return this.d; } // Paper - OBFHELPER + private boolean e = true; + private static final ParameterizedType f = new ParameterizedType() { + public Type[] getActualTypeArguments() { +@@ -83,8 +85,13 @@ public class JsonList> { + } + + public V get(K k0) { +- this.h(); +- return (V) this.d.get(this.a(k0)); // CraftBukkit - fix decompile error ++ // Paper start ++ // this.h(); ++ // return (V) this.d.get(this.a(k0)); // CraftBukkit - fix decompile error ++ return (V) this.getBackingMap().computeIfPresent(this.getMappingKey(k0), (k, v) -> { ++ return v.hasExpired() ? null : v; ++ }); ++ // Paper end + } + + public void remove(K k0) { +@@ -109,9 +116,11 @@ public class JsonList> { + // CraftBukkit end + + public boolean isEmpty() { +- return this.d.size() < 1; ++ // return this.d.size() < 1; // Paper ++ return this.getBackingMap().isEmpty(); // Paper - readability is the goal. As an aside, isEmpty() uses only sumCount() and a comparison. size() uses sumCount(), casts, and boolean logic + } + ++ protected final String getMappingKey(K k0) { return a(k0); } // Paper - OBFHELPER + protected String a(K k0) { + return k0.toString(); + } +@@ -120,8 +129,10 @@ public class JsonList> { + return this.d.containsKey(this.a(k0)); + } + ++ private void removeStaleEntries() { h(); } // Paper - OBFHELPER + private void h() { +- ArrayList arraylist = Lists.newArrayList(); ++ // Paper start ++ /*ArrayList arraylist = Lists.newArrayList(); + Iterator iterator = this.d.values().iterator(); + + while (iterator.hasNext()) { +@@ -138,8 +149,10 @@ public class JsonList> { + Object object = iterator.next(); + + this.d.remove(object); +- } +- ++ }*/ ++ ++ this.getBackingMap().values().removeIf((v) -> v.hasExpired()); ++ // Paper end + } + + protected JsonListEntry a(JsonObject jsonobject) { +@@ -151,6 +164,8 @@ public class JsonList> { + } + + public void save() throws IOException { ++ this.removeStaleEntries(); // Paper - remove expired values before saving ++ + Collection collection = this.d.values(); + String s = this.b.toJson(collection); + BufferedWriter bufferedwriter = null; +-- +2.18.0.windows.1 +