magibney commented on code in PR #13570: URL: https://github.com/apache/lucene/pull/13570#discussion_r1686805197
########## lucene/core/src/java21/org/apache/lucene/store/RefCountedSharedArena.java: ########## @@ -0,0 +1,138 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.lucene.store; + +import java.lang.foreign.Arena; +import java.lang.foreign.MemorySegment; +import java.util.concurrent.atomic.AtomicInteger; + +/** + * A reference counted share Arena. + * + * <p>The purpose of this class is to allow a number of mmapped memory segments to be associated + * with a single underlying arena in order to avoid closing the underlying arena until all segments + * are closed. Typically, these memory segments belong to the same logical group, e.g. individual + * files of the same index segment. We do this to avoid the expensive cost of closing a shared + * Arena. + * + * <p>The reference count is increased by {@link #acquire()}, and decreased by {@link #release()}. + * When the reference count reaches 0, then the underlying arena is closed and the given {@code + * onClose} runnable is executed. No more references can be acquired. + * + * <p>The total number of acquires that can be obtained for the lifetime of an instance of this + * class is 1024. When the total number of acquires is exhausted, then not more acquires are + * permitted and {@link #acquire()} returns false. This is independent of the actual number of the + * ref count. + */ +@SuppressWarnings("preview") +final class RefCountedSharedArena implements Arena { + + static final int CLOSED = 0; + // initial state of 0x400 (1024) maximum permits, and a ref count of 0 + static final int INITIAL = 0x04000000; + + private final String segmentName; + private final Runnable onClose; + private final Arena arena; + + // high 16 bits contain the total remaining acquires; monotonically decreasing + // low 16 bit contain the current ref count + private final AtomicInteger state; + + RefCountedSharedArena(String segmentName, Runnable onClose) { + this.segmentName = segmentName; + this.onClose = onClose; + this.arena = Arena.ofShared(); + this.state = new AtomicInteger(INITIAL); + } + + // for debugging + String getSegmentName() { + return segmentName; + } + + /** + * Returns true if the ref count has been increased. Otherwise, false if there are no remaining + * acquires. + */ + boolean acquire() { + int value; + while (true) { + value = state.get(); + if (value == CLOSED) { + throw new IllegalStateException("closed"); + } + final int remaining = value >>> 16; + if (remaining == 0) { + return false; + } Review Comment: I think we must remove the `if (value == CLOSED)` ISE here, and simply return `false` below if `remaining == 0`. Unlike in `release()`, where we expect to hold a permit that would have prevented the entry from being closed, there's no such guarantee here. We can instead simply stick with `if (remaining == 0) return false`. ########## lucene/core/src/java21/org/apache/lucene/store/RefCountedSharedArena.java: ########## @@ -0,0 +1,138 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.lucene.store; + +import java.lang.foreign.Arena; +import java.lang.foreign.MemorySegment; +import java.util.concurrent.atomic.AtomicInteger; + +/** + * A reference counted share Arena. + * + * <p>The purpose of this class is to allow a number of mmapped memory segments to be associated + * with a single underlying arena in order to avoid closing the underlying arena until all segments + * are closed. Typically, these memory segments belong to the same logical group, e.g. individual + * files of the same index segment. We do this to avoid the expensive cost of closing a shared + * Arena. + * + * <p>The reference count is increased by {@link #acquire()}, and decreased by {@link #release()}. + * When the reference count reaches 0, then the underlying arena is closed and the given {@code + * onClose} runnable is executed. No more references can be acquired. + * + * <p>The total number of acquires that can be obtained for the lifetime of an instance of this + * class is 1024. When the total number of acquires is exhausted, then not more acquires are + * permitted and {@link #acquire()} returns false. This is independent of the actual number of the + * ref count. + */ +@SuppressWarnings("preview") +final class RefCountedSharedArena implements Arena { + + static final int CLOSED = 0; + // initial state of 0x400 (1024) maximum permits, and a ref count of 0 + static final int INITIAL = 0x04000000; + + private final String segmentName; + private final Runnable onClose; + private final Arena arena; + + // high 16 bits contain the total remaining acquires; monotonically decreasing + // low 16 bit contain the current ref count + private final AtomicInteger state; + + RefCountedSharedArena(String segmentName, Runnable onClose) { + this.segmentName = segmentName; + this.onClose = onClose; + this.arena = Arena.ofShared(); + this.state = new AtomicInteger(INITIAL); + } + + // for debugging + String getSegmentName() { + return segmentName; + } + + /** + * Returns true if the ref count has been increased. Otherwise, false if there are no remaining + * acquires. + */ + boolean acquire() { + int value; + while (true) { + value = state.get(); + if (value == CLOSED) { + throw new IllegalStateException("closed"); + } + final int remaining = value >>> 16; + if (remaining == 0) { + return false; + } + int newValue = ((remaining - 1) << 16) | ((value & 0xFFFF) + 1); + if (this.state.compareAndSet(value, newValue)) { + return true; + } + } + } + + /** Decrements the ref count. */ + void release() { + int value; + while (true) { + value = state.get(); + if (value == CLOSED) { + throw new IllegalStateException("closed"); + } + final int count = value & 0xFFFF; + if (count == 0) { + throw new IllegalStateException("nothing to release"); + } Review Comment: these 2 ISEs are effectively redundant, could be combined as: ```java final int count = value & 0xFFFF; if (count == 0) { throw new IllegalStateException(value == CLOSED ? "closed" : "nothing to release"); } ``` ########## lucene/core/src/java/org/apache/lucene/store/MMapDirectory.java: ########## @@ -256,15 +323,15 @@ default IOException convertMapFailedIOException( } } - private static MMapIndexInputProvider lookupProvider() { + private static <A> MMapIndexInputProvider<A> lookupProvider() { Review Comment: This is a minor point, but I wonder if at some point it'd be worth constraining the "attachment" type param to `interface MMapIndexInputProvider<A extends AutoCloseable>`. If attachment extends `AutoCloseable` that just feels cleaner from a lifecycle perspective, since the MMapDirectory that owns each attachment can then report back to the `MMapIndexInputProvider` when it's finished with the attachment (closing `attachment` in the `Directory.close()` method). Then again, given that the current implementations don't actually require this functionality, if there'd be no barrier to modifying the API if/when such functionality is actually required, there's no reason to accommodate it provisionally (can leave it as-is). (this thought occurred to me while considering handling all `Arena.close()` in a single-threaded executor within the MMapIndexInputProvider. If this is worth considering at all, it would be in a separate follow-up PR; I mention this here only as context for why I was thinking about making attachment `AutoCloseable`). ########## lucene/core/src/java21/org/apache/lucene/store/RefCountedSharedArena.java: ########## @@ -0,0 +1,138 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.lucene.store; + +import java.lang.foreign.Arena; +import java.lang.foreign.MemorySegment; +import java.util.concurrent.atomic.AtomicInteger; + +/** + * A reference counted share Arena. + * + * <p>The purpose of this class is to allow a number of mmapped memory segments to be associated + * with a single underlying arena in order to avoid closing the underlying arena until all segments + * are closed. Typically, these memory segments belong to the same logical group, e.g. individual + * files of the same index segment. We do this to avoid the expensive cost of closing a shared + * Arena. + * + * <p>The reference count is increased by {@link #acquire()}, and decreased by {@link #release()}. + * When the reference count reaches 0, then the underlying arena is closed and the given {@code + * onClose} runnable is executed. No more references can be acquired. + * + * <p>The total number of acquires that can be obtained for the lifetime of an instance of this + * class is 1024. When the total number of acquires is exhausted, then not more acquires are + * permitted and {@link #acquire()} returns false. This is independent of the actual number of the + * ref count. + */ +@SuppressWarnings("preview") +final class RefCountedSharedArena implements Arena { + + static final int CLOSED = 0; + // initial state of 0x400 (1024) maximum permits, and a ref count of 0 + static final int INITIAL = 0x04000000; Review Comment: If we were to make this configurable (e.g. via sysprop) we'd have to validate that the max possible max permits would be `0xffff`; ########## lucene/core/src/java21/org/apache/lucene/store/RefCountedSharedArena.java: ########## @@ -0,0 +1,138 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.lucene.store; + +import java.lang.foreign.Arena; +import java.lang.foreign.MemorySegment; +import java.util.concurrent.atomic.AtomicInteger; + +/** + * A reference counted share Arena. + * + * <p>The purpose of this class is to allow a number of mmapped memory segments to be associated + * with a single underlying arena in order to avoid closing the underlying arena until all segments + * are closed. Typically, these memory segments belong to the same logical group, e.g. individual + * files of the same index segment. We do this to avoid the expensive cost of closing a shared + * Arena. + * + * <p>The reference count is increased by {@link #acquire()}, and decreased by {@link #release()}. + * When the reference count reaches 0, then the underlying arena is closed and the given {@code + * onClose} runnable is executed. No more references can be acquired. + * + * <p>The total number of acquires that can be obtained for the lifetime of an instance of this + * class is 1024. When the total number of acquires is exhausted, then not more acquires are + * permitted and {@link #acquire()} returns false. This is independent of the actual number of the + * ref count. + */ +@SuppressWarnings("preview") +final class RefCountedSharedArena implements Arena { + + static final int CLOSED = 0; + // initial state of 0x400 (1024) maximum permits, and a ref count of 0 + static final int INITIAL = 0x04000000; + + private final String segmentName; + private final Runnable onClose; + private final Arena arena; + + // high 16 bits contain the total remaining acquires; monotonically decreasing + // low 16 bit contain the current ref count + private final AtomicInteger state; + + RefCountedSharedArena(String segmentName, Runnable onClose) { + this.segmentName = segmentName; + this.onClose = onClose; + this.arena = Arena.ofShared(); + this.state = new AtomicInteger(INITIAL); + } + + // for debugging + String getSegmentName() { + return segmentName; + } + + /** + * Returns true if the ref count has been increased. Otherwise, false if there are no remaining + * acquires. + */ + boolean acquire() { + int value; + while (true) { + value = state.get(); + if (value == CLOSED) { + throw new IllegalStateException("closed"); + } + final int remaining = value >>> 16; + if (remaining == 0) { + return false; + } + int newValue = ((remaining - 1) << 16) | ((value & 0xFFFF) + 1); + if (this.state.compareAndSet(value, newValue)) { Review Comment: semantically it would be less clear (feel free to ignore), but fwiw (for max possible max permits `<= 0x7fff`) these lines would be equivalent to: ```java private static final int REMAINING_UNIT = 1 << 16; private static final int ACQUIRE_DECREMENT = REMAINING_UNIT - 1; // 0xffff ... if (value < REMAINING_UNIT) { return false; } if (this.state.compareAndSet(value, value - ACQUIRE_DECREMENT)) { ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
