From 9ae823ef92ce713161c16f5ed8ff78389b667263 Mon Sep 17 00:00:00 2001 From: Jordan Ramsay Date: Tue, 3 Aug 2021 16:52:00 +1000 Subject: [PATCH 1/2] Patch to stop the java.util.ConcurrentModificationException. --- .../backend/instancing/InstanceManager.java | 38 +++++++++++++++---- 1 file changed, 30 insertions(+), 8 deletions(-) diff --git a/src/main/java/com/jozufozu/flywheel/backend/instancing/InstanceManager.java b/src/main/java/com/jozufozu/flywheel/backend/instancing/InstanceManager.java index 304bac4f0..636a59eb4 100644 --- a/src/main/java/com/jozufozu/flywheel/backend/instancing/InstanceManager.java +++ b/src/main/java/com/jozufozu/flywheel/backend/instancing/InstanceManager.java @@ -3,7 +3,10 @@ package com.jozufozu.flywheel.backend.instancing; import java.util.ArrayList; import java.util.HashMap; import java.util.Map; +import java.util.Objects; import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.locks.ReadWriteLock; +import java.util.concurrent.locks.ReentrantReadWriteLock; import javax.annotation.Nullable; @@ -19,8 +22,12 @@ public abstract class InstanceManager implements MaterialManager.OriginShiftL public final MaterialManager materialManager; - protected final ArrayList queuedAdditions; - protected final ConcurrentHashMap.KeySetView queuedUpdates; + //Thread locks to ensure concurrent access exceptions don't occur + private final ReadWriteLock queuedUpdatesLock = new ReentrantReadWriteLock(); + private final ReadWriteLock queuedAdditionsLock = new ReentrantReadWriteLock(); + + private final ArrayList queuedAdditions; + private final ConcurrentHashMap.KeySetView queuedUpdates; protected final Map instances; protected final Object2ObjectOpenHashMap tickableInstances; @@ -73,11 +80,20 @@ public abstract class InstanceManager implements MaterialManager.OriginShiftL }); } + + + + //suggested replacement ? + //Unable to confirm if the call to update(te) causes updates to the que. + //queuedUpdatesLock.writeLock().lock(); + //* queuedUpdates.forEach(te -> { queuedUpdates.remove(te); - update(te); - }); + });//*/ + //queuedUpdates.forEach(this::update); + //queuedUpdates.clear(); + //queuedUpdatesLock.writeLock().unlock(); } public void beginFrame(ActiveRenderInfo info) { @@ -118,7 +134,9 @@ public abstract class InstanceManager implements MaterialManager.OriginShiftL if (!Backend.getInstance() .canUseInstancing()) return; + queuedAdditionsLock.writeLock().lock(); queuedAdditions.add(obj); + queuedAdditionsLock.writeLock().unlock(); } public void update(T obj) { @@ -144,8 +162,9 @@ public abstract class InstanceManager implements MaterialManager.OriginShiftL public synchronized void queueUpdate(T obj) { if (!Backend.getInstance() .canUseInstancing()) return; - + queuedUpdatesLock.writeLock().lock(); queuedUpdates.add(obj); + queuedUpdatesLock.writeLock().unlock(); } public void onLightUpdate(T obj) { @@ -194,9 +213,12 @@ public abstract class InstanceManager implements MaterialManager.OriginShiftL } protected synchronized void processQueuedAdditions() { - if (queuedAdditions.size() > 0) { - queuedAdditions.forEach(this::addInternal); - queuedAdditions.clear(); + queuedAdditionsLock.writeLock().lock(); + ArrayList queued = new ArrayList<>(queuedAdditions); + queuedAdditions.clear(); + queuedAdditionsLock.writeLock().unlock(); + if (queued.size() > 0) { + queued.forEach(this::addInternal); } } From 7eb0284db95be8a2d4e0e07e4ae3b914ca08f8ec Mon Sep 17 00:00:00 2001 From: Jozufozu Date: Tue, 3 Aug 2021 02:06:41 -0700 Subject: [PATCH 2/2] Simplify synchronization - Use `synchronized` on the queue sets in favor of explicit locks. - Updates and additions work the same now - Move updates processing to separate function --- .../backend/instancing/InstanceManager.java | 72 ++++++++++--------- 1 file changed, 37 insertions(+), 35 deletions(-) diff --git a/src/main/java/com/jozufozu/flywheel/backend/instancing/InstanceManager.java b/src/main/java/com/jozufozu/flywheel/backend/instancing/InstanceManager.java index 636a59eb4..7a8800bc3 100644 --- a/src/main/java/com/jozufozu/flywheel/backend/instancing/InstanceManager.java +++ b/src/main/java/com/jozufozu/flywheel/backend/instancing/InstanceManager.java @@ -1,12 +1,16 @@ package com.jozufozu.flywheel.backend.instancing; import java.util.ArrayList; +import java.util.Collection; import java.util.HashMap; +import java.util.HashSet; import java.util.Map; import java.util.Objects; +import java.util.Set; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.locks.ReadWriteLock; import java.util.concurrent.locks.ReentrantReadWriteLock; +import java.util.function.Consumer; import javax.annotation.Nullable; @@ -14,6 +18,7 @@ import com.jozufozu.flywheel.backend.Backend; import com.jozufozu.flywheel.backend.material.MaterialManager; import it.unimi.dsi.fastutil.objects.Object2ObjectOpenHashMap; +import it.unimi.dsi.fastutil.objects.ObjectArraySet; import net.minecraft.client.renderer.ActiveRenderInfo; import net.minecraft.util.math.BlockPos; import net.minecraft.util.math.vector.Vector3f; @@ -22,12 +27,8 @@ public abstract class InstanceManager implements MaterialManager.OriginShiftL public final MaterialManager materialManager; - //Thread locks to ensure concurrent access exceptions don't occur - private final ReadWriteLock queuedUpdatesLock = new ReentrantReadWriteLock(); - private final ReadWriteLock queuedAdditionsLock = new ReentrantReadWriteLock(); - - private final ArrayList queuedAdditions; - private final ConcurrentHashMap.KeySetView queuedUpdates; + private final Set queuedAdditions; + private final Set queuedUpdates; protected final Map instances; protected final Object2ObjectOpenHashMap tickableInstances; @@ -38,8 +39,8 @@ public abstract class InstanceManager implements MaterialManager.OriginShiftL public InstanceManager(MaterialManager materialManager) { this.materialManager = materialManager; - this.queuedUpdates = ConcurrentHashMap.newKeySet(64); - this.queuedAdditions = new ArrayList<>(64); + this.queuedUpdates = new HashSet<>(64); + this.queuedAdditions = new HashSet<>(64); this.instances = new HashMap<>(); this.dynamicInstances = new Object2ObjectOpenHashMap<>(); @@ -56,6 +57,7 @@ public abstract class InstanceManager implements MaterialManager.OriginShiftL public void tick(double cameraX, double cameraY, double cameraZ) { tick++; + processQueuedUpdates(); // integer camera pos int cX = (int) cameraX; @@ -79,21 +81,6 @@ public abstract class InstanceManager implements MaterialManager.OriginShiftL if ((tick % getUpdateDivisor(dX, dY, dZ)) == 0) instance.tick(); }); } - - - - - //suggested replacement ? - //Unable to confirm if the call to update(te) causes updates to the que. - //queuedUpdatesLock.writeLock().lock(); - //* - queuedUpdates.forEach(te -> { - queuedUpdates.remove(te); - update(te); - });//*/ - //queuedUpdates.forEach(this::update); - //queuedUpdates.clear(); - //queuedUpdatesLock.writeLock().unlock(); } public void beginFrame(ActiveRenderInfo info) { @@ -134,9 +121,9 @@ public abstract class InstanceManager implements MaterialManager.OriginShiftL if (!Backend.getInstance() .canUseInstancing()) return; - queuedAdditionsLock.writeLock().lock(); - queuedAdditions.add(obj); - queuedAdditionsLock.writeLock().unlock(); + synchronized (queuedAdditions) { + queuedAdditions.add(obj); + } } public void update(T obj) { @@ -162,9 +149,9 @@ public abstract class InstanceManager implements MaterialManager.OriginShiftL public synchronized void queueUpdate(T obj) { if (!Backend.getInstance() .canUseInstancing()) return; - queuedUpdatesLock.writeLock().lock(); - queuedUpdates.add(obj); - queuedUpdatesLock.writeLock().unlock(); + synchronized (queuedUpdates) { + queuedUpdates.add(obj); + } } public void onLightUpdate(T obj) { @@ -195,7 +182,6 @@ public abstract class InstanceManager implements MaterialManager.OriginShiftL tickableInstances.clear(); } - @SuppressWarnings("unchecked") @Nullable protected IInstance getInstance(I obj, boolean create) { if (!Backend.getInstance() @@ -212,16 +198,32 @@ public abstract class InstanceManager implements MaterialManager.OriginShiftL } } - protected synchronized void processQueuedAdditions() { - queuedAdditionsLock.writeLock().lock(); - ArrayList queued = new ArrayList<>(queuedAdditions); - queuedAdditions.clear(); - queuedAdditionsLock.writeLock().unlock(); + protected void processQueuedAdditions() { + ArrayList queued; + + synchronized (queuedAdditions) { + queued = new ArrayList<>(queuedAdditions); + queuedAdditions.clear(); + } + if (queued.size() > 0) { queued.forEach(this::addInternal); } } + protected void processQueuedUpdates() { + ArrayList queued; + + synchronized (queuedUpdates) { + queued = new ArrayList<>(queuedUpdates); + queuedUpdates.clear(); + } + + if (queued.size() > 0) { + queued.forEach(this::update); + } + } + protected boolean shouldFrameUpdate(BlockPos worldPos, float lookX, float lookY, float lookZ, int cX, int cY, int cZ) { int dX = worldPos.getX() - cX; int dY = worldPos.getY() - cY;