We had ConcurrentHashMap at home

- Use ConcurrentHashMap in InstancerStorage.
- Fix exception thrown by stack walking while warning about empty models

src/main/java/com/jozufozu/flywheel/backend/engine/InstancerStorage.java

@@ -1,9 +1,9 @@
 package com.jozufozu.flywheel.backend.engine;
 
 import java.util.ArrayList;
-import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
 
 import com.jozufozu.flywheel.Flywheel;
 import com.jozufozu.flywheel.api.event.RenderStage;
@@ -17,78 +17,32 @@ public abstract class InstancerStorage<N extends AbstractInstancer<?>> {
 	 * A map of instancer keys to instancers.
 	 * <br>
 	 * This map is populated as instancers are requested and contains both initialized and uninitialized instancers.
-	 * Write access to this map must be synchronized on {@link #creationLock}.
-	 * <br>
-	 * See {@link #getInstancer} for insertion details.
 	 */
-	protected final Map<InstancerKey<?>, N> instancers = new HashMap<>();
+	protected final Map<InstancerKey<?>, N> instancers = new ConcurrentHashMap<>();
 	/**
 	 * A list of instancers that have not yet been initialized.
 	 * <br>
 	 * All new instancers land here before having resources allocated in {@link #flush}.
-	 * Write access to this list must be synchronized on {@link #creationLock}.
 	 */
-	protected final List<UninitializedInstancer<N, ?>> uninitializedInstancers = new ArrayList<>();
+	protected final List<UninitializedInstancer<N, ?>> initializationQueue = new ArrayList<>();
-	/**
-	 * Mutex for {@link #instancers} and {@link #uninitializedInstancers}.
-	 */
-	protected final Object creationLock = new Object();
 
 	@SuppressWarnings("unchecked")
 	public <I extends Instance> Instancer<I> getInstancer(InstanceType<I> type, Model model, RenderStage stage) {
-		InstancerKey<I> key = new InstancerKey<>(type, model, stage);
+		return (Instancer<I>) instancers.computeIfAbsent(new InstancerKey<>(type, model, stage), this::createAndDeferInit);
-
-		N instancer = instancers.get(key);
-		// Happy path: instancer is already initialized.
-		if (instancer != null) {
-			return (Instancer<I>) instancer;
-		}
-
-		// Unhappy path: instancer is not initialized, need to sync to make sure we don't create duplicates.
-		synchronized (creationLock) {
-			// Someone else might have initialized it while we were waiting for the lock.
-			instancer = instancers.get(key);
-			if (instancer != null) {
-				return (Instancer<I>) instancer;
-			}
-
-			maybeWarnEmptyModel(model);
-
-			// Create a new instancer and add it to the uninitialized list.
-			instancer = create(type);
-			instancers.put(key, instancer);
-			uninitializedInstancers.add(new UninitializedInstancer<>(key, instancer, model, stage));
-			return (Instancer<I>) instancer;
-		}
-	}
-
-	private static void maybeWarnEmptyModel(Model model) {
-		if (!model.meshes()
-				.isEmpty()) {
-			// All is good.
-			return;
-		}
-
-		StringBuilder builder = new StringBuilder();
-		builder.append("Creating an instancer for a model with no meshes! Stack trace:");
-		StackWalker.getInstance()
-				.walk(s -> s.skip(3)) // skip 3 to get back to the caller of InstancerProvider#instancer
-				.forEach(f -> builder.append("\n\t")
-						.append(f.toString()));
-
-		Flywheel.LOGGER.warn(builder.toString());
 	}
 
 	public void delete() {
 		instancers.clear();
-		uninitializedInstancers.clear();
+		initializationQueue.clear();
 	}
 
 	public void flush() {
-		for (var instancer : uninitializedInstancers) {
+		// Thread safety: flush is called from the render thread after all visual updates have been made,
-			add(instancer.key(), instancer.instancer(), instancer.model(), instancer.stage());
+		// so there are no:tm: threads we could be racing with.
+		for (var instancer : initializationQueue) {
+			initialize(instancer.key(), instancer.instancer());
 		}
-		uninitializedInstancers.clear();
+		initializationQueue.clear();
 	}
 
 	public void onRenderOriginChanged() {
@@ -98,8 +52,37 @@ public abstract class InstancerStorage<N extends AbstractInstancer<?>> {
 
 	protected abstract <I extends Instance> N create(InstanceType<I> type);
 
-	protected abstract <I extends Instance> void add(InstancerKey<I> key, N instancer, Model model, RenderStage stage);
+	protected abstract <I extends Instance> void initialize(InstancerKey<I> key, N instancer);
 
-	private record UninitializedInstancer<N, I extends Instance>(InstancerKey<I> key, N instancer, Model model, RenderStage stage) {
+	private N createAndDeferInit(InstancerKey<?> key) {
+		var out = create(key.type());
+
+		// Only queue the instancer for initialization if it has anything to render.
+		if (key.model()
+				.meshes()
+				.isEmpty()) {
+			warnEmptyModel();
+		} else {
+			// Thread safety: this method is called atomically from within computeIfAbsent,
+			// so we don't need extra synchronization to protect the queue.
+			initializationQueue.add(new UninitializedInstancer<>(key, out));
+		}
+		return out;
+	}
+
+	protected record UninitializedInstancer<N, I extends Instance>(InstancerKey<I> key, N instancer) {
+
+	}
+
+	private static void warnEmptyModel() {
+		StringBuilder builder = new StringBuilder();
+		builder.append("Creating an instancer for a model with no meshes! Stack trace:");
+
+		StackWalker.getInstance()
+				// .walk(s -> s.skip(3)) // this causes forEach to crash for some reason
+				.forEach(f -> builder.append("\n\t")
+						.append(f.toString()));
+
+		Flywheel.LOGGER.warn(builder.toString());
 	}
 }

src/main/java/com/jozufozu/flywheel/backend/engine/indirect/IndirectDrawManager.java

@@ -17,7 +17,6 @@ import com.jozufozu.flywheel.api.backend.Engine;
 import com.jozufozu.flywheel.api.event.RenderStage;
 import com.jozufozu.flywheel.api.instance.Instance;
 import com.jozufozu.flywheel.api.instance.InstanceType;
-import com.jozufozu.flywheel.api.model.Model;
 import com.jozufozu.flywheel.backend.compile.IndirectPrograms;
 import com.jozufozu.flywheel.backend.engine.CommonCrumbling;
 import com.jozufozu.flywheel.backend.engine.InstanceHandleImpl;
@@ -52,15 +51,9 @@ public class IndirectDrawManager extends InstancerStorage<IndirectInstancer<?>>
 
 	@SuppressWarnings("unchecked")
 	@Override
-	protected <I extends Instance> void add(InstancerKey<I> key, IndirectInstancer<?> instancer, Model model, RenderStage stage) {
+	protected <I extends Instance> void initialize(InstancerKey<I> key, IndirectInstancer<?> instancer) {
-		if (model.meshes()
-				.isEmpty()) {
-			// Don't bother allocating resources for models with no meshes.
-			return;
-		}
-
 		var group = (IndirectCullingGroup<I>) cullingGroups.computeIfAbsent(key.type(), t -> new IndirectCullingGroup<>(t, programs));
-		group.add((IndirectInstancer<I>) instancer, model, stage);
+		group.add((IndirectInstancer<I>) instancer, key.model(), key.stage());
 	}
 
 	public boolean hasStage(RenderStage stage) {

src/main/java/com/jozufozu/flywheel/backend/engine/instancing/InstancedDrawManager.java

@@ -14,7 +14,6 @@ import com.jozufozu.flywheel.api.event.RenderStage;
 import com.jozufozu.flywheel.api.instance.Instance;
 import com.jozufozu.flywheel.api.instance.InstanceType;
 import com.jozufozu.flywheel.api.model.Mesh;
-import com.jozufozu.flywheel.api.model.Model;
 import com.jozufozu.flywheel.backend.engine.InstancerKey;
 import com.jozufozu.flywheel.backend.engine.InstancerStorage;
 
@@ -64,18 +63,13 @@ public class InstancedDrawManager extends InstancerStorage<InstancedInstancer<?>
 	}
 
 	@Override
-	protected <I extends Instance> void add(InstancerKey<I> key, InstancedInstancer<?> instancer, Model model, RenderStage stage) {
+	protected <I extends Instance> void initialize(InstancerKey<I> key, InstancedInstancer<?> instancer) {
-		if (model.meshes()
-				.isEmpty()) {
-			// Don't bother allocating resources for models with no meshes.
-			return;
-		}
-
 		instancer.init();
 
-		DrawSet drawSet = drawSets.computeIfAbsent(stage, DrawSet::new);
+		DrawSet drawSet = drawSets.computeIfAbsent(key.stage(), DrawSet::new);
 
-		var meshes = model.meshes();
+		var meshes = key.model()
+				.meshes();
 		for (var entry : meshes.entrySet()) {
 			var mesh = alloc(entry.getValue());