PaperMC/Spigot-Server-Patches/0353-Optimize-and-Fix-ExpiringMap-Issues.patch

From c6ea2b8e4435461e2bfd287410e4a0845eb47720 Mon Sep 17 00:00:00 2001
From: Aikar <aikar@aikar.co>
Date: Sun, 16 Sep 2018 00:00:16 -0400
Subject: [PATCH] Optimize and Fix ExpiringMap Issues

computeIfAbsent would leak as the entry was never
registered into the ttl map, as well as some other
methods were at risk, so they were added.

This also synchronizes all access make the map thread safe.

This also redesigns cleaning to not run on every
manipulation, and instead to run clean
once per tick per active expiring map.

diff --git a/src/main/java/net/minecraft/server/ChunkGeneratorAbstract.java b/src/main/java/net/minecraft/server/ChunkGeneratorAbstract.java
index a237684410..835a2aae40 100644
--- a/src/main/java/net/minecraft/server/ChunkGeneratorAbstract.java
+++ b/src/main/java/net/minecraft/server/ChunkGeneratorAbstract.java
@@ -142,13 +142,13 @@ public abstract class ChunkGeneratorAbstract<C extends GeneratorSettings> implem
 
     public Long2ObjectMap<StructureStart> getStructureStartCache(StructureGenerator<? extends WorldGenFeatureConfiguration> structuregenerator) {
         return (Long2ObjectMap) this.d.computeIfAbsent(structuregenerator, (structuregenerator1) -> {
-            return Long2ObjectMaps.synchronize(new ExpiringMap<>(8192, 10000));
+            return new ExpiringMap<>(8192, 10000); // Paper - already synchronized
         });
     }
 
     public Long2ObjectMap<LongSet> getStructureCache(StructureGenerator<? extends WorldGenFeatureConfiguration> structuregenerator) {
         return (Long2ObjectMap) this.e.computeIfAbsent(structuregenerator, (structuregenerator1) -> {
-            return Long2ObjectMaps.synchronize(new ExpiringMap<>(8192, 10000));
+            return new ExpiringMap<>(8192, 10000); // Paper - already synchronized
         });
     }
 
diff --git a/src/main/java/net/minecraft/server/ExpiringMap.java b/src/main/java/net/minecraft/server/ExpiringMap.java
index bd36364796..bf6095137a 100644
--- a/src/main/java/net/minecraft/server/ExpiringMap.java
+++ b/src/main/java/net/minecraft/server/ExpiringMap.java
@@ -2,87 +2,211 @@ package net.minecraft.server;
 
 import it.unimi.dsi.fastutil.longs.Long2LongLinkedOpenHashMap;
 import it.unimi.dsi.fastutil.longs.Long2LongMap;
+import it.unimi.dsi.fastutil.longs.Long2ObjectMaps;
 import it.unimi.dsi.fastutil.longs.Long2ObjectOpenHashMap;
-import it.unimi.dsi.fastutil.longs.Long2LongMap.Entry;
 import it.unimi.dsi.fastutil.objects.ObjectCollection;
 import it.unimi.dsi.fastutil.objects.ObjectIterator;
 import java.util.Map;
+import java.util.function.BiFunction;
+import java.util.function.Function;
 import java.util.function.LongFunction;
 
