From addad713c5f76f7c4cb874c2bb7bc2798801b479 Mon Sep 17 00:00:00 2001 From: Petr Portnov Date: Mon, 15 May 2023 02:50:07 +0300 Subject: [PATCH 1/4] Make memory fence in `ByteBufferGuard` explicit --- .../org/apache/lucene/store/ByteBufferGuard.java | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-) diff --git a/lucene/core/src/java/org/apache/lucene/store/ByteBufferGuard.java b/lucene/core/src/java/org/apache/lucene/store/ByteBufferGuard.java index 2d75597f9deb..822d6af0128f 100644 --- a/lucene/core/src/java/org/apache/lucene/store/ByteBufferGuard.java +++ b/lucene/core/src/java/org/apache/lucene/store/ByteBufferGuard.java @@ -17,6 +17,7 @@ package org.apache.lucene.store; import java.io.IOException; +import java.lang.invoke.VarHandle; import java.nio.ByteBuffer; import java.nio.FloatBuffer; import java.nio.IntBuffer; @@ -39,7 +40,7 @@ final class ByteBufferGuard { * this to allow unmapping of bytebuffers with private Java APIs. */ @FunctionalInterface - static interface BufferCleaner { + interface BufferCleaner { void freeBuffer(String resourceDescription, ByteBuffer b) throws IOException; } @@ -49,9 +50,6 @@ static interface BufferCleaner { /** Not volatile; see comments on visibility below! */ private boolean invalidated = false; - /** Used as a store-store barrier; see comments below! */ - private final AtomicInteger barrier = new AtomicInteger(); - /** * Creates an instance to be used for a single {@link ByteBufferIndexInput} which must be shared * by all of its clones. @@ -65,14 +63,8 @@ public ByteBufferGuard(String resourceDescription, BufferCleaner cleaner) { public void invalidateAndUnmap(ByteBuffer... bufs) throws IOException { if (cleaner != null) { invalidated = true; - // This call should hopefully flush any CPU caches and as a result make - // the "invalidated" field update visible to other threads. We specifically - // don't make "invalidated" field volatile for performance reasons, hoping the - // JVM won't optimize away reads of that field and hardware should ensure - // caches are in sync after this call. This isn't entirely "fool-proof" - // (see LUCENE-7409 discussion), but it has been shown to work in practice - // and we count on this behavior. - barrier.lazySet(0); + // Makes "invalidated" field visible to other threads. + VarHandle.fullFence(); // we give other threads a bit of time to finish reads on their ByteBuffer...: Thread.yield(); // finally unmap the ByteBuffers: From bf48bf8899717d2b13742caf2cad9118ae45f549 Mon Sep 17 00:00:00 2001 From: Petr Portnov Date: Tue, 16 May 2023 17:35:13 +0300 Subject: [PATCH 2/4] Rollback removal of `static` on `ByteBufferGuard.BufferCleaner` --- .../core/src/java/org/apache/lucene/store/ByteBufferGuard.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lucene/core/src/java/org/apache/lucene/store/ByteBufferGuard.java b/lucene/core/src/java/org/apache/lucene/store/ByteBufferGuard.java index 822d6af0128f..9e80382943fe 100644 --- a/lucene/core/src/java/org/apache/lucene/store/ByteBufferGuard.java +++ b/lucene/core/src/java/org/apache/lucene/store/ByteBufferGuard.java @@ -40,7 +40,7 @@ final class ByteBufferGuard { * this to allow unmapping of bytebuffers with private Java APIs. */ @FunctionalInterface - interface BufferCleaner { + static interface BufferCleaner { void freeBuffer(String resourceDescription, ByteBuffer b) throws IOException; } From b70fdb46dc7ead271faaf69c17c0dcffddb31a72 Mon Sep 17 00:00:00 2001 From: Petr Portnov Date: Tue, 16 May 2023 17:39:17 +0300 Subject: [PATCH 3/4] Remove redundant `AtomicInteger` import in `ByteBufferGuard` --- .../core/src/java/org/apache/lucene/store/ByteBufferGuard.java | 1 - 1 file changed, 1 deletion(-) diff --git a/lucene/core/src/java/org/apache/lucene/store/ByteBufferGuard.java b/lucene/core/src/java/org/apache/lucene/store/ByteBufferGuard.java index 9e80382943fe..314868b14f69 100644 --- a/lucene/core/src/java/org/apache/lucene/store/ByteBufferGuard.java +++ b/lucene/core/src/java/org/apache/lucene/store/ByteBufferGuard.java @@ -22,7 +22,6 @@ import java.nio.FloatBuffer; import java.nio.IntBuffer; import java.nio.LongBuffer; -import java.util.concurrent.atomic.AtomicInteger; /** * A guard that is created for every {@link ByteBufferIndexInput} that tries on best effort to From 3c529ba4b4a4dfa32ca8a200cfaab000abd218fa Mon Sep 17 00:00:00 2001 From: Petr Portnov Date: Thu, 1 Jun 2023 13:04:46 +0300 Subject: [PATCH 4/4] Update CHANGES.txt to reflect GITHUB#12290 --- lucene/CHANGES.txt | 4 +++- .../src/java/org/apache/lucene/store/ByteBufferGuard.java | 7 ++++++- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/lucene/CHANGES.txt b/lucene/CHANGES.txt index 7de2179817d0..218417ce0575 100644 --- a/lucene/CHANGES.txt +++ b/lucene/CHANGES.txt @@ -119,7 +119,9 @@ New Features Improvements --------------------- -GITHUB#12245: Add support for Score Mode to `ToParentBlockJoinQuery` explain. (Marcus Eagan via Mikhail Khludnev) +* GITHUB#12245: Add support for Score Mode to `ToParentBlockJoinQuery` explain. (Marcus Eagan via Mikhail Khludnev) + +* GITHUB#12290: Make memory fence in ByteBufferGuard explicit using `VarHandle.fullFence()` Optimizations --------------------- diff --git a/lucene/core/src/java/org/apache/lucene/store/ByteBufferGuard.java b/lucene/core/src/java/org/apache/lucene/store/ByteBufferGuard.java index 314868b14f69..a9e65ffa06c2 100644 --- a/lucene/core/src/java/org/apache/lucene/store/ByteBufferGuard.java +++ b/lucene/core/src/java/org/apache/lucene/store/ByteBufferGuard.java @@ -62,7 +62,12 @@ public ByteBufferGuard(String resourceDescription, BufferCleaner cleaner) { public void invalidateAndUnmap(ByteBuffer... bufs) throws IOException { if (cleaner != null) { invalidated = true; - // Makes "invalidated" field visible to other threads. + // This call should hopefully flush any CPU caches and as a result make + // the "invalidated" field update visible to other threads. We specifically + // don't make "invalidated" field volatile for performance reasons, hoping the + // JVM won't optimize away reads of that field and hardware should ensure + // caches are in sync after this call. + // For previous implementation (based on `AtomicInteger#lazySet(0)`) see LUCENE-7409. VarHandle.fullFence(); // we give other threads a bit of time to finish reads on their ByteBuffer...: Thread.yield();