From c80abeb56776943f19494bfa6b5dc3afebf5dff7 Mon Sep 17 00:00:00 2001 From: James Seibel Date: Sat, 11 Jan 2025 13:36:37 -0600 Subject: [PATCH] Fix LodQuadTree render source leaks --- .../render/columnViews/ColumnArrayView.java | 24 ++++++- .../core/render/LodQuadTree.java | 71 +++++++++++++++---- .../core/render/LodRenderSection.java | 15 ++-- 3 files changed, 84 insertions(+), 26 deletions(-) diff --git a/core/src/main/java/com/seibel/distanthorizons/core/dataObjects/render/columnViews/ColumnArrayView.java b/core/src/main/java/com/seibel/distanthorizons/core/dataObjects/render/columnViews/ColumnArrayView.java index a6f807adb..b956d5477 100644 --- a/core/src/main/java/com/seibel/distanthorizons/core/dataObjects/render/columnViews/ColumnArrayView.java +++ b/core/src/main/java/com/seibel/distanthorizons/core/dataObjects/render/columnViews/ColumnArrayView.java @@ -25,6 +25,7 @@ import com.seibel.distanthorizons.core.util.RenderDataPointUtil; import it.unimi.dsi.fastutil.longs.LongArrayList; import java.util.Arrays; +import java.util.ConcurrentModificationException; public final class ColumnArrayView implements IColumnDataView { @@ -53,12 +54,18 @@ public final class ColumnArrayView implements IColumnDataView // constructor // //=============// - public ColumnArrayView(LongArrayList data, int size, int offset, int verticalSize) + /** @throws IllegalArgumentException if the offset is greater than the data's size */ + public ColumnArrayView(LongArrayList data, int size, int offset, int verticalSize) throws IllegalArgumentException { this.data = data; this.size = size; this.offset = offset; this.verticalSize = verticalSize; + + if (this.data.size() < this.offset) + { + throw new IllegalArgumentException("data size ["+this.data.size()+"] is shorter than offset ["+this.offset+"]."); + } } @@ -68,7 +75,20 @@ public final class ColumnArrayView implements IColumnDataView //=====================// @Override - public long get(int index) { return data.getLong(index + offset); } + public long get(int index) + { + try + { + return this.data.getLong(index + this.offset); + } + catch (IndexOutOfBoundsException e) + { + // we can fairly confidently say this is a concurrent exception over an actual + // index out of bounds, since we're generally iterating over the whole + // array any time we use this getter. + throw new ConcurrentModificationException("Potential concurrent modification detected. Make sure the parent ColumnRenderSource isn't being closed before the ColumnArrayView processing is complete.", e); + } + } public void set(int index, long value) { data.set(index + offset, value); } @Override diff --git a/core/src/main/java/com/seibel/distanthorizons/core/render/LodQuadTree.java b/core/src/main/java/com/seibel/distanthorizons/core/render/LodQuadTree.java index eb833fc10..055551b97 100644 --- a/core/src/main/java/com/seibel/distanthorizons/core/render/LodQuadTree.java +++ b/core/src/main/java/com/seibel/distanthorizons/core/render/LodQuadTree.java @@ -47,6 +47,7 @@ import org.apache.logging.log4j.Logger; import javax.annotation.WillNotClose; import java.awt.*; import java.util.*; +import java.util.concurrent.CompletableFuture; import java.util.concurrent.ConcurrentLinkedQueue; import java.util.concurrent.ThreadPoolExecutor; import java.util.concurrent.atomic.AtomicBoolean; @@ -103,10 +104,10 @@ public class LodQuadTree extends QuadTree implements IDebugRen .removalListener((RemovalNotification removalNotification) -> { RemovalCause cause = removalNotification.getCause(); - if (cause == RemovalCause.EXPIRED - || cause == RemovalCause.COLLECTED - || cause == RemovalCause.SIZE - || cause == RemovalCause.EXPLICIT) + if (cause == RemovalCause.EXPLICIT + || cause == RemovalCause.EXPIRED + || cause == RemovalCause.COLLECTED + || cause == RemovalCause.SIZE) { // cleanup needs to be handled on a different thread to prevent locking up the main loading threads ThreadPoolExecutor executor = ThreadPoolUtil.getCleanupExecutor(); @@ -788,29 +789,69 @@ public class LodQuadTree extends QuadTree implements IDebugRen @Override public void close() { - LOGGER.info("Shutting down " + LodQuadTree.class.getSimpleName() + "..."); + LOGGER.info("Shutting down LodQuadTree..."); DebugRenderer.unregister(this, Config.Client.Advanced.Debugging.DebugWireframe.showQuadTreeRenderStatus); - ThreadPoolExecutor executor = ThreadPoolUtil.getCleanupExecutor(); - // closing each node make take a few moments + ThreadPoolExecutor mainCleanupExecutor = ThreadPoolUtil.getCleanupExecutor(); + // closing every node may take a few moments // so this is run on a separate thread to prevent lagging the render thread - executor.execute(() -> + mainCleanupExecutor.execute(() -> { - Iterator> nodeIterator = this.nodeIterator(); - while (nodeIterator.hasNext()) + this.treeReadWriteLock.lock(); + try { - QuadNode quadNode = nodeIterator.next(); - if (quadNode.value != null) + // walk through each node + Iterator> nodeIterator = this.nodeIterator(); + ArrayList> renderDataBuildFutures = new ArrayList<>(); + while (nodeIterator.hasNext()) { - quadNode.value.close(); - quadNode.value = null; + QuadNode quadNode = nodeIterator.next(); + LodRenderSection renderSection = quadNode.value; + if (renderSection != null) + { + // we need to wait for the render data to finish building before we can close the cache + CompletableFuture future = renderSection.getRenderDataBuildFuture(); + if (future != null) + { + renderDataBuildFutures.add(future); + } + + renderSection.close(); + quadNode.value = null; + } } + + + // close the render cache after it is done being used + LOGGER.info("waiting for ["+renderDataBuildFutures.size()+"] futures before closing render cache..."); + CompletableFuture.allOf(renderDataBuildFutures.toArray(new CompletableFuture[0])) + .handle((voidObj, throwable) -> + { + // run on a separate thread so we don't lock up the main cleanup thread + // with the sleep() call + new Thread(() -> + { + // Sleep shouldn't be necessary, but James found a few cases where + // the futures incorrectly claimed they were done. + // Sleeping solved those issues. + try { Thread.sleep(5_000); } catch (InterruptedException ignore) { } + + LOGGER.debug("closing render cache"); + this.cachedRenderSourceByPos.invalidateAll(); + }).start(); + + return null; + }); + } + finally + { + this.treeReadWriteLock.unlock(); } }); - LOGGER.info("Finished shutting down " + LodQuadTree.class.getSimpleName()); + LOGGER.info("Finished shutting down LodQuadTree"); } diff --git a/core/src/main/java/com/seibel/distanthorizons/core/render/LodRenderSection.java b/core/src/main/java/com/seibel/distanthorizons/core/render/LodRenderSection.java index cdf6714b3..ae2c10c4f 100644 --- a/core/src/main/java/com/seibel/distanthorizons/core/render/LodRenderSection.java +++ b/core/src/main/java/com/seibel/distanthorizons/core/render/LodRenderSection.java @@ -70,8 +70,8 @@ public class LodRenderSection implements IDebugRenderable, AutoCloseable @WillNotClose private final FullDataSourceProviderV2 fullDataSourceProvider; private final LodQuadTree quadTree; - private final Cache cachedRenderSourceByPos; private final KeyedLockContainer renderLoadLockContainer; + private final Cache cachedRenderSourceByPos; @@ -86,6 +86,9 @@ public class LodRenderSection implements IDebugRenderable, AutoCloseable * up to the point when geometry data is uploaded to the GPU. */ private CompletableFuture getAndBuildRenderDataFuture = null; + @Nullable + public CompletableFuture getRenderDataBuildFuture() { return this.getAndBuildRenderDataFuture; } + /** * used alongside {@link LodRenderSection#getAndBuildRenderDataFuture} so we can remove * unnecessary tasks from the executor. @@ -254,6 +257,7 @@ public class LodRenderSection implements IDebugRenderable, AutoCloseable return renderSource; } + // generate new render source try (FullDataSourceV2 fullDataSource = this.fullDataSourceProvider.get(pos)) { renderSource = FullDataToRenderDataTransformer.transformFullDataToRenderSource(fullDataSource, this.level); @@ -498,14 +502,8 @@ public class LodRenderSection implements IDebugRenderable, AutoCloseable CompletableFuture buildFuture = this.getAndBuildRenderDataFuture; if (buildFuture != null) { - // Note to self: - // canceling a task prevents it from running, but doesn't allow - // us to purge it from the executor it was queued in. - // As far as James can tell this appears to be a Java bug. - buildFuture.cancel(true); - - // remove the task from our executor if present + // note: don't cancel the task since that prevents cleanup, we just don't want it to run PriorityTaskPicker.Executor executor = ThreadPoolUtil.getFileHandlerExecutor(); if (executor != null && !executor.isTerminated()) { @@ -521,7 +519,6 @@ public class LodRenderSection implements IDebugRenderable, AutoCloseable if (uploadFuture != null) { uploadFuture.cancel(true); - this.bufferUploadFuture = null; }