From a7e709086662ffdec21be7a4e7859b4aa0a33692 Mon Sep 17 00:00:00 2001 From: Jozufozu Date: Mon, 28 Oct 2024 21:53:38 -0700 Subject: [PATCH 1/7] Finding an old bookmark - Make AbstractInstancer much more slim and move logic to BaseInstancer - Extend paging concept to the indirect instancer - Extend ObjectStorage to support more interesting layouts - Instance creation on indirect is now entirely lock free and deletions no longer require re-uploading the entire instancer --- .../backend/engine/AbstractInstancer.java | 229 +-------------- .../backend/engine/BaseInstancer.java | 175 +++++++++++ .../backend/engine/InstanceHandleImpl.java | 3 +- .../engine/indirect/IndirectInstancer.java | 273 ++++++++++++++++-- .../engine/indirect/ObjectStorage.java | 27 +- .../engine/instancing/InstancedInstancer.java | 58 +++- .../flywheel/internal/indirect/cull.glsl | 10 +- 7 files changed, 511 insertions(+), 264 deletions(-) create mode 100644 common/src/backend/java/dev/engine_room/flywheel/backend/engine/BaseInstancer.java diff --git a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/AbstractInstancer.java b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/AbstractInstancer.java index 3215d12d2..95bc88e58 100644 --- a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/AbstractInstancer.java +++ b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/AbstractInstancer.java @@ -1,27 +1,14 @@ package dev.engine_room.flywheel.backend.engine; -import java.util.ArrayList; - -import org.jetbrains.annotations.Nullable; - import dev.engine_room.flywheel.api.instance.Instance; import dev.engine_room.flywheel.api.instance.InstanceType; import dev.engine_room.flywheel.api.instance.Instancer; import dev.engine_room.flywheel.backend.engine.embed.Environment; -import dev.engine_room.flywheel.backend.util.AtomicBitSet; -public abstract class AbstractInstancer implements Instancer, InstanceHandleImpl.State { +public abstract class AbstractInstancer implements Instancer { public final InstanceType type; public final Environment environment; - private final Recreate recreate; - - // Lock for all instances, only needs to be used in methods that may run on the TaskExecutor. - protected final Object lock = new Object(); - protected final ArrayList instances = new ArrayList<>(); - protected final ArrayList> handles = new ArrayList<>(); - - protected final AtomicBitSet changed = new AtomicBitSet(); - protected final AtomicBitSet deleted = new AtomicBitSet(); + public final Recreate recreate; protected AbstractInstancer(InstancerKey key, Recreate recreate) { this.type = key.type(); @@ -29,218 +16,16 @@ public abstract class AbstractInstancer implements Instancer this.recreate = recreate; } - @Override - public InstanceHandleImpl.State setChanged(int index) { - notifyDirty(index); - return this; - } + public abstract InstanceHandleImpl.State revealInstance(InstanceHandleImpl handle, I instance); - @Override - public InstanceHandleImpl.State setDeleted(int index) { - notifyRemoval(index); - return InstanceHandleImpl.Deleted.instance(); - } + public abstract int instanceCount(); - @Override - public InstanceHandleImpl.State setVisible(InstanceHandleImpl handle, int index, boolean visible) { - if (visible) { - return this; - } - - notifyRemoval(index); - - I instance; - synchronized (lock) { - // I think we need to lock to prevent wacky stuff from happening if the array gets resized. - instance = instances.get(index); - } - - return new InstanceHandleImpl.Hidden<>(recreate, instance); - } - - @Override - public I createInstance() { - var handle = new InstanceHandleImpl<>(this); - I instance = type.create(handle); - - synchronized (lock) { - handle.index = instances.size(); - addLocked(instance, handle); - return instance; - } - } - - public void revealInstance(InstanceHandleImpl handle, I instance) { - synchronized (lock) { - handle.index = instances.size(); - addLocked(instance, handle); - } - } - - @Override - public void stealInstance(@Nullable I instance) { - if (instance == null) { - return; - } - - var instanceHandle = instance.handle(); - - if (!(instanceHandle instanceof InstanceHandleImpl)) { - // UB: do nothing - return; - } - - // Should InstanceType have an isInstance method? - @SuppressWarnings("unchecked") - var handle = (InstanceHandleImpl) instanceHandle; - - // No need to steal if this instance is already owned by this instancer. - if (handle.state == this) { - return; - } - // Not allowed to steal deleted instances. - if (handle.state instanceof InstanceHandleImpl.Deleted) { - return; - } - // No need to steal if the instance will recreate to us. - if (handle.state instanceof InstanceHandleImpl.Hidden hidden && recreate.equals(hidden.recreate())) { - return; - } - - // FIXME: in theory there could be a race condition here if the instance - // is somehow being stolen by 2 different instancers between threads. - // That seems kinda impossible so I'm fine leaving it as is for now. - - // Add the instance to this instancer. - if (handle.state instanceof AbstractInstancer other) { - // Remove the instance from its old instancer. - // This won't have any unwanted effect when the old instancer - // is filtering deleted instances later, so is safe. - other.notifyRemoval(handle.index); - - handle.state = this; - // Only lock now that we'll be mutating our state. - synchronized (lock) { - handle.index = instances.size(); - addLocked(instance, handle); - } - } else if (handle.state instanceof InstanceHandleImpl.Hidden) { - handle.state = new InstanceHandleImpl.Hidden<>(recreate, instance); - } - } - - /** - * Calls must be synchronized on {@link #lock}. - */ - private void addLocked(I instance, InstanceHandleImpl handle) { - instances.add(instance); - handles.add(handle); - setIndexChanged(handle.index); - } - - public int instanceCount() { - return instances.size(); - } - - public void notifyDirty(int index) { - if (index < 0 || index >= instanceCount()) { - return; - } - setIndexChanged(index); - } - - protected void setIndexChanged(int index) { - changed.set(index); - } - - public void notifyRemoval(int index) { - if (index < 0 || index >= instanceCount()) { - return; - } - deleted.set(index); - } - - public void removeDeletedInstances() { - if (deleted.isEmpty()) { - return; - } - - // Figure out which elements are to be removed. - final int oldSize = this.instances.size(); - int removeCount = deleted.cardinality(); - - if (oldSize == removeCount) { - clear(); - return; - } - - final int newSize = oldSize - removeCount; - - // Start from the first deleted index. - int writePos = deleted.nextSetBit(0); - - if (writePos < newSize) { - // Since we'll be shifting everything into this space we can consider it all changed. - setRangeChanged(writePos, newSize); - } - - // We definitely shouldn't consider the deleted instances as changed though, - // else we might try some out of bounds accesses later. - changed.clear(newSize, oldSize); - - // Punch out the deleted instances, shifting over surviving instances to fill their place. - for (int scanPos = writePos; (scanPos < oldSize) && (writePos < newSize); scanPos++, writePos++) { - // Find next non-deleted element. - scanPos = deleted.nextClearBit(scanPos); - - if (scanPos != writePos) { - // Grab the old instance/handle from scanPos... - var handle = handles.get(scanPos); - I instance = instances.get(scanPos); - - // ... and move it to writePos. - handles.set(writePos, handle); - instances.set(writePos, instance); - - // Make sure the handle knows it's been moved - handle.index = writePos; - } - } - - deleted.clear(); - instances.subList(newSize, oldSize) - .clear(); - handles.subList(newSize, oldSize) - .clear(); - } - - protected void setRangeChanged(int start, int end) { - changed.set(start, end); - } - - /** - * Clear all instances without freeing resources. - */ - public void clear() { - for (InstanceHandleImpl handle : handles) { - // Only clear instances that belong to this instancer. - // If one of these handles was stolen by another instancer, - // clearing it here would cause significant visual artifacts and instance leaks. - // At the same time, we need to clear handles we own to prevent - // instances from changing/deleting positions in this instancer that no longer exist. - if (handle.state == this) { - handle.clear(); - handle.state = InstanceHandleImpl.Deleted.instance(); - } - } - instances.clear(); - handles.clear(); - changed.clear(); - deleted.clear(); - } + public abstract void removeDeletedInstances(); public abstract void delete(); + public abstract void clear(); + @Override public String toString() { return "AbstractInstancer[" + instanceCount() + ']'; diff --git a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/BaseInstancer.java b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/BaseInstancer.java new file mode 100644 index 000000000..eae6d73cb --- /dev/null +++ b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/BaseInstancer.java @@ -0,0 +1,175 @@ +package dev.engine_room.flywheel.backend.engine; + +import java.util.ArrayList; + +import org.jetbrains.annotations.Nullable; + +import dev.engine_room.flywheel.api.instance.Instance; +import dev.engine_room.flywheel.backend.util.AtomicBitSet; + +public abstract class BaseInstancer extends AbstractInstancer implements InstanceHandleImpl.State { + // Lock for all instances, only needs to be used in methods that may run on the TaskExecutor. + protected final Object lock = new Object(); + protected final ArrayList instances = new ArrayList<>(); + protected final ArrayList> handles = new ArrayList<>(); + + protected final AtomicBitSet changed = new AtomicBitSet(); + protected final AtomicBitSet deleted = new AtomicBitSet(); + + protected BaseInstancer(InstancerKey key, Recreate recreate) { + super(key, recreate); + } + + + @Override + public InstanceHandleImpl.State setChanged(int index) { + notifyDirty(index); + return this; + } + + @Override + public InstanceHandleImpl.State setDeleted(int index) { + notifyRemoval(index); + return InstanceHandleImpl.Deleted.instance(); + } + + @Override + public InstanceHandleImpl.State setVisible(InstanceHandleImpl handle, int index, boolean visible) { + if (visible) { + return this; + } + + notifyRemoval(index); + + I instance; + synchronized (lock) { + // I think we need to lock to prevent wacky stuff from happening if the array gets resized. + instance = instances.get(index); + } + + return new InstanceHandleImpl.Hidden<>(recreate, instance); + } + + @Override + public I createInstance() { + var handle = new InstanceHandleImpl<>(this); + I instance = type.create(handle); + + synchronized (lock) { + handle.index = instances.size(); + addLocked(instance, handle); + return instance; + } + } + + public InstanceHandleImpl.State revealInstance(InstanceHandleImpl handle, I instance) { + synchronized (lock) { + handle.index = instances.size(); + addLocked(instance, handle); + } + return this; + } + + @Override + public void stealInstance(@Nullable I instance) { + if (instance == null) { + return; + } + + var instanceHandle = instance.handle(); + + if (!(instanceHandle instanceof InstanceHandleImpl)) { + // UB: do nothing + return; + } + + // Should InstanceType have an isInstance method? + @SuppressWarnings("unchecked") var handle = (InstanceHandleImpl) instanceHandle; + + // No need to steal if this instance is already owned by this instancer. + if (handle.state == this) { + return; + } + // Not allowed to steal deleted instances. + if (handle.state instanceof InstanceHandleImpl.Deleted) { + return; + } + // No need to steal if the instance will recreate to us. + if (handle.state instanceof InstanceHandleImpl.Hidden hidden && recreate.equals(hidden.recreate())) { + return; + } + + // FIXME: in theory there could be a race condition here if the instance + // is somehow being stolen by 2 different instancers between threads. + // That seems kinda impossible so I'm fine leaving it as is for now. + + // Add the instance to this instancer. + if (handle.state instanceof BaseInstancer other) { + // Remove the instance from its old instancer. + // This won't have any unwanted effect when the old instancer + // is filtering deleted instances later, so is safe. + other.notifyRemoval(handle.index); + + handle.state = this; + // Only lock now that we'll be mutating our state. + synchronized (lock) { + handle.index = instances.size(); + addLocked(instance, handle); + } + } else if (handle.state instanceof InstanceHandleImpl.Hidden) { + handle.state = new InstanceHandleImpl.Hidden<>(recreate, instance); + } + } + + /** + * Calls must be synchronized on {@link #lock}. + */ + private void addLocked(I instance, InstanceHandleImpl handle) { + instances.add(instance); + handles.add(handle); + setIndexChanged(handle.index); + } + + public int instanceCount() { + return instances.size(); + } + + public void notifyDirty(int index) { + if (index < 0 || index >= instanceCount()) { + return; + } + setIndexChanged(index); + } + + protected void setIndexChanged(int index) { + changed.set(index); + } + + public void notifyRemoval(int index) { + if (index < 0 || index >= instanceCount()) { + return; + } + deleted.set(index); + } + + /** + * Clear all instances without freeing resources. + */ + public void clear() { + for (InstanceHandleImpl handle : handles) { + // Only clear instances that belong to this instancer. + // If one of these handles was stolen by another instancer, + // clearing it here would cause significant visual artifacts and instance leaks. + // At the same time, we need to clear handles we own to prevent + // instances from changing/deleting positions in this instancer that no longer exist. + if (handle.state == this) { + handle.clear(); + handle.state = InstanceHandleImpl.Deleted.instance(); + } + } + instances.clear(); + handles.clear(); + changed.clear(); + deleted.clear(); + } +} diff --git a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/InstanceHandleImpl.java b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/InstanceHandleImpl.java index d5932f45e..445f5dfba 100644 --- a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/InstanceHandleImpl.java +++ b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/InstanceHandleImpl.java @@ -62,8 +62,7 @@ public class InstanceHandleImpl implements InstanceHandle { return this; } var instancer = recreate.recreate(); - instancer.revealInstance(handle, instance); - return instancer; + return instancer.revealInstance(handle, instance); } } diff --git a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/IndirectInstancer.java b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/IndirectInstancer.java index ee11999a8..c09a3e9e9 100644 --- a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/IndirectInstancer.java +++ b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/IndirectInstancer.java @@ -1,8 +1,12 @@ package dev.engine_room.flywheel.backend.engine.indirect; +import java.lang.reflect.Array; import java.util.ArrayList; import java.util.List; +import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.atomic.AtomicReference; +import org.jetbrains.annotations.Nullable; import org.jetbrains.annotations.UnknownNullability; import org.joml.Vector4fc; import org.lwjgl.system.MemoryUtil; @@ -10,17 +14,22 @@ import org.lwjgl.system.MemoryUtil; import dev.engine_room.flywheel.api.instance.Instance; import dev.engine_room.flywheel.api.instance.InstanceWriter; import dev.engine_room.flywheel.backend.engine.AbstractInstancer; +import dev.engine_room.flywheel.backend.engine.InstanceHandleImpl; import dev.engine_room.flywheel.backend.engine.InstancerKey; import dev.engine_room.flywheel.backend.util.AtomicBitSet; import dev.engine_room.flywheel.lib.math.MoreMath; public class IndirectInstancer extends AbstractInstancer { + private final AtomicReference pages; + private final long instanceStride; private final InstanceWriter writer; private final List associatedDraws = new ArrayList<>(); private final Vector4fc boundingSphere; private final AtomicBitSet changedPages = new AtomicBitSet(); + private final AtomicBitSet fullPages = new AtomicBitSet(); + private final Class instanceClass; public ObjectStorage.@UnknownNullability Mapping mapping; @@ -33,18 +42,93 @@ public class IndirectInstancer extends AbstractInstancer .byteSize()); writer = this.type.writer(); boundingSphere = key.model().boundingSphere(); + + instanceClass = (Class) type.create(new InstanceHandleImpl(null)) + .getClass(); + pages = new AtomicReference<>((InstancePage[]) Array.newInstance(InstancePage.class, 0)); } - @Override - public void setIndexChanged(int index) { - changedPages.set(ObjectStorage.objectIndex2PageIndex(index)); - } + public final class InstancePage implements InstanceHandleImpl.State { + private final int pageNo; + private final I[] instances; + private final InstanceHandleImpl[] handles; + /** + * A bitset describing which indices in the instances/handles arrays contain live instances. + */ + private final AtomicInteger valid; - @Override - protected void setRangeChanged(int start, int end) { - super.setRangeChanged(start, end); + InstancePage(Class clazz, int pageNo) { + this.pageNo = pageNo; + this.instances = (I[]) Array.newInstance(clazz, ObjectStorage.PAGE_SIZE); + this.handles = (InstanceHandleImpl[]) new InstanceHandleImpl[ObjectStorage.PAGE_SIZE]; + this.valid = new AtomicInteger(0); + } - changedPages.set(ObjectStorage.objectIndex2PageIndex(start), ObjectStorage.objectIndex2PageIndex(end) + 1); + public boolean add(I instance, InstanceHandleImpl handle) { + // Thread safety: we loop until we either win the race and add the given instance, or we + // run out of space because other threads trying to add at the same time. + while (true) { + int currentValue = valid.get(); + if (currentValue == 0xFFFFFFFF) { + // The page is full, must search elsewhere + return false; + } + + // determine what the new long value will be after we set the appropriate bit. + int index = Integer.numberOfTrailingZeros(~currentValue); + + int newValue = currentValue | (1 << index); + + // if no other thread has modified the value since we read it, we won the race and we are done. + if (valid.compareAndSet(currentValue, newValue)) { + instances[index] = instance; + handles[index] = handle; + handle.state = this; + handle.index = (pageNo << ObjectStorage.LOG_2_PAGE_SIZE) + index; + + changedPages.set(pageNo); + if (newValue == 0xFFFFFFFF) { + fullPages.set(pageNo); + } + return true; + } + } + } + + @Override + public InstanceHandleImpl.State setChanged(int index) { + changedPages.set(pageNo); + return this; + } + + @Override + public InstanceHandleImpl.State setDeleted(int index) { + int localIndex = index % ObjectStorage.PAGE_SIZE; + + instances[localIndex] = null; + handles[localIndex] = null; + + while (true) { + int currentValue = valid.get(); + int newValue = currentValue & ~(1 << localIndex); + + if (valid.compareAndSet(currentValue, newValue)) { + fullPages.clear(pageNo); + return InstanceHandleImpl.Deleted.instance(); + } + } + } + + @Override + public InstanceHandleImpl.State setVisible(InstanceHandleImpl handle, int index, boolean visible) { + if (visible) { + return this; + } + + int localIndex = index % ObjectStorage.PAGE_SIZE; + + return new InstanceHandleImpl.Hidden<>(recreate, instances[localIndex]); + } } public void addDraw(IndirectDraw draw) { @@ -56,9 +140,18 @@ public class IndirectInstancer extends AbstractInstancer } public void update(int modelIndex, int baseInstance) { - this.modelIndex = modelIndex; this.baseInstance = baseInstance; - mapping.update(modelIndex, instanceCount()); + + if (this.modelIndex == modelIndex && changedPages.isEmpty()) { + return; + } + this.modelIndex = modelIndex; + var pages = this.pages.get(); + mapping.updateCount(pages.length); + + for (int i = 0; i < pages.length; i++) { + mapping.updatePage(i, modelIndex, pages[i].valid.get()); + } } public void writeModel(long ptr) { @@ -76,21 +169,12 @@ public class IndirectInstancer extends AbstractInstancer return; } - int numPages = mapping.pageCount(); - - var instanceCount = instances.size(); - - for (int page = changedPages.nextSetBit(0); page >= 0 && page < numPages; page = changedPages.nextSetBit(page + 1)) { - int startObject = ObjectStorage.pageIndex2ObjectIndex(page); - - if (startObject >= instanceCount) { - break; - } - - int endObject = Math.min(instanceCount, ObjectStorage.pageIndex2ObjectIndex(page + 1)); + var pages = this.pages.get(); + for (int page = changedPages.nextSetBit(0); page >= 0 && page < pages.length; page = changedPages.nextSetBit(page + 1)) { + var instances = pages[page].instances; long baseByte = mapping.page2ByteOffset(page); - long size = (endObject - startObject) * instanceStride; + long size = ObjectStorage.PAGE_SIZE * instanceStride; // Because writes are broken into pages, we end up with significantly more calls into // StagingBuffer#enqueueCopy and the allocations for the writer got out of hand. Here @@ -101,9 +185,10 @@ public class IndirectInstancer extends AbstractInstancer long direct = stagingBuffer.reserveForCopy(size, instanceVbo, baseByte); if (direct != MemoryUtil.NULL) { - for (int i = startObject; i < endObject; i++) { - var instance = instances.get(i); - writer.write(direct, instance); + for (I instance : instances) { + if (instance != null) { + writer.write(direct, instance); + } direct += instanceStride; } continue; @@ -112,9 +197,10 @@ public class IndirectInstancer extends AbstractInstancer // Otherwise, write to a scratch buffer and enqueue a copy. var block = stagingBuffer.getScratch(size); var ptr = block.ptr(); - for (int i = startObject; i < endObject; i++) { - var instance = instances.get(i); - writer.write(ptr, instance); + for (I instance : instances) { + if (instance != null) { + writer.write(ptr, instance); + } ptr += instanceStride; } stagingBuffer.enqueueCopy(block.ptr(), size, instanceVbo, baseByte); @@ -123,6 +209,10 @@ public class IndirectInstancer extends AbstractInstancer changedPages.clear(); } + public void removeDeletedInstances() { + + } + @Override public void delete() { for (IndirectDraw draw : draws()) { @@ -143,4 +233,129 @@ public class IndirectInstancer extends AbstractInstancer public int local2GlobalInstanceIndex(int instanceIndex) { return mapping.objectIndex2GlobalIndex(instanceIndex); } + + @Override + public I createInstance() { + var handle = new InstanceHandleImpl(null); + I instance = type.create(handle); + + addInner(instance, handle); + + return instance; + } + + public InstanceHandleImpl.State revealInstance(InstanceHandleImpl handle, I instance) { + addInner(instance, handle); + return handle.state; + } + + @Override + public void stealInstance(@Nullable I instance) { + if (instance == null) { + return; + } + + var instanceHandle = instance.handle(); + + if (!(instanceHandle instanceof InstanceHandleImpl)) { + // UB: do nothing + return; + } + + // Should InstanceType have an isInstance method? + @SuppressWarnings("unchecked") var handle = (InstanceHandleImpl) instanceHandle; + + // Not allowed to steal deleted instances. + if (handle.state instanceof InstanceHandleImpl.Deleted) { + return; + } + // No need to steal if the instance will recreate to us. + if (handle.state instanceof InstanceHandleImpl.Hidden hidden && recreate.equals(hidden.recreate())) { + return; + } + + // FIXME: in theory there could be a race condition here if the instance + // is somehow being stolen by 2 different instancers between threads. + // That seems kinda impossible so I'm fine leaving it as is for now. + + // Add the instance to this instancer. + if (handle.state instanceof InstancePage other) { + // TODO: shortcut here if we already own the instance + + // Remove the instance from its old instancer. + // This won't have any unwanted effect when the old instancer + // is filtering deleted instances later, so is safe. + other.setDeleted(handle.index); + + // Only lock now that we'll be mutating our state. + addInner(instance, handle); + } else if (handle.state instanceof InstanceHandleImpl.Hidden) { + handle.state = new InstanceHandleImpl.Hidden<>(recreate, instance); + } + } + + private void addInner(I instance, InstanceHandleImpl handle) { + // Outer loop: + // - try to find an empty space + // - or grow the page array if we can't + // - add the instance to the new page, or try again + while (true) { + var pages = this.pages.get(); + + // First, try to find a page with space. + for (int i = fullPages.nextClearBit(0); i < pages.length; i = fullPages.nextClearBit(i + 1)) { + // It may have been filled in while we were searching, but hopefully not. + if (pages[i].add(instance, handle)) { + return; + } + } + + // If we're here, all other pages are full + // If we hit this on the second iteration of the outer loop then `pages` is once again full. + var desiredLength = pages.length + 1; + + // Inner loop: grow the page array. This is very similar to the logic in AtomicBitSet. + while (pages.length < desiredLength) { + // Thread safety: segments contains all pages from the currently visible pages, plus extra. + // all pages in the currently visible pages are canonical and will not change. + // Can't just `new InstancePage[]` because it has a generic parameter. + InstancePage[] newPages = (InstancePage[]) Array.newInstance(InstancePage.class, desiredLength); + + System.arraycopy(pages, 0, newPages, 0, pages.length); + newPages[pages.length] = new InstancePage(instanceClass, pages.length); + + // because we are using a compareAndSet, if this thread "wins the race" and successfully sets this variable, then the new page becomes canonical. + if (this.pages.compareAndSet(pages, newPages)) { + pages = newPages; + } else { + // If we "lose the race" and are growing the AtomicBitset segments larger, + // then we will gather the new canonical pages from the update which we missed on the next iteration of this loop. + // The new page will be discarded and never seen again. + pages = this.pages.get(); + } + } + + // Shortcut: try to add the instance to the last page. + // Technically we could just let the outer loop go again, but that + // involves a good bit of work just to likely get back here. + if (pages[pages.length - 1].add(instance, handle)) { + return; + } + // It may be the case that many other instances were added in the same instant. + // We can still lose this race, though it is very unlikely. + } + } + + public int instanceCount() { + // Not exactly accurate but it's an upper bound. + // TODO: maybe this could be tracked with an AtomicInteger? + return pages.get().length << ObjectStorage.LOG_2_PAGE_SIZE; + } + + /** + * Clear all instances without freeing resources. + */ + public void clear() { + + } } diff --git a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/ObjectStorage.java b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/ObjectStorage.java index 56d39613e..bf12c46a1 100644 --- a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/ObjectStorage.java +++ b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/ObjectStorage.java @@ -14,6 +14,7 @@ public class ObjectStorage extends AbstractArena { public static final int PAGE_MASK = PAGE_SIZE - 1; public static final int INITIAL_PAGES_ALLOCATED = 4; + public static final int DESCRIPTOR_SIZE_BYTES = Integer.BYTES * 2; /** * The GPU side buffer containing all the objects, logically divided into page frames. @@ -37,8 +38,8 @@ public class ObjectStorage extends AbstractArena { this.frameDescriptorBuffer = new ResizableStorageBuffer(); objectBuffer.ensureCapacity(INITIAL_PAGES_ALLOCATED * elementSizeBytes); - frameDescriptorBuffer.ensureCapacity(INITIAL_PAGES_ALLOCATED * Integer.BYTES); - frameDescriptors = MemoryBlock.malloc(INITIAL_PAGES_ALLOCATED * Integer.BYTES); + frameDescriptorBuffer.ensureCapacity(INITIAL_PAGES_ALLOCATED * DESCRIPTOR_SIZE_BYTES); + frameDescriptors = MemoryBlock.malloc(INITIAL_PAGES_ALLOCATED * DESCRIPTOR_SIZE_BYTES); } public Mapping createMapping() { @@ -79,7 +80,7 @@ public class ObjectStorage extends AbstractArena { } private long ptrForPage(int page) { - return frameDescriptors.ptr() + (long) page * Integer.BYTES; + return frameDescriptors.ptr() + (long) page * DESCRIPTOR_SIZE_BYTES; } public static int objectIndex2PageIndex(int objectIndex) { @@ -100,6 +101,14 @@ public class ObjectStorage extends AbstractArena { private int modelIndex = -1; private int objectCount = 0; + public void updatePage(int i, int modelIndex, int i1) { + var ptr = ptrForPage(pages[i]); + MemoryUtil.memPutInt(ptr, modelIndex); + MemoryUtil.memPutInt(ptr + 4, i1); + + ObjectStorage.this.needsUpload = true; + } + /** * Adjust this allocation to the given model index and object count. * @@ -155,6 +164,18 @@ public class ObjectStorage extends AbstractArena { } } + public void updateCount(int newLength) { + var oldLength = pages.length; + if (oldLength > newLength) { + // Eagerly free the now unnecessary pages. + // shrink will zero out the pageTable entries for the freed pages. + shrink(oldLength, newLength); + } else if (oldLength < newLength) { + // Allocate new pages to fit the new object count. + grow(newLength, oldLength); + } + } + public int pageCount() { return pages.length; } diff --git a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/instancing/InstancedInstancer.java b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/instancing/InstancedInstancer.java index 2895195a5..833e90ddf 100644 --- a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/instancing/InstancedInstancer.java +++ b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/instancing/InstancedInstancer.java @@ -7,7 +7,7 @@ import org.jetbrains.annotations.Nullable; import dev.engine_room.flywheel.api.instance.Instance; import dev.engine_room.flywheel.api.instance.InstanceWriter; -import dev.engine_room.flywheel.backend.engine.AbstractInstancer; +import dev.engine_room.flywheel.backend.engine.BaseInstancer; import dev.engine_room.flywheel.backend.engine.InstancerKey; import dev.engine_room.flywheel.backend.gl.TextureBuffer; import dev.engine_room.flywheel.backend.gl.buffer.GlBuffer; @@ -15,7 +15,7 @@ import dev.engine_room.flywheel.backend.gl.buffer.GlBufferUsage; import dev.engine_room.flywheel.lib.math.MoreMath; import dev.engine_room.flywheel.lib.memory.MemoryBlock; -public class InstancedInstancer extends AbstractInstancer { +public class InstancedInstancer extends BaseInstancer { private final int instanceStride; private final InstanceWriter writer; @@ -109,6 +109,60 @@ public class InstancedInstancer extends AbstractInstancer return capacity > vbo.size(); } + public void removeDeletedInstances() { + if (deleted.isEmpty()) { + return; + } + + // Figure out which elements are to be removed. + final int oldSize = this.instances.size(); + int removeCount = deleted.cardinality(); + + if (oldSize == removeCount) { + clear(); + return; + } + + final int newSize = oldSize - removeCount; + + // Start from the first deleted index. + int writePos = deleted.nextSetBit(0); + + if (writePos < newSize) { + // Since we'll be shifting everything into this space we can consider it all changed. + changed.set(writePos, newSize); + } + + // We definitely shouldn't consider the deleted instances as changed though, + // else we might try some out of bounds accesses later. + changed.clear(newSize, oldSize); + + // Punch out the deleted instances, shifting over surviving instances to fill their place. + for (int scanPos = writePos; (scanPos < oldSize) && (writePos < newSize); scanPos++, writePos++) { + // Find next non-deleted element. + scanPos = deleted.nextClearBit(scanPos); + + if (scanPos != writePos) { + // Grab the old instance/handle from scanPos... + var handle = handles.get(scanPos); + I instance = instances.get(scanPos); + + // ... and move it to writePos. + handles.set(writePos, handle); + instances.set(writePos, instance); + + // Make sure the handle knows it's been moved + handle.index = writePos; + } + } + + deleted.clear(); + instances.subList(newSize, oldSize) + .clear(); + handles.subList(newSize, oldSize) + .clear(); + } + public void delete() { if (vbo == null) { return; diff --git a/common/src/backend/resources/assets/flywheel/flywheel/internal/indirect/cull.glsl b/common/src/backend/resources/assets/flywheel/flywheel/internal/indirect/cull.glsl index 395979d5c..1869e33b8 100644 --- a/common/src/backend/resources/assets/flywheel/flywheel/internal/indirect/cull.glsl +++ b/common/src/backend/resources/assets/flywheel/flywheel/internal/indirect/cull.glsl @@ -126,24 +126,22 @@ bool _flw_isVisible(uint instanceIndex, uint modelIndex) { } void main() { - uint pageIndex = gl_WorkGroupID.x; + uint pageIndex = gl_WorkGroupID.x << 1u; if (pageIndex >= _flw_pageFrameDescriptors.length()) { return; } - uint packedModelIndexAndCount = _flw_pageFrameDescriptors[pageIndex]; + uint modelIndex = _flw_pageFrameDescriptors[pageIndex]; - uint pageInstanceCount = packedModelIndexAndCount >> _FLW_PAGE_COUNT_OFFSET; + uint pageValidity = _flw_pageFrameDescriptors[pageIndex + 1]; - if (gl_LocalInvocationID.x >= pageInstanceCount) { + if (((1u << gl_LocalInvocationID.x) & pageValidity) == 0) { return; } uint instanceIndex = gl_GlobalInvocationID.x; - uint modelIndex = packedModelIndexAndCount & _FLW_MODEL_INDEX_MASK; - if (_flw_isVisible(instanceIndex, modelIndex)) { uint localIndex = atomicAdd(_flw_models[modelIndex].instanceCount, 1); uint targetIndex = _flw_models[modelIndex].baseInstance + localIndex; From 20b3f78b9cadbf5e3c0eb455b0dcb142c81ce1fe Mon Sep 17 00:00:00 2001 From: Jozufozu Date: Tue, 29 Oct 2024 20:01:20 -0700 Subject: [PATCH 2/7] A real page turner - Try to shuffle over instances into pages with space - Clear out now-unused logic from ObjectStorage - Some cleanup and more comments in IndirectInstancer --- .../backend/engine/AbstractInstancer.java | 2 +- .../flywheel/backend/engine/DrawManager.java | 2 +- .../engine/indirect/IndirectInstancer.java | 143 ++++++++++++++++-- .../engine/indirect/ObjectStorage.java | 85 ----------- .../engine/instancing/InstancedInstancer.java | 2 +- 5 files changed, 135 insertions(+), 99 deletions(-) diff --git a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/AbstractInstancer.java b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/AbstractInstancer.java index 95bc88e58..605ee0441 100644 --- a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/AbstractInstancer.java +++ b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/AbstractInstancer.java @@ -20,7 +20,7 @@ public abstract class AbstractInstancer implements Instancer public abstract int instanceCount(); - public abstract void removeDeletedInstances(); + public abstract void parallelUpdate(); public abstract void delete(); diff --git a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/DrawManager.java b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/DrawManager.java index f34887f00..898253bb0 100644 --- a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/DrawManager.java +++ b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/DrawManager.java @@ -50,7 +50,7 @@ public abstract class DrawManager> { public Plan createFramePlan() { // Go wide on instancers to process deletions in parallel. - return ForEachPlan.of(() -> new ArrayList<>(instancers.values()), AbstractInstancer::removeDeletedInstances); + return ForEachPlan.of(() -> new ArrayList<>(instancers.values()), AbstractInstancer::parallelUpdate); } public void flush(LightStorage lightStorage, EnvironmentStorage environmentStorage) { diff --git a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/IndirectInstancer.java b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/IndirectInstancer.java index c09a3e9e9..3bec5e4f7 100644 --- a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/IndirectInstancer.java +++ b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/IndirectInstancer.java @@ -6,6 +6,7 @@ import java.util.List; import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicReference; +import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; import org.jetbrains.annotations.UnknownNullability; import org.joml.Vector4fc; @@ -20,13 +21,12 @@ import dev.engine_room.flywheel.backend.util.AtomicBitSet; import dev.engine_room.flywheel.lib.math.MoreMath; public class IndirectInstancer extends AbstractInstancer { - private final AtomicReference pages; - private final long instanceStride; private final InstanceWriter writer; private final List associatedDraws = new ArrayList<>(); private final Vector4fc boundingSphere; + private final AtomicReference pages; private final AtomicBitSet changedPages = new AtomicBitSet(); private final AtomicBitSet fullPages = new AtomicBitSet(); private final Class instanceClass; @@ -45,7 +45,19 @@ public class IndirectInstancer extends AbstractInstancer instanceClass = (Class) type.create(new InstanceHandleImpl(null)) .getClass(); - pages = new AtomicReference<>((InstancePage[]) Array.newInstance(InstancePage.class, 0)); + pages = new AtomicReference<>(pageArray(0)); + } + + @NotNull + @SuppressWarnings("unchecked") + private InstancePage[] pageArray(int length) { + return (InstancePage[]) Array.newInstance(InstancePage.class, length); + } + + @NotNull + @SuppressWarnings("unchecked") + private I[] instanceArray() { + return (I[]) Array.newInstance(instanceClass, ObjectStorage.PAGE_SIZE); } public final class InstancePage implements InstanceHandleImpl.State { @@ -57,19 +69,30 @@ public class IndirectInstancer extends AbstractInstancer */ private final AtomicInteger valid; - InstancePage(Class clazz, int pageNo) { + InstancePage(int pageNo) { this.pageNo = pageNo; - this.instances = (I[]) Array.newInstance(clazz, ObjectStorage.PAGE_SIZE); + this.instances = instanceArray(); this.handles = (InstanceHandleImpl[]) new InstanceHandleImpl[ObjectStorage.PAGE_SIZE]; this.valid = new AtomicInteger(0); } + public int count() { + return Integer.bitCount(valid.get()); + } + + /** + * Attempt to add the given instance/handle to this page. + * + * @param instance The instance to add + * @param handle The instance's handle + * @return true if the instance was added, false if the page is full + */ public boolean add(I instance, InstanceHandleImpl handle) { // Thread safety: we loop until we either win the race and add the given instance, or we // run out of space because other threads trying to add at the same time. while (true) { int currentValue = valid.get(); - if (currentValue == 0xFFFFFFFF) { + if (isFull(currentValue)) { // The page is full, must search elsewhere return false; } @@ -84,10 +107,13 @@ public class IndirectInstancer extends AbstractInstancer instances[index] = instance; handles[index] = handle; handle.state = this; - handle.index = (pageNo << ObjectStorage.LOG_2_PAGE_SIZE) + index; + // Handle index is unique amongst all pages of this instancer. + handle.index = local2HandleIndex(index); changedPages.set(pageNo); - if (newValue == 0xFFFFFFFF) { + if (isFull(newValue)) { + // The page is now full, mark it so in the bitset. + // This is safe because only one bit position changes at a time. fullPages.set(pageNo); } return true; @@ -95,6 +121,10 @@ public class IndirectInstancer extends AbstractInstancer } } + private int local2HandleIndex(int index) { + return (pageNo << ObjectStorage.LOG_2_PAGE_SIZE) + index; + } + @Override public InstanceHandleImpl.State setChanged(int index) { changedPages.set(pageNo); @@ -114,6 +144,7 @@ public class IndirectInstancer extends AbstractInstancer if (valid.compareAndSet(currentValue, newValue)) { fullPages.clear(pageNo); + changedPages.set(pageNo); return InstanceHandleImpl.Deleted.instance(); } } @@ -129,6 +160,56 @@ public class IndirectInstancer extends AbstractInstancer return new InstanceHandleImpl.Hidden<>(recreate, instances[localIndex]); } + + public int takeFrom(InstancePage other) { + // Fill the holes in this page with instances from the other page. + + int valid = this.valid.get(); + int otherValid = other.valid.get(); + + // If the other page is empty, or we're full, we're done. + if (otherValid == 0 || valid == 0xFFFFFFFF) { + return valid; + } + + // Something is going to change, so mark stuff ahead of time. + changedPages.set(pageNo); + changedPages.set(other.pageNo); + + for (int i = 0; i < ObjectStorage.PAGE_SIZE; i++) { + int mask = 1 << i; + + if ((otherValid & mask) == 0) { + continue; + } + + int writePos = Integer.numberOfTrailingZeros(~valid); + + instances[writePos] = other.instances[i]; + handles[writePos] = other.handles[i]; + + handles[writePos].state = this; + handles[writePos].index = local2HandleIndex(writePos); + + // Clear out the other page. + otherValid &= ~mask; + other.handles[i] = null; + other.instances[i] = null; + + // Set the bit in this page and find the next write position. + valid |= 1 << writePos; + + // If we're full, we're done. + if (isFull(valid)) { + break; + } + } + + this.valid.set(valid); + other.valid.set(otherValid); + + return valid; + } } public void addDraw(IndirectDraw draw) { @@ -209,8 +290,45 @@ public class IndirectInstancer extends AbstractInstancer changedPages.clear(); } - public void removeDeletedInstances() { + public void parallelUpdate() { + if (true) { + // FIXME: infinite loop when the page in readpos doesn't have enough to fill the page in writepos + return; + } + var pages = this.pages.get(); + + // If there are at least 2 pages with space, we can consolidate. + if (fullPages.cardinality() > (pages.length - 2)) { + return; + } + + // Note this runs after visuals are updated so we don't really have to take care for thread safety. + + int writePos = 0; + + while (true) { + writePos = fullPages.nextClearBit(writePos); + int readPos = fullPages.nextClearBit(writePos + 1); + + if (writePos >= pages.length || readPos >= pages.length) { + break; + } + + InstancePage writeTo = pages[writePos]; + InstancePage readFrom = pages[readPos]; + + int validNow = writeTo.takeFrom(readFrom); + + if (isFull(validNow)) { + fullPages.set(writePos); + writePos = readPos; + } + } + } + + private static boolean isFull(int valid) { + return valid == 0xFFFFFFFF; } @Override @@ -319,10 +437,10 @@ public class IndirectInstancer extends AbstractInstancer // Thread safety: segments contains all pages from the currently visible pages, plus extra. // all pages in the currently visible pages are canonical and will not change. // Can't just `new InstancePage[]` because it has a generic parameter. - InstancePage[] newPages = (InstancePage[]) Array.newInstance(InstancePage.class, desiredLength); + InstancePage[] newPages = pageArray(desiredLength); System.arraycopy(pages, 0, newPages, 0, pages.length); - newPages[pages.length] = new InstancePage(instanceClass, pages.length); + newPages[pages.length] = new InstancePage(pages.length); // because we are using a compareAndSet, if this thread "wins the race" and successfully sets this variable, then the new page becomes canonical. if (this.pages.compareAndSet(pages, newPages)) { @@ -356,6 +474,9 @@ public class IndirectInstancer extends AbstractInstancer * Clear all instances without freeing resources. */ public void clear() { + this.pages.set(pageArray(0)); + changedPages.clear(); + fullPages.clear(); } } diff --git a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/ObjectStorage.java b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/ObjectStorage.java index bf12c46a1..3f43b8374 100644 --- a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/ObjectStorage.java +++ b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/ObjectStorage.java @@ -98,9 +98,6 @@ public class ObjectStorage extends AbstractArena { private static final int[] EMPTY_ALLOCATION = new int[0]; private int[] pages = EMPTY_ALLOCATION; - private int modelIndex = -1; - private int objectCount = 0; - public void updatePage(int i, int modelIndex, int i1) { var ptr = ptrForPage(pages[i]); MemoryUtil.memPutInt(ptr, modelIndex); @@ -109,61 +106,6 @@ public class ObjectStorage extends AbstractArena { ObjectStorage.this.needsUpload = true; } - /** - * Adjust this allocation to the given model index and object count. - * - *

This method triggers eager resizing of the allocation to fit the new object count. - * If the model index is different from the current one, all frame descriptors will be updated. - * - * @param modelIndex The model index the objects in this allocation are associated with. - * @param objectCount The number of objects in this allocation. - */ - public void update(int modelIndex, int objectCount) { - boolean incremental = this.modelIndex == modelIndex; - - if (incremental && objectCount == this.objectCount) { - // Nothing will change. - return; - } - - ObjectStorage.this.needsUpload = true; - - this.modelIndex = modelIndex; - this.objectCount = objectCount; - - var oldLength = pages.length; - var newLength = objectIndex2PageIndex((objectCount + PAGE_MASK)); - - if (oldLength > newLength) { - // Eagerly free the now unnecessary pages. - // shrink will zero out the pageTable entries for the freed pages. - shrink(oldLength, newLength); - - if (incremental) { - // Only update the last page, everything else is unchanged. - updateRange(newLength - 1, newLength); - } - } else if (oldLength < newLength) { - // Allocate new pages to fit the new object count. - grow(newLength, oldLength); - - if (incremental) { - // Update the old last page + all new pages - updateRange(oldLength - 1, newLength); - } - } else { - if (incremental) { - // Only update the last page. - updateRange(oldLength - 1, oldLength); - } - } - - if (!incremental) { - // Update all pages. - updateRange(0, newLength); - } - } - public void updateCount(int newLength) { var oldLength = pages.length; if (oldLength > newLength) { @@ -189,37 +131,10 @@ public class ObjectStorage extends AbstractArena { ObjectStorage.this.free(page); } pages = EMPTY_ALLOCATION; - modelIndex = -1; - objectCount = 0; ObjectStorage.this.needsUpload = true; } - /** - * Calculates the page descriptor for the given page index. - * Runs under the assumption than all pages are full except maybe the last one. - */ - private int calculatePageDescriptor(int pageIndex) { - int countInPage; - if (objectCount % PAGE_SIZE != 0 && pageIndex == pages.length - 1) { - // Last page && it isn't full -> use the remainder. - countInPage = objectCount & PAGE_MASK; - } else if (objectCount > 0) { - // Full page. - countInPage = PAGE_SIZE; - } else { - // Empty page, this shouldn't be reachable because we eagerly free empty pages. - countInPage = 0; - } - return (modelIndex & 0x3FFFFF) | (countInPage << 26); - } - - private void updateRange(int start, int oldLength) { - for (int i = start; i < oldLength; i++) { - MemoryUtil.memPutInt(ptrForPage(pages[i]), calculatePageDescriptor(i)); - } - } - private void grow(int neededPages, int oldLength) { pages = Arrays.copyOf(pages, neededPages); diff --git a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/instancing/InstancedInstancer.java b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/instancing/InstancedInstancer.java index 833e90ddf..c56b281eb 100644 --- a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/instancing/InstancedInstancer.java +++ b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/instancing/InstancedInstancer.java @@ -109,7 +109,7 @@ public class InstancedInstancer extends BaseInstancer { return capacity > vbo.size(); } - public void removeDeletedInstances() { + public void parallelUpdate() { if (deleted.isEmpty()) { return; } From fac63168c1c445883867f48dafc54b987a84bba8 Mon Sep 17 00:00:00 2001 From: Jozufozu Date: Fri, 1 Nov 2024 12:47:15 -0700 Subject: [PATCH 3/7] Bookkeeping - Mappings drop pages when they write zero validity bits - Instancer only updates pages that changed --- .../engine/indirect/IndirectInstancer.java | 59 +++++++------------ .../engine/indirect/ObjectStorage.java | 54 +++++++++++++++-- 2 files changed, 69 insertions(+), 44 deletions(-) diff --git a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/IndirectInstancer.java b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/IndirectInstancer.java index 3bec5e4f7..8ee084136 100644 --- a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/IndirectInstancer.java +++ b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/IndirectInstancer.java @@ -168,7 +168,7 @@ public class IndirectInstancer extends AbstractInstancer int otherValid = other.valid.get(); // If the other page is empty, or we're full, we're done. - if (otherValid == 0 || valid == 0xFFFFFFFF) { + if (isEmpty(otherValid) || isFull(valid)) { return valid; } @@ -223,15 +223,27 @@ public class IndirectInstancer extends AbstractInstancer public void update(int modelIndex, int baseInstance) { this.baseInstance = baseInstance; - if (this.modelIndex == modelIndex && changedPages.isEmpty()) { + var sameModelIndex = this.modelIndex == modelIndex; + if (sameModelIndex && changedPages.isEmpty()) { + // Nothing to do! return; } + this.modelIndex = modelIndex; + var pages = this.pages.get(); mapping.updateCount(pages.length); - for (int i = 0; i < pages.length; i++) { - mapping.updatePage(i, modelIndex, pages[i].valid.get()); + if (sameModelIndex) { + // Only need to update the changed pages. + for (int page = changedPages.nextSetBit(0); page >= 0 && page < pages.length; page = changedPages.nextSetBit(page + 1)) { + mapping.updatePage(page, modelIndex, pages[page].valid.get()); + } + } else { + // Need to update all pages since the model index changed. + for (int i = 0; i < pages.length; i++) { + mapping.updatePage(i, modelIndex, pages[i].valid.get()); + } } } @@ -291,46 +303,17 @@ public class IndirectInstancer extends AbstractInstancer } public void parallelUpdate() { - if (true) { - // FIXME: infinite loop when the page in readpos doesn't have enough to fill the page in writepos - return; - } - - var pages = this.pages.get(); - - // If there are at least 2 pages with space, we can consolidate. - if (fullPages.cardinality() > (pages.length - 2)) { - return; - } - - // Note this runs after visuals are updated so we don't really have to take care for thread safety. - - int writePos = 0; - - while (true) { - writePos = fullPages.nextClearBit(writePos); - int readPos = fullPages.nextClearBit(writePos + 1); - - if (writePos >= pages.length || readPos >= pages.length) { - break; - } - - InstancePage writeTo = pages[writePos]; - InstancePage readFrom = pages[readPos]; - - int validNow = writeTo.takeFrom(readFrom); - - if (isFull(validNow)) { - fullPages.set(writePos); - writePos = readPos; - } - } + // TODO: Merge pages when they're less than half full. } private static boolean isFull(int valid) { return valid == 0xFFFFFFFF; } + private static boolean isEmpty(int otherValid) { + return otherValid == 0; + } + @Override public void delete() { for (IndirectDraw draw : draws()) { diff --git a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/ObjectStorage.java b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/ObjectStorage.java index 3f43b8374..20b6ee87a 100644 --- a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/ObjectStorage.java +++ b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/ObjectStorage.java @@ -13,6 +13,8 @@ public class ObjectStorage extends AbstractArena { public static final int PAGE_SIZE = 1 << LOG_2_PAGE_SIZE; public static final int PAGE_MASK = PAGE_SIZE - 1; + public static final int INVALID_PAGE = -1; + public static final int INITIAL_PAGES_ALLOCATED = 4; public static final int DESCRIPTOR_SIZE_BYTES = Integer.BYTES * 2; @@ -53,8 +55,13 @@ public class ObjectStorage extends AbstractArena { @Override public void free(int i) { + if (i == INVALID_PAGE) { + return; + } super.free(i); - MemoryUtil.memPutInt(ptrForPage(i), 0); + var ptr = ptrForPage(i); + MemoryUtil.memPutInt(ptr, 0); + MemoryUtil.memPutInt(ptr + 4, 0); } @Override @@ -98,14 +105,49 @@ public class ObjectStorage extends AbstractArena { private static final int[] EMPTY_ALLOCATION = new int[0]; private int[] pages = EMPTY_ALLOCATION; - public void updatePage(int i, int modelIndex, int i1) { - var ptr = ptrForPage(pages[i]); + public void updatePage(int index, int modelIndex, int validBits) { + if (validBits == 0) { + holePunch(index); + return; + } + var page = pages[index]; + + if (page == INVALID_PAGE) { + // Un-holed punch. + page = unHolePunch(index); + } + + var ptr = ptrForPage(page); MemoryUtil.memPutInt(ptr, modelIndex); - MemoryUtil.memPutInt(ptr + 4, i1); + MemoryUtil.memPutInt(ptr + 4, validBits); ObjectStorage.this.needsUpload = true; } + /** + * Free a page on the inside of the mapping, maintaining the same virtual mapping size. + * + * @param index The index of the page to free. + */ + public void holePunch(int index) { + ObjectStorage.this.free(pages[index]); + pages[index] = INVALID_PAGE; + + ObjectStorage.this.needsUpload = true; + } + + /** + * Allocate a new page on the inside of the mapping, maintaining the same virtual mapping size. + * + * @param index The index of the page to allocate. + * @return The allocated page. + */ + private int unHolePunch(int index) { + int page = ObjectStorage.this.alloc(); + pages[index] = page; + return page; + } + public void updateCount(int newLength) { var oldLength = pages.length; if (oldLength > newLength) { @@ -122,8 +164,8 @@ public class ObjectStorage extends AbstractArena { return pages.length; } - public long page2ByteOffset(int page) { - return ObjectStorage.this.byteOffsetOf(pages[page]); + public long page2ByteOffset(int index) { + return ObjectStorage.this.byteOffsetOf(pages[index]); } public void delete() { From fc3e475ec9e096bd4f9dc6bf4f5815782f42a97d Mon Sep 17 00:00:00 2001 From: Jozufozu Date: Fri, 1 Nov 2024 23:26:12 -0700 Subject: [PATCH 4/7] Gattai! - Combine pages only when they're at most half full, and not empty - This guarantees that we'll fully empty a page, allowing us to free the memory for use by other instancers - Track mergeable pages via a separate bitset --- .../engine/indirect/IndirectInstancer.java | 65 +++++++++++++++---- .../flywheel/backend/util/AtomicBitSet.java | 8 +++ 2 files changed, 62 insertions(+), 11 deletions(-) diff --git a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/IndirectInstancer.java b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/IndirectInstancer.java index 8ee084136..b3a032337 100644 --- a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/IndirectInstancer.java +++ b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/IndirectInstancer.java @@ -29,6 +29,12 @@ public class IndirectInstancer extends AbstractInstancer private final AtomicReference pages; private final AtomicBitSet changedPages = new AtomicBitSet(); private final AtomicBitSet fullPages = new AtomicBitSet(); + /** + * The set of mergable pages. A page is mergeable if it is not empty and has 16 or fewer instances. + * These constraints are set so that we can guarantee that merging two pages leaves one entirely empty, + * but we also don't want to waste work merging into pages that are already empty. + */ + private final AtomicBitSet mergeablePages = new AtomicBitSet(); private final Class instanceClass; public ObjectStorage.@UnknownNullability Mapping mapping; @@ -116,6 +122,13 @@ public class IndirectInstancer extends AbstractInstancer // This is safe because only one bit position changes at a time. fullPages.set(pageNo); } + if (isEmpty(currentValue)) { + // Value we just saw was zero, so since we added something we are now mergeable! + mergeablePages.set(pageNo); + } else if (Integer.bitCount(currentValue) == 16) { + // We just filled the 17th instance, so we are no longer mergeable. + mergeablePages.clear(pageNo); + } return true; } } @@ -143,8 +156,16 @@ public class IndirectInstancer extends AbstractInstancer int newValue = currentValue & ~(1 << localIndex); if (valid.compareAndSet(currentValue, newValue)) { - fullPages.clear(pageNo); changedPages.set(pageNo); + if (isEmpty(newValue)) { + // If we decremented to zero then we're no longer mergeable. + mergeablePages.clear(pageNo); + } else if (Integer.bitCount(newValue) == 16) { + // If we decremented to 16 then we're now mergeable. + mergeablePages.set(pageNo); + } + // Set full page last so that other threads don't race to set the other bitsets. + fullPages.clear(pageNo); return InstanceHandleImpl.Deleted.instance(); } } @@ -161,17 +182,12 @@ public class IndirectInstancer extends AbstractInstancer return new InstanceHandleImpl.Hidden<>(recreate, instances[localIndex]); } - public int takeFrom(InstancePage other) { + public void takeFrom(InstancePage other) { // Fill the holes in this page with instances from the other page. int valid = this.valid.get(); int otherValid = other.valid.get(); - // If the other page is empty, or we're full, we're done. - if (isEmpty(otherValid) || isFull(valid)) { - return valid; - } - // Something is going to change, so mark stuff ahead of time. changedPages.set(pageNo); changedPages.set(other.pageNo); @@ -208,7 +224,13 @@ public class IndirectInstancer extends AbstractInstancer this.valid.set(valid); other.valid.set(otherValid); - return valid; + mergeablePages.set(pageNo, isMergeable(valid)); + // With all assumptions in place we should have fully drained the other page, but check just to be safe. + mergeablePages.set(other.pageNo, isMergeable(otherValid)); + + if (isFull(valid)) { + fullPages.set(pageNo); + } } } @@ -303,15 +325,36 @@ public class IndirectInstancer extends AbstractInstancer } public void parallelUpdate() { - // TODO: Merge pages when they're less than half full. + var pages = this.pages.get(); + + int page = 0; + while (mergeablePages.cardinality() > 1) { + page = mergeablePages.nextSetBit(page); + if (page < 0) { + break; + } + + // Find the next mergeable page. + int next = mergeablePages.nextSetBit(page + 1); + if (next < 0) { + break; + } + + // Try to merge the pages. + pages[page].takeFrom(pages[next]); + } } private static boolean isFull(int valid) { return valid == 0xFFFFFFFF; } - private static boolean isEmpty(int otherValid) { - return otherValid == 0; + private static boolean isEmpty(int valid) { + return valid == 0; + } + + private static boolean isMergeable(int valid) { + return !isEmpty(valid) && Integer.bitCount(valid) <= 16; } @Override diff --git a/common/src/backend/java/dev/engine_room/flywheel/backend/util/AtomicBitSet.java b/common/src/backend/java/dev/engine_room/flywheel/backend/util/AtomicBitSet.java index 334b02732..beeeb403d 100644 --- a/common/src/backend/java/dev/engine_room/flywheel/backend/util/AtomicBitSet.java +++ b/common/src/backend/java/dev/engine_room/flywheel/backend/util/AtomicBitSet.java @@ -43,6 +43,14 @@ public class AtomicBitSet { segments = new AtomicReference<>(new AtomicBitSetSegments(numSegmentsToPreallocate, numLongsPerSegment)); } + public void set(int position, boolean value) { + if (value) { + set(position); + } else { + clear(position); + } + } + public void set(int position) { int longPosition = longIndexInSegmentForPosition(position); From a9f2018c0a267216bdc30cf8f30e258d23a7b2be Mon Sep 17 00:00:00 2001 From: Jozufozu Date: Sat, 2 Nov 2024 14:01:22 -0700 Subject: [PATCH 5/7] The epitome of laziness - Only upload changed page frame descriptors - In the instancer, track changed contents separately from changed validity, so we can separately upload objects and descriptors --- .../engine/indirect/IndirectInstancer.java | 69 ++++++++++++++----- .../engine/indirect/ObjectStorage.java | 43 +++++++----- 2 files changed, 76 insertions(+), 36 deletions(-) diff --git a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/IndirectInstancer.java b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/IndirectInstancer.java index b3a032337..d6a73d61e 100644 --- a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/IndirectInstancer.java +++ b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/IndirectInstancer.java @@ -27,7 +27,19 @@ public class IndirectInstancer extends AbstractInstancer private final Vector4fc boundingSphere; private final AtomicReference pages; - private final AtomicBitSet changedPages = new AtomicBitSet(); + /** + * The set of pages whose count changed and thus need their descriptor re-uploaded. + */ + private final AtomicBitSet validityChanged = new AtomicBitSet(); + /** + * The set of pages whose content changed and thus need their instances re-uploaded. + * Note that we don't re-upload for deletions, as the memory becomes invalid and masked out by the validity bits. + */ + private final AtomicBitSet contentsChanged = new AtomicBitSet(); + /** + * The set of pages that are entirely full. + * We scan the clear bits of this set when trying to add an instance. + */ private final AtomicBitSet fullPages = new AtomicBitSet(); /** * The set of mergable pages. A page is mergeable if it is not empty and has 16 or fewer instances. @@ -69,6 +81,7 @@ public class IndirectInstancer extends AbstractInstancer public final class InstancePage implements InstanceHandleImpl.State { private final int pageNo; private final I[] instances; + // Handles are only read in #takeFrom. It would be nice to avoid tracking these at all. private final InstanceHandleImpl[] handles; /** * A bitset describing which indices in the instances/handles arrays contain live instances. @@ -116,7 +129,8 @@ public class IndirectInstancer extends AbstractInstancer // Handle index is unique amongst all pages of this instancer. handle.index = local2HandleIndex(index); - changedPages.set(pageNo); + contentsChanged.set(pageNo); + validityChanged.set(pageNo); if (isFull(newValue)) { // The page is now full, mark it so in the bitset. // This is safe because only one bit position changes at a time. @@ -140,7 +154,7 @@ public class IndirectInstancer extends AbstractInstancer @Override public InstanceHandleImpl.State setChanged(int index) { - changedPages.set(pageNo); + contentsChanged.set(pageNo); return this; } @@ -156,7 +170,7 @@ public class IndirectInstancer extends AbstractInstancer int newValue = currentValue & ~(1 << localIndex); if (valid.compareAndSet(currentValue, newValue)) { - changedPages.set(pageNo); + validityChanged.set(pageNo); if (isEmpty(newValue)) { // If we decremented to zero then we're no longer mergeable. mergeablePages.clear(pageNo); @@ -182,19 +196,21 @@ public class IndirectInstancer extends AbstractInstancer return new InstanceHandleImpl.Hidden<>(recreate, instances[localIndex]); } - public void takeFrom(InstancePage other) { + /** + * Only call this on 2 pages that are mergeable. + * + * @param other The page to take instances from. + */ + private void takeFrom(InstancePage other) { // Fill the holes in this page with instances from the other page. int valid = this.valid.get(); int otherValid = other.valid.get(); - // Something is going to change, so mark stuff ahead of time. - changedPages.set(pageNo); - changedPages.set(other.pageNo); - for (int i = 0; i < ObjectStorage.PAGE_SIZE; i++) { int mask = 1 << i; + // Find set bits in the other page. if ((otherValid & mask) == 0) { continue; } @@ -212,7 +228,7 @@ public class IndirectInstancer extends AbstractInstancer other.handles[i] = null; other.instances[i] = null; - // Set the bit in this page and find the next write position. + // Set the bit in this page so we can find the next write position. valid |= 1 << writePos; // If we're full, we're done. @@ -224,9 +240,18 @@ public class IndirectInstancer extends AbstractInstancer this.valid.set(valid); other.valid.set(otherValid); + // If the other page was quite empty we may still be mergeable. mergeablePages.set(pageNo, isMergeable(valid)); - // With all assumptions in place we should have fully drained the other page, but check just to be safe. - mergeablePages.set(other.pageNo, isMergeable(otherValid)); + + // We definitely changed the contents and validity of this page. + contentsChanged.set(pageNo); + validityChanged.set(pageNo); + + // The other page will end up empty, so the validity changes and it's no longer mergeable. + // Also clear the changed bit so we don't re-upload the instances. + contentsChanged.clear(other.pageNo); + validityChanged.set(other.pageNo); + mergeablePages.clear(other.pageNo); if (isFull(valid)) { fullPages.set(pageNo); @@ -246,7 +271,7 @@ public class IndirectInstancer extends AbstractInstancer this.baseInstance = baseInstance; var sameModelIndex = this.modelIndex == modelIndex; - if (sameModelIndex && changedPages.isEmpty()) { + if (sameModelIndex && validityChanged.isEmpty()) { // Nothing to do! return; } @@ -258,7 +283,7 @@ public class IndirectInstancer extends AbstractInstancer if (sameModelIndex) { // Only need to update the changed pages. - for (int page = changedPages.nextSetBit(0); page >= 0 && page < pages.length; page = changedPages.nextSetBit(page + 1)) { + for (int page = validityChanged.nextSetBit(0); page >= 0 && page < pages.length; page = validityChanged.nextSetBit(page + 1)) { mapping.updatePage(page, modelIndex, pages[page].valid.get()); } } else { @@ -267,6 +292,8 @@ public class IndirectInstancer extends AbstractInstancer mapping.updatePage(i, modelIndex, pages[i].valid.get()); } } + + validityChanged.clear(); } public void writeModel(long ptr) { @@ -280,15 +307,21 @@ public class IndirectInstancer extends AbstractInstancer } public void uploadInstances(StagingBuffer stagingBuffer, int instanceVbo) { - if (changedPages.isEmpty()) { + if (contentsChanged.isEmpty()) { return; } var pages = this.pages.get(); - for (int page = changedPages.nextSetBit(0); page >= 0 && page < pages.length; page = changedPages.nextSetBit(page + 1)) { + for (int page = contentsChanged.nextSetBit(0); page >= 0 && page < pages.length; page = contentsChanged.nextSetBit(page + 1)) { var instances = pages[page].instances; long baseByte = mapping.page2ByteOffset(page); + + if (baseByte < 0) { + // This page is not mapped to the VBO. + continue; + } + long size = ObjectStorage.PAGE_SIZE * instanceStride; // Because writes are broken into pages, we end up with significantly more calls into @@ -321,7 +354,7 @@ public class IndirectInstancer extends AbstractInstancer stagingBuffer.enqueueCopy(block.ptr(), size, instanceVbo, baseByte); } - changedPages.clear(); + contentsChanged.clear(); } public void parallelUpdate() { @@ -501,7 +534,7 @@ public class IndirectInstancer extends AbstractInstancer */ public void clear() { this.pages.set(pageArray(0)); - changedPages.clear(); + contentsChanged.clear(); fullPages.clear(); } diff --git a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/ObjectStorage.java b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/ObjectStorage.java index 20b6ee87a..d7c7e67b3 100644 --- a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/ObjectStorage.java +++ b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/ObjectStorage.java @@ -1,6 +1,7 @@ package dev.engine_room.flywheel.backend.engine.indirect; import java.util.Arrays; +import java.util.BitSet; import org.lwjgl.system.MemoryUtil; @@ -18,6 +19,7 @@ public class ObjectStorage extends AbstractArena { public static final int INITIAL_PAGES_ALLOCATED = 4; public static final int DESCRIPTOR_SIZE_BYTES = Integer.BYTES * 2; + private final BitSet changedFrames = new BitSet(); /** * The GPU side buffer containing all the objects, logically divided into page frames. */ @@ -31,8 +33,6 @@ public class ObjectStorage extends AbstractArena { */ private MemoryBlock frameDescriptors; - private boolean needsUpload = false; - public ObjectStorage(long objectSizeBytes) { super(PAGE_SIZE * objectSizeBytes); @@ -62,6 +62,16 @@ public class ObjectStorage extends AbstractArena { var ptr = ptrForPage(i); MemoryUtil.memPutInt(ptr, 0); MemoryUtil.memPutInt(ptr + 4, 0); + + changedFrames.set(i); + } + + private void set(int i, int modelIndex, int validBits) { + var ptr = ptrForPage(i); + MemoryUtil.memPutInt(ptr, modelIndex); + MemoryUtil.memPutInt(ptr + 4, validBits); + + changedFrames.set(i); } @Override @@ -72,12 +82,17 @@ public class ObjectStorage extends AbstractArena { } public void uploadDescriptors(StagingBuffer stagingBuffer) { - if (!needsUpload) { + if (changedFrames.isEmpty()) { return; } - // We could be smarter about which spans are uploaded but this thing is so small it's probably not worth it. - stagingBuffer.enqueueCopy(frameDescriptors.ptr(), frameDescriptors.size(), frameDescriptorBuffer.handle(), 0); - needsUpload = false; + + var ptr = frameDescriptors.ptr(); + for (int i = changedFrames.nextSetBit(0); i >= 0 && i < capacity(); i = changedFrames.nextSetBit(i + 1)) { + var offset = (long) i * DESCRIPTOR_SIZE_BYTES; + stagingBuffer.enqueueCopy(ptr + offset, DESCRIPTOR_SIZE_BYTES, frameDescriptorBuffer.handle(), offset); + } + + changedFrames.clear(); } public void delete() { @@ -110,18 +125,14 @@ public class ObjectStorage extends AbstractArena { holePunch(index); return; } - var page = pages[index]; + var frame = pages[index]; - if (page == INVALID_PAGE) { + if (frame == INVALID_PAGE) { // Un-holed punch. - page = unHolePunch(index); + frame = unHolePunch(index); } - var ptr = ptrForPage(page); - MemoryUtil.memPutInt(ptr, modelIndex); - MemoryUtil.memPutInt(ptr + 4, validBits); - - ObjectStorage.this.needsUpload = true; + ObjectStorage.this.set(frame, modelIndex, validBits); } /** @@ -132,8 +143,6 @@ public class ObjectStorage extends AbstractArena { public void holePunch(int index) { ObjectStorage.this.free(pages[index]); pages[index] = INVALID_PAGE; - - ObjectStorage.this.needsUpload = true; } /** @@ -173,8 +182,6 @@ public class ObjectStorage extends AbstractArena { ObjectStorage.this.free(page); } pages = EMPTY_ALLOCATION; - - ObjectStorage.this.needsUpload = true; } private void grow(int neededPages, int oldLength) { From 540fe7a7feac03e20b2bd7c2ddb192427b175590 Mon Sep 17 00:00:00 2001 From: Jozufozu Date: Sat, 2 Nov 2024 16:56:26 -0700 Subject: [PATCH 6/7] Acetaminophen - Generic pain relief - Use new Instance[] rather than capturing the class object of the instance type - Make InstancePage static, but manually track the instancer parent so we can check when stealing - Simplify array creation helpers and make them static - Mark InstanceHandleImpl#state as UnknownNullability --- .../backend/engine/InstanceHandleImpl.java | 5 +- .../engine/indirect/IndirectInstancer.java | 87 +++++++++---------- 2 files changed, 46 insertions(+), 46 deletions(-) diff --git a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/InstanceHandleImpl.java b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/InstanceHandleImpl.java index 445f5dfba..e83936b0c 100644 --- a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/InstanceHandleImpl.java +++ b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/InstanceHandleImpl.java @@ -1,13 +1,16 @@ package dev.engine_room.flywheel.backend.engine; +import org.jetbrains.annotations.UnknownNullability; + import dev.engine_room.flywheel.api.instance.Instance; import dev.engine_room.flywheel.api.instance.InstanceHandle; public class InstanceHandleImpl implements InstanceHandle { + @UnknownNullability public State state; public int index; - public InstanceHandleImpl(State state) { + public InstanceHandleImpl(@UnknownNullability State state) { this.state = state; } diff --git a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/IndirectInstancer.java b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/IndirectInstancer.java index d6a73d61e..d1f39907a 100644 --- a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/IndirectInstancer.java +++ b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/IndirectInstancer.java @@ -1,12 +1,10 @@ package dev.engine_room.flywheel.backend.engine.indirect; -import java.lang.reflect.Array; import java.util.ArrayList; import java.util.List; import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicReference; -import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; import org.jetbrains.annotations.UnknownNullability; import org.joml.Vector4fc; @@ -26,7 +24,7 @@ public class IndirectInstancer extends AbstractInstancer private final List associatedDraws = new ArrayList<>(); private final Vector4fc boundingSphere; - private final AtomicReference pages; + private final AtomicReference[]> pages = new AtomicReference<>(pageArray(0)); /** * The set of pages whose count changed and thus need their descriptor re-uploaded. */ @@ -47,7 +45,6 @@ public class IndirectInstancer extends AbstractInstancer * but we also don't want to waste work merging into pages that are already empty. */ private final AtomicBitSet mergeablePages = new AtomicBitSet(); - private final Class instanceClass; public ObjectStorage.@UnknownNullability Mapping mapping; @@ -60,25 +57,25 @@ public class IndirectInstancer extends AbstractInstancer .byteSize()); writer = this.type.writer(); boundingSphere = key.model().boundingSphere(); - - instanceClass = (Class) type.create(new InstanceHandleImpl(null)) - .getClass(); - pages = new AtomicReference<>(pageArray(0)); } - @NotNull @SuppressWarnings("unchecked") - private InstancePage[] pageArray(int length) { - return (InstancePage[]) Array.newInstance(InstancePage.class, length); + private static InstancePage[] pageArray(int length) { + return new InstancePage[length]; } - @NotNull @SuppressWarnings("unchecked") - private I[] instanceArray() { - return (I[]) Array.newInstance(instanceClass, ObjectStorage.PAGE_SIZE); + private static I[] instanceArray() { + return (I[]) new Instance[ObjectStorage.PAGE_SIZE]; } - public final class InstancePage implements InstanceHandleImpl.State { + @SuppressWarnings("unchecked") + private static InstanceHandleImpl[] handleArray() { + return new InstanceHandleImpl[ObjectStorage.PAGE_SIZE]; + } + + private static final class InstancePage implements InstanceHandleImpl.State { + private final IndirectInstancer parent; private final int pageNo; private final I[] instances; // Handles are only read in #takeFrom. It would be nice to avoid tracking these at all. @@ -88,17 +85,14 @@ public class IndirectInstancer extends AbstractInstancer */ private final AtomicInteger valid; - InstancePage(int pageNo) { + private InstancePage(IndirectInstancer parent, int pageNo) { + this.parent = parent; this.pageNo = pageNo; this.instances = instanceArray(); - this.handles = (InstanceHandleImpl[]) new InstanceHandleImpl[ObjectStorage.PAGE_SIZE]; + this.handles = handleArray(); this.valid = new AtomicInteger(0); } - public int count() { - return Integer.bitCount(valid.get()); - } - /** * Attempt to add the given instance/handle to this page. * @@ -129,19 +123,19 @@ public class IndirectInstancer extends AbstractInstancer // Handle index is unique amongst all pages of this instancer. handle.index = local2HandleIndex(index); - contentsChanged.set(pageNo); - validityChanged.set(pageNo); + parent.contentsChanged.set(pageNo); + parent.validityChanged.set(pageNo); if (isFull(newValue)) { // The page is now full, mark it so in the bitset. // This is safe because only one bit position changes at a time. - fullPages.set(pageNo); + parent.fullPages.set(pageNo); } if (isEmpty(currentValue)) { // Value we just saw was zero, so since we added something we are now mergeable! - mergeablePages.set(pageNo); + parent.mergeablePages.set(pageNo); } else if (Integer.bitCount(currentValue) == 16) { // We just filled the 17th instance, so we are no longer mergeable. - mergeablePages.clear(pageNo); + parent.mergeablePages.clear(pageNo); } return true; } @@ -154,7 +148,7 @@ public class IndirectInstancer extends AbstractInstancer @Override public InstanceHandleImpl.State setChanged(int index) { - contentsChanged.set(pageNo); + parent.contentsChanged.set(pageNo); return this; } @@ -170,16 +164,16 @@ public class IndirectInstancer extends AbstractInstancer int newValue = currentValue & ~(1 << localIndex); if (valid.compareAndSet(currentValue, newValue)) { - validityChanged.set(pageNo); + parent.validityChanged.set(pageNo); if (isEmpty(newValue)) { // If we decremented to zero then we're no longer mergeable. - mergeablePages.clear(pageNo); + parent.mergeablePages.clear(pageNo); } else if (Integer.bitCount(newValue) == 16) { // If we decremented to 16 then we're now mergeable. - mergeablePages.set(pageNo); + parent.mergeablePages.set(pageNo); } // Set full page last so that other threads don't race to set the other bitsets. - fullPages.clear(pageNo); + parent.fullPages.clear(pageNo); return InstanceHandleImpl.Deleted.instance(); } } @@ -193,7 +187,7 @@ public class IndirectInstancer extends AbstractInstancer int localIndex = index % ObjectStorage.PAGE_SIZE; - return new InstanceHandleImpl.Hidden<>(recreate, instances[localIndex]); + return new InstanceHandleImpl.Hidden<>(parent.recreate, instances[localIndex]); } /** @@ -201,7 +195,7 @@ public class IndirectInstancer extends AbstractInstancer * * @param other The page to take instances from. */ - private void takeFrom(InstancePage other) { + private void takeFrom(InstancePage other) { // Fill the holes in this page with instances from the other page. int valid = this.valid.get(); @@ -241,20 +235,20 @@ public class IndirectInstancer extends AbstractInstancer other.valid.set(otherValid); // If the other page was quite empty we may still be mergeable. - mergeablePages.set(pageNo, isMergeable(valid)); + parent.mergeablePages.set(pageNo, isMergeable(valid)); // We definitely changed the contents and validity of this page. - contentsChanged.set(pageNo); - validityChanged.set(pageNo); + parent.contentsChanged.set(pageNo); + parent.validityChanged.set(pageNo); // The other page will end up empty, so the validity changes and it's no longer mergeable. // Also clear the changed bit so we don't re-upload the instances. - contentsChanged.clear(other.pageNo); - validityChanged.set(other.pageNo); - mergeablePages.clear(other.pageNo); + parent.contentsChanged.clear(other.pageNo); + parent.validityChanged.set(other.pageNo); + parent.mergeablePages.clear(other.pageNo); if (isFull(valid)) { - fullPages.set(pageNo); + parent.fullPages.set(pageNo); } } } @@ -456,8 +450,10 @@ public class IndirectInstancer extends AbstractInstancer // That seems kinda impossible so I'm fine leaving it as is for now. // Add the instance to this instancer. - if (handle.state instanceof InstancePage other) { - // TODO: shortcut here if we already own the instance + if (handle.state instanceof InstancePage other) { + if (other.parent == this) { + return; + } // Remove the instance from its old instancer. // This won't have any unwanted effect when the old instancer @@ -496,10 +492,10 @@ public class IndirectInstancer extends AbstractInstancer // Thread safety: segments contains all pages from the currently visible pages, plus extra. // all pages in the currently visible pages are canonical and will not change. // Can't just `new InstancePage[]` because it has a generic parameter. - InstancePage[] newPages = pageArray(desiredLength); + InstancePage[] newPages = pageArray(desiredLength); System.arraycopy(pages, 0, newPages, 0, pages.length); - newPages[pages.length] = new InstancePage(pages.length); + newPages[pages.length] = new InstancePage<>(this, pages.length); // because we are using a compareAndSet, if this thread "wins the race" and successfully sets this variable, then the new page becomes canonical. if (this.pages.compareAndSet(pages, newPages)) { @@ -535,7 +531,8 @@ public class IndirectInstancer extends AbstractInstancer public void clear() { this.pages.set(pageArray(0)); contentsChanged.clear(); + validityChanged.clear(); fullPages.clear(); - + mergeablePages.clear(); } } From 4f0c1cc1ae2be5dc5315f7e3c360f75305ab0596 Mon Sep 17 00:00:00 2001 From: Jozufozu Date: Sun, 3 Nov 2024 12:15:04 -0800 Subject: [PATCH 7/7] Checking the source - Remove source checks, it was very old and untouched - Remove FIXMEs and TODOs that were already fixed/done --- .../backend/compile/SourceChecks.java | 55 ------------------- .../component/InstanceAssemblerComponent.java | 2 +- .../flywheel/backend/engine/LightStorage.java | 2 - .../visualization/VisualizerRegistryImpl.java | 1 - .../flywheel/vanilla/VanillaVisuals.java | 20 ------- 5 files changed, 1 insertion(+), 79 deletions(-) delete mode 100644 common/src/backend/java/dev/engine_room/flywheel/backend/compile/SourceChecks.java diff --git a/common/src/backend/java/dev/engine_room/flywheel/backend/compile/SourceChecks.java b/common/src/backend/java/dev/engine_room/flywheel/backend/compile/SourceChecks.java deleted file mode 100644 index fdfc5c3d1..000000000 --- a/common/src/backend/java/dev/engine_room/flywheel/backend/compile/SourceChecks.java +++ /dev/null @@ -1,55 +0,0 @@ -package dev.engine_room.flywheel.backend.compile; - -// TODO: recycle to be invoked by the shader compiler -public class SourceChecks { - // public static final BiConsumer LAYOUT_VERTEX = checkFunctionArity("flw_layoutVertex", 0); - // public static final BiConsumer INSTANCE_VERTEX = checkFunctionParameterTypeExists("flw_instanceVertex", 1, 0); - // public static final BiConsumer MATERIAL_VERTEX = checkFunctionArity("flw_materialVertex", 0); - // public static final BiConsumer MATERIAL_FRAGMENT = checkFunctionArity("flw_materialFragment", 0); - // public static final BiConsumer CONTEXT_VERTEX = checkFunctionArity("flw_contextVertex", 0); - // public static final BiConsumer CONTEXT_FRAGMENT = checkFunctionArity("flw_contextFragment", 0).andThen(checkFunctionArity("flw_initFragment", 0)); - // public static final BiConsumer PIPELINE = checkFunctionArity("main", 0); - // - // public static BiConsumer checkFunctionArity(String name, int arity) { - // return (errorReporter, file) -> checkFunctionArity(errorReporter, file, name, arity); - // } - // - // public static BiConsumer checkFunctionParameterTypeExists(String name, int arity, int param) { - // return (errorReporter, file) -> { - // var func = checkFunctionArity(errorReporter, file, name, arity); - // - // if (func == null) { - // return; - // } - // - // var maybeStruct = func.getParameterType(param) - // .findStruct(); - // - // if (maybeStruct.isEmpty()) { - // errorReporter.generateMissingStruct(file, func.getParameterType(param), "struct not defined"); - // } - // }; - // } - // - // /** - // * @return {@code null} if the function doesn't exist, or if the function has the wrong arity. - // */ - // @Nullable - // private static ShaderFunction checkFunctionArity(ErrorReporter errorReporter, SourceFile file, String name, int arity) { - // Optional maybeFunc = file.findFunction(name); - // - // if (maybeFunc.isEmpty()) { - // errorReporter.generateMissingFunction(file, name, "\"" + name + "\" function not defined"); - // return null; - // } - // - // ShaderFunction func = maybeFunc.get(); - // ImmutableList params = func.getParameters(); - // if (params.size() != arity) { - // errorReporter.generateFunctionArgumentCountError(name, arity, func.getArgs()); - // return null; - // } - // - // return func; - // } -} diff --git a/common/src/backend/java/dev/engine_room/flywheel/backend/compile/component/InstanceAssemblerComponent.java b/common/src/backend/java/dev/engine_room/flywheel/backend/compile/component/InstanceAssemblerComponent.java index 7115e1a8b..eb42189b4 100644 --- a/common/src/backend/java/dev/engine_room/flywheel/backend/compile/component/InstanceAssemblerComponent.java +++ b/common/src/backend/java/dev/engine_room/flywheel/backend/compile/component/InstanceAssemblerComponent.java @@ -62,7 +62,7 @@ public abstract class InstanceAssemblerComponent implements SourceComponent { FLOAT_UNPACKING_FUNCS.put(FloatRepr.UNSIGNED_INT, e -> e.cast("float")); FLOAT_UNPACKING_FUNCS.put(FloatRepr.NORMALIZED_UNSIGNED_INT, e -> e.cast("float").div(4294967295f)); - FLOAT_UNPACKING_FUNCS.put(FloatRepr.FLOAT, e -> e.callFunction("uintBitsToFloat")); // FIXME: GLSL 330+ + FLOAT_UNPACKING_FUNCS.put(FloatRepr.FLOAT, e -> e.callFunction("uintBitsToFloat")); } protected final Layout layout; diff --git a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/LightStorage.java b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/LightStorage.java index 47e884a52..5b5ac1d51 100644 --- a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/LightStorage.java +++ b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/LightStorage.java @@ -22,7 +22,6 @@ import net.minecraft.world.level.LightLayer; import net.minecraft.world.level.lighting.LayerLightEventListener; /** - * TODO: AO data * A managed arena of light sections for uploading to the GPU. * *

Each section represents an 18x18x18 block volume of light data. @@ -116,7 +115,6 @@ public class LightStorage { } // Now actually do the collection. - // TODO: Should this be done in parallel? sectionsToCollect.forEach(this::collectSection); updatedSections.clear(); diff --git a/common/src/main/java/dev/engine_room/flywheel/impl/visualization/VisualizerRegistryImpl.java b/common/src/main/java/dev/engine_room/flywheel/impl/visualization/VisualizerRegistryImpl.java index b78562b0b..40522c89d 100644 --- a/common/src/main/java/dev/engine_room/flywheel/impl/visualization/VisualizerRegistryImpl.java +++ b/common/src/main/java/dev/engine_room/flywheel/impl/visualization/VisualizerRegistryImpl.java @@ -11,7 +11,6 @@ import net.minecraft.world.entity.EntityType; import net.minecraft.world.level.block.entity.BlockEntity; import net.minecraft.world.level.block.entity.BlockEntityType; -// TODO: Add freezing @SuppressWarnings("unchecked") public final class VisualizerRegistryImpl { @Nullable diff --git a/common/src/main/java/dev/engine_room/flywheel/vanilla/VanillaVisuals.java b/common/src/main/java/dev/engine_room/flywheel/vanilla/VanillaVisuals.java index 53efc4562..3516cd041 100644 --- a/common/src/main/java/dev/engine_room/flywheel/vanilla/VanillaVisuals.java +++ b/common/src/main/java/dev/engine_room/flywheel/vanilla/VanillaVisuals.java @@ -7,26 +7,6 @@ import net.minecraft.client.model.geom.ModelLayers; import net.minecraft.world.entity.EntityType; import net.minecraft.world.level.block.entity.BlockEntityType; -/** - * TODO: - * - * - * - * - * - * - * - * - * - * - * - * - * - * - * - * - *
{@link BlockEntityType#SIGN} {@link net.minecraft.client.renderer.blockentity.SignRenderer SignRenderer}
{@link BlockEntityType#PISTON} {@link net.minecraft.client.renderer.blockentity.PistonHeadRenderer PistonHeadRenderer}
{@link BlockEntityType#CONDUIT} {@link net.minecraft.client.renderer.blockentity.ConduitRenderer ConduitRenderer}
{@link BlockEntityType#ENCHANTING_TABLE} {@link net.minecraft.client.renderer.blockentity.EnchantTableRenderer EnchantTableRenderer}
{@link BlockEntityType#LECTERN} {@link net.minecraft.client.renderer.blockentity.LecternRenderer LecternRenderer}
{@link BlockEntityType#MOB_SPAWNER} {@link net.minecraft.client.renderer.blockentity.SpawnerRenderer SpawnerRenderer}
{@link BlockEntityType#BED} {@link net.minecraft.client.renderer.blockentity.BedRenderer BedRenderer}
^^ Interesting - Major vv
{@link BlockEntityType#END_PORTAL} {@link net.minecraft.client.renderer.blockentity.TheEndPortalRenderer TheEndPortalRenderer}
{@link BlockEntityType#END_GATEWAY} {@link net.minecraft.client.renderer.blockentity.TheEndGatewayRenderer TheEndGatewayRenderer}
{@link BlockEntityType#BEACON} {@link net.minecraft.client.renderer.blockentity.BeaconRenderer BeaconRenderer}
{@link BlockEntityType#SKULL} {@link net.minecraft.client.renderer.blockentity.SkullBlockRenderer SkullBlockRenderer}
{@link BlockEntityType#BANNER} {@link net.minecraft.client.renderer.blockentity.BannerRenderer BannerRenderer}
{@link BlockEntityType#STRUCTURE_BLOCK} {@link net.minecraft.client.renderer.debug.StructureRenderer StructureRenderer}
{@link BlockEntityType#CAMPFIRE} {@link net.minecraft.client.renderer.blockentity.CampfireRenderer CampfireRenderer}
- */ public class VanillaVisuals { public static void init() { builder(BlockEntityType.CHEST)