From c438fb57cafc1c179dbc2d2cec2c57bfdc8ef529 Mon Sep 17 00:00:00 2001 From: Jozufozu Date: Sat, 15 Feb 2025 15:51:30 -0800 Subject: [PATCH] Atomic fusion before Flywheel 1.0 - Add workaround for race condition flipping the "mergeable" bit for instance pages - Sometimes a page would end up full while another thread was racing to set the mergeable bit, so when we later tried to merge pages we'd get an array index out of bounds - This change makes it so we only ever set the mergeable bit and handle the case of a full page when trying to merge - Should also help us avoid the alternative case where a page that is actually mergeable isn't marked as such --- .../engine/indirect/IndirectInstancer.java | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 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 aa9b372f1..499825bbb 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 @@ -141,12 +141,8 @@ public class IndirectInstancer extends AbstractInstancer // This is safe because only one bit position changes at a time. parent.fullPages.set(pageNo); } - if (isEmpty(currentValue)) { - // Value we just saw was zero, so since we added something we are now mergeable! + if (isMergeable(newValue)) { parent.mergeablePages.set(pageNo); - } else if (Integer.bitCount(currentValue) == 16) { - // We just filled the 17th instance, so we are no longer mergeable. - parent.mergeablePages.clear(pageNo); } parent.instanceCount.incrementAndGet(); @@ -199,11 +195,7 @@ public class IndirectInstancer extends AbstractInstancer if (valid.compareAndSet(currentValue, newValue)) { parent.validityChanged.set(pageNo); - if (isEmpty(newValue)) { - // If we decremented to zero then we're no longer mergeable. - parent.mergeablePages.clear(pageNo); - } else if (Integer.bitCount(newValue) == 16) { - // If we decremented to 16 then we're now mergeable. + if (isMergeable(newValue)) { parent.mergeablePages.set(pageNo); } // Set full page last so that other threads don't race to set the other bitsets. @@ -223,6 +215,13 @@ public class IndirectInstancer extends AbstractInstancer // Fill the holes in this page with instances from the other page. int valid = this.valid.get(); + + if (isFull(valid)) { + // We got filled after being marked mergeable, nothing to do + parent.mergeablePages.clear(pageNo); + return; + } + int otherValid = other.valid.get(); for (int i = 0; i < ObjectStorage.PAGE_SIZE; i++) {