-public class ExpiringMap<T> extends Long2ObjectOpenHashMap<T> {
-
+public class ExpiringMap<T> extends Long2ObjectMaps.SynchronizedMap<T> { // paper - synchronize accesss
     private final int a;
-    private final Long2LongMap b = new Long2LongLinkedOpenHashMap();
+    private final Long2LongMap ttl = new Long2LongLinkedOpenHashMap(); // Paper
+    private static final boolean DEBUG_EXPIRING_MAP = Boolean.getBoolean("debug.expiringmap");
 
     public ExpiringMap(int i, int j) {
-        super(i);
+        super(new Long2ObjectOpenHashMap<>(i)); // Paper
         this.a = j;
     }
 
+    // Paper start
+    private void setAccess(long i) { a(i); } // Paper - OBFHELPER
     private void a(long i) {
-        long j = SystemUtils.getMonotonicMillis();
+        synchronized (this.sync) {
+            long j = System.currentTimeMillis(); // Paper
+            this.ttl.put(i, j);
+            if (!registered) {
+                registered = true;
+                MinecraftServer.getServer().expiringMaps.add(this);
+            }
+        }
+    }
 
-        this.b.put(i, j);
-        // CraftBukkit start
-        cleanup();
+    @Override
+    public T compute(long l, BiFunction<? super Long, ? super T, ? extends T> biFunction) {
+        setAccess(l);
+        return super.compute(l, biFunction);
     }
 
-    public void cleanup() {
-        long j = SystemUtils.getMonotonicMillis();
-        // CraftBukkit end
-        ObjectIterator objectiterator = this.b.long2LongEntrySet().iterator();
+    @Override
+    public T putIfAbsent(long l, T t) {
+        setAccess(l);
+        return super.putIfAbsent(l, t);
+    }
 
-        while (objectiterator.hasNext()) {
-            Long2LongMap.Entry entry = (Long2LongMap.Entry) objectiterator.next(); // CraftBukkit - decompile error
-            T t0 = super.get(entry.getLongKey());
+    @Override
+    public T computeIfPresent(long l, BiFunction<? super Long, ? super T, ? extends T> biFunction) {
+        setAccess(l);
+        return super.computeIfPresent(l, biFunction);
+    }
 
-            if (j - entry.getLongValue() <= (long) this.a) {
-                break;
-            }
+    @Override
+    public T computeIfAbsent(long l, LongFunction<? extends T> longFunction) {
+        setAccess(l);
+        return super.computeIfAbsent(l, longFunction);
+    }
 
-            if (t0 != null && this.a(t0)) {
-                super.remove(entry.getLongKey());
-                objectiterator.remove();
-            }
+
+    @Override
+    public boolean replace(long l, T t, T v1) {
+        setAccess(l);
+        return super.replace(l, t, v1);
+    }
+
+    @Override
+    public T replace(long l, T t) {
+        setAccess(l);
+        return super.replace(l, t);
+    }
+
+    @Override
+    public T putIfAbsent(Long aLong, T t) {
+        setAccess(aLong);
+        return super.putIfAbsent(aLong, t);
+    }
+
+    @Override
+    public boolean replace(Long aLong, T t, T v1) {
+        setAccess(aLong);
+        return super.replace(aLong, t, v1);
+    }
+
+    @Override
+    public T replace(Long aLong, T t) {
+        setAccess(aLong);
+        return super.replace(aLong, t);
+    }
+
+    @Override
+    public T computeIfAbsent(Long aLong, Function<? super Long, ? extends T> function) {
+        setAccess(aLong);
+        return super.computeIfAbsent(aLong, function);
+    }
+
+    @Override
+    public T computeIfPresent(Long aLong, BiFunction<? super Long, ? super T, ? extends T> biFunction) {
+        setAccess(aLong);
+        return super.computeIfPresent(aLong, biFunction);
+    }
+
+    @Override
+    public T compute(Long aLong, BiFunction<? super Long, ? super T, ? extends T> biFunction) {
+        setAccess(aLong);
+        return super.compute(aLong, biFunction);
+    }
+
+    @Override
+    public void clear() {
+        synchronized (this.sync) {
+            ttl.clear();
+            super.clear();
         }
+    }
 
+    private boolean registered = false;
+
+    // Break clean to its own method to be ticked
+    boolean clean() {
+        synchronized (this.sync) {
+            long now = System.currentTimeMillis();
+            ObjectIterator<Long2LongMap.Entry> objectiterator = this.ttl.long2LongEntrySet().iterator(); // Paper
+
+            while (objectiterator.hasNext()) {
+                Long2LongMap.Entry entry = objectiterator.next(); // Paper
+                T object = super.get(entry.getLongKey()); // Paper
+                if (now - entry.getLongValue() <= (long) this.a) {
+                    break;
+                }
+
+                if (object != null && this.a(object)) {
+                    super.remove(entry.getLongKey());
+                    objectiterator.remove();
+                }
+            }
+            int ttlSize = this.ttl.size();
+            int thisSize = this.size();
+            if (ttlSize < thisSize) {
+                if (DEBUG_EXPIRING_MAP) {
+                    MinecraftServer.LOGGER.warn("WARNING: ExpiringMap desync (ttl:" + ttlSize + " < actual:" + thisSize + ")");
+                }
+                try {
+                    for (Entry<T> entry : this.long2ObjectEntrySet()) {
+                        ttl.putIfAbsent(entry.getLongKey(), now);
+                    }
+                } catch (Exception ignored) {
+                } // Ignore any como's
+            } else if (ttlSize > this.size()) {
+                if (DEBUG_EXPIRING_MAP) {
+                    MinecraftServer.LOGGER.warn("WARNING: ExpiringMap desync (ttl:" + ttlSize + " > actual:" + thisSize + ")");
+                }
+                try {
+                    this.ttl.long2LongEntrySet().removeIf(entry -> !this.containsKey(entry.getLongKey()));
+                } catch (Exception ignored) {
+                } // Ignore any como's
+            }
+            if (isEmpty()) {
+                registered = false;
+                return true;
+            }
+            return false;
+        }
+        // Paper end
     }
 
-    protected boolean a(T t0) {
+    protected boolean a(T var1) {
         return true;
     }
 
-    public T put(long i, T t0) {
+    public T put(long i, T object) {
         this.a(i);
-        return super.put(i, t0);
+        return (T)super.put(i, object);
     }
 
-    public T put(Long olong, T t0) {
+    public T put(Long olong, T object) {
         this.a(olong);
-        return super.put(olong, t0);
+        return (T)super.put(olong, object);
     }
 
     public T get(long i) {
-        this.a(i);
-        return super.get(i);
+        // Paper start - don't setAccess unless a hit
+        T t = super.get(i);
+        if (t != null) {
+            this.setAccess(i);
+        }
+        return t;
+        // Paper end
     }
 
-    public void putAll(Map<? extends Long, ? extends T> map) {
+    public void putAll(Map<? extends Long, ? extends T> var1) {
         throw new RuntimeException("Not implemented");
     }
 
-    public T remove(long i) {
+    public T remove(long var1) {
         throw new RuntimeException("Not implemented");
     }
 
-    public T remove(Object object) {
+    public T remove(Object var1) {
         throw new RuntimeException("Not implemented");
     }
 
+    // Paper start
+    /*
     // CraftBukkit start
     @Override
     public T computeIfAbsent(long l, LongFunction<? extends T> lf) {
-        this.b.put(l, SystemUtils.getMonotonicMillis());
+        this.ttl.put(l, SystemUtils.getMonotonicMillis()); // Paper
         return super.computeIfAbsent(l, lf);
     }
 
@@ -92,4 +216,5 @@ public class ExpiringMap<T> extends Long2ObjectOpenHashMap<T> {
         return super.values();
     }
     // CraftBukkit end
+    */ // Paper end
 }
diff --git a/src/main/java/net/minecraft/server/MinecraftServer.java b/src/main/java/net/minecraft/server/MinecraftServer.java
index 38c0201acb..ae271a57fc 100644
--- a/src/main/java/net/minecraft/server/MinecraftServer.java
+++ b/src/main/java/net/minecraft/server/MinecraftServer.java
@@ -150,6 +150,7 @@ public abstract class MinecraftServer implements IAsyncTaskHandler, IMojangStati
     public int autosavePeriod;
     public File bukkitDataPackFolder;
     public CommandDispatcher vanillaCommandDispatcher;
+    public List<ExpiringMap> expiringMaps = java.util.Collections.synchronizedList(new java.util.ArrayList<>()); // Paper
     // CraftBukkit end
     // Spigot start
     public static final int TPS = 20;
@@ -995,6 +996,7 @@ public abstract class MinecraftServer implements IAsyncTaskHandler, IMojangStati
         this.methodProfiler.exit();
         org.spigotmc.WatchdogThread.tick(); // Spigot
         PaperLightingQueue.processQueue(startTime); // Paper
+        expiringMaps.removeIf(ExpiringMap::clean); // Paper
         this.slackActivityAccountant.tickEnded(l); // Spigot
         co.aikar.timings.TimingsManager.FULL_SERVER_TICK.stopTiming(); // Paper
     }
diff --git a/src/main/java/org/bukkit/craftbukkit/generator/CustomChunkGenerator.java b/src/main/java/org/bukkit/craftbukkit/generator/CustomChunkGenerator.java
index 23f4e24cea..9e553866eb 100644
--- a/src/main/java/org/bukkit/craftbukkit/generator/CustomChunkGenerator.java
+++ b/src/main/java/org/bukkit/craftbukkit/generator/CustomChunkGenerator.java
@@ -159,7 +159,7 @@ public class CustomChunkGenerator extends InternalChunkGenerator<GeneratorSettin
     @Override
     public Long2ObjectMap<StructureStart> getStructureStartCache(StructureGenerator<? extends WorldGenFeatureConfiguration> structuregenerator) {
         return (Long2ObjectMap) this.structureStartCache.computeIfAbsent(structuregenerator, (s) -> {
-            return Long2ObjectMaps.synchronize(new ExpiringMap(8192, 10000));
+            return new ExpiringMap(8192, 10000); // Paper - already synchronized
         });
     }
 
@@ -169,7 +169,7 @@ public class CustomChunkGenerator extends InternalChunkGenerator<GeneratorSettin
     @Override
     public Long2ObjectMap<LongSet> getStructureCache(StructureGenerator<? extends WorldGenFeatureConfiguration> structuregenerator) {
         return (Long2ObjectMap) this.structureCache.computeIfAbsent(structuregenerator, (s) -> {
-            return Long2ObjectMaps.synchronize(new ExpiringMap(8192, 10000));
+            return new ExpiringMap(8192, 10000); // Paper - already synchronized
         });
     }
 
-- 
2.21.0