From b797655d0cfc50272dcbaa780953b62a2cfdc9e0 Mon Sep 17 00:00:00 2001 From: Jordan Date: Mon, 13 Jun 2022 22:42:40 +0100 Subject: [PATCH] Only implement cache to MaskingExtent when off main thread (#1789) * Only implement cache to MaskingExtent when off main thread - It's possible for a [Chunk/Char]FilterBlock to be used multiple times in the same "tree" of method calls meaning the mutable paramets (particularly index) get increased within a loop, causing AIOOBs and other issues - This is only possible on the main thread due to the handling of submissions in SingleThreadQueueExtent, as it ensures all operation remains on the main thread to prevent deadlocks - Therefore safe usage of FilterBlocks requires that main-threaded operation create a new [Chunk/Char]FilterBlock instance each time - Further fixes #1681 * Fix typos * Switch to LongFunction --- .../fastasyncworldedit/core/FaweCache.java | 28 +++++++++++++++++++ .../sk89q/worldedit/extent/MaskingExtent.java | 20 ++++++------- 2 files changed, 37 insertions(+), 11 deletions(-) diff --git a/worldedit-core/src/main/java/com/fastasyncworldedit/core/FaweCache.java b/worldedit-core/src/main/java/com/fastasyncworldedit/core/FaweCache.java index c87344f7d..583069ee6 100644 --- a/worldedit-core/src/main/java/com/fastasyncworldedit/core/FaweCache.java +++ b/worldedit-core/src/main/java/com/fastasyncworldedit/core/FaweCache.java @@ -18,6 +18,7 @@ import com.fastasyncworldedit.core.util.collection.CleanableThreadLocal; import com.google.common.cache.CacheBuilder; import com.google.common.cache.CacheLoader; import com.google.common.cache.LoadingCache; +import com.plotsquared.core.util.AnnotationHelper; import com.sk89q.jnbt.ByteArrayTag; import com.sk89q.jnbt.ByteTag; import com.sk89q.jnbt.CompoundTag; @@ -54,6 +55,7 @@ import java.util.concurrent.ThreadPoolExecutor; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicBoolean; import java.util.function.Function; +import java.util.function.LongFunction; import java.util.function.Supplier; import static com.google.common.base.Preconditions.checkNotNull; @@ -137,6 +139,32 @@ public enum FaweCache implements Trimable { }); } + /** + * Create a new cache aimed to act as a thread-cache that is safe to the main server thread. If the method is called away + * from the main thread, it will return a {@link Function} referencing the {@link LoadingCache} returned by + * {@link FaweCache#createCache(Supplier)}. If it is called from the main thread, it will return a {@link Function} that + * will always return the result of the given {@link Supplier}. It is designed to prevent issues caused by + * internally-mutable and resettable classes such as {@link com.fastasyncworldedit.core.extent.filter.block.CharFilterBlock} + * from causing issues when used in edits on the main thread. + * + * @param withInitial The supplier used to determine the initial value if a thread cache is created, else to provide a new + * instance of the class being cached if on the main thread. + * @return a {@link Function} referencing a cache, or the given {@link Supplier} + * @since TODO + */ + @AnnotationHelper.ApiDescription(info = "Designed for specific internal use.") + public LongFunction createMainThreadSafeCache(Supplier withInitial) { + return new LongFunction<>() { + private final LoadingCache loadingCache = Fawe.isMainThread() ? null : FaweCache.INSTANCE.createCache( + withInitial); + + @Override + public V apply(final long input) { + return loadingCache != null ? loadingCache.getUnchecked(input) : withInitial.get(); + } + }; + } + /* Exceptions */ diff --git a/worldedit-core/src/main/java/com/sk89q/worldedit/extent/MaskingExtent.java b/worldedit-core/src/main/java/com/sk89q/worldedit/extent/MaskingExtent.java index 0187e52ca..64b6eecc3 100644 --- a/worldedit-core/src/main/java/com/sk89q/worldedit/extent/MaskingExtent.java +++ b/worldedit-core/src/main/java/com/sk89q/worldedit/extent/MaskingExtent.java @@ -29,15 +29,13 @@ import com.fastasyncworldedit.core.queue.IBatchProcessor; import com.fastasyncworldedit.core.queue.IChunk; import com.fastasyncworldedit.core.queue.IChunkGet; import com.fastasyncworldedit.core.queue.IChunkSet; -import com.google.common.cache.LoadingCache; import com.sk89q.worldedit.WorldEditException; import com.sk89q.worldedit.function.mask.Mask; import com.sk89q.worldedit.math.BlockVector3; import com.sk89q.worldedit.world.biome.BiomeType; import com.sk89q.worldedit.world.block.BlockStateHolder; -import java.util.concurrent.CompletableFuture; -import java.util.concurrent.Future; +import java.util.function.LongFunction; import static com.google.common.base.Preconditions.checkNotNull; @@ -46,9 +44,9 @@ import static com.google.common.base.Preconditions.checkNotNull; */ public class MaskingExtent extends AbstractDelegateExtent implements IBatchProcessor, Filter { - private Mask mask; //FAWE start - private final LoadingCache threadIdToFilter; + private final LongFunction getOrCreateFilterBlock; + private Mask mask; //FAWE end /** @@ -62,16 +60,16 @@ public class MaskingExtent extends AbstractDelegateExtent implements IBatchProce checkNotNull(mask); this.mask = mask; //FAWE start - this.threadIdToFilter = FaweCache.INSTANCE.createCache(() -> new CharFilterBlock(getExtent())); + this.getOrCreateFilterBlock = FaweCache.INSTANCE.createMainThreadSafeCache(() -> new CharFilterBlock(getExtent())); //FAWE end } //FAWE start - private MaskingExtent(Extent extent, Mask mask, LoadingCache threadIdToFilter) { + private MaskingExtent(Extent extent, Mask mask, LongFunction getOrCreateFilterBlock) { super(extent); checkNotNull(mask); this.mask = mask; - this.threadIdToFilter = threadIdToFilter; + this.getOrCreateFilterBlock = getOrCreateFilterBlock; } //FAWE end @@ -107,7 +105,7 @@ public class MaskingExtent extends AbstractDelegateExtent implements IBatchProce @Override public IChunkSet processSet(final IChunk chunk, final IChunkGet get, final IChunkSet set) { - final ChunkFilterBlock filter = threadIdToFilter.getUnchecked(Thread.currentThread().getId()); + final ChunkFilterBlock filter = getOrCreateFilterBlock.apply(Thread.currentThread().getId()); return filter.filter(chunk, get, set, MaskingExtent.this); } @@ -123,12 +121,12 @@ public class MaskingExtent extends AbstractDelegateExtent implements IBatchProce if (child == getExtent()) { return this; } - return new MaskingExtent(child, this.mask.copy(), this.threadIdToFilter); + return new MaskingExtent(child, this.mask.copy(), this.getOrCreateFilterBlock); } @Override public Filter fork() { - return new MaskingExtent(getExtent(), this.mask.copy(), this.threadIdToFilter); + return new MaskingExtent(getExtent(), this.mask.copy(), this.getOrCreateFilterBlock); } @Override