From 3a88b52b23c38e6a7d563399d80ac2692871f580 Mon Sep 17 00:00:00 2001 From: DarkAtra Date: Thu, 27 Aug 2026 23:48:29 +0200 Subject: [PATCH 1/3] fix(gh-1284): MVStore backend can grow uncontrollably --- .../no2/mvstore/MVStoreModuleBuilder.java | 27 ++- .../dizitart/no2/mvstore/MVStoreUtils.java | 19 ++- .../dizitart/no2/mvstore/NitriteMVMap.java | 154 ++++++++++++++---- .../no2/mvstore/NitriteMVRTreeMap.java | 139 ++++++++++++---- .../dizitart/no2/mvstore/NitriteMVStore.java | 37 ++++- .../dizitart/no2/mvstore/VersionUsage.java | 45 +++++ .../no2/integration/NitriteBuilderTest.java | 44 ++--- .../mvstore/MVStoreFileGrowthTest.java | 118 ++++++++++++++ .../no2/mvstore/NitriteMVMapTest.java | 126 +++++++++++--- .../no2/mvstore/NitriteMVRTreeMapTest.java | 115 +++++++++++++ .../no2/mvstore/NitriteMVStoreTest.java | 141 +++++++++++++++- 11 files changed, 830 insertions(+), 135 deletions(-) create mode 100644 nitrite-mvstore-adapter/src/main/java/org/dizitart/no2/mvstore/VersionUsage.java create mode 100644 nitrite-mvstore-adapter/src/test/java/org/dizitart/no2/integration/mvstore/MVStoreFileGrowthTest.java create mode 100644 nitrite-mvstore-adapter/src/test/java/org/dizitart/no2/mvstore/NitriteMVRTreeMapTest.java diff --git a/nitrite-mvstore-adapter/src/main/java/org/dizitart/no2/mvstore/MVStoreModuleBuilder.java b/nitrite-mvstore-adapter/src/main/java/org/dizitart/no2/mvstore/MVStoreModuleBuilder.java index 215ffbe2c..63bcdddc5 100644 --- a/nitrite-mvstore-adapter/src/main/java/org/dizitart/no2/mvstore/MVStoreModuleBuilder.java +++ b/nitrite-mvstore-adapter/src/main/java/org/dizitart/no2/mvstore/MVStoreModuleBuilder.java @@ -16,25 +16,26 @@ package org.dizitart.no2.mvstore; +import java.io.File; +import java.util.HashSet; +import java.util.Set; + +import org.dizitart.no2.store.events.StoreEventListener; +import org.h2.mvstore.FileStore; + import lombok.AccessLevel; import lombok.Getter; import lombok.Setter; import lombok.experimental.Accessors; -import org.dizitart.no2.store.events.StoreEventListener; -import org.h2.mvstore.FileStore; - -import java.io.File; -import java.util.HashSet; -import java.util.Set; /** * The MVStoreModuleBuilder class is responsible for building an instance of * {@link MVStoreModule}. It provides methods to set various configuration * options for the MVStore database. - * - * @since 4.0 - * @see MVStoreModule + * * @author Anindya Chatterjee + * @see MVStoreModule + * @since 4.0 */ @Getter @Setter @@ -81,6 +82,13 @@ public class MVStoreModuleBuilder { */ private boolean autoCommit = true; + /** + * Flag to enable/disable auto-compact mode. If set to true, fragmented + * chunks or chunks that are sufficiently below the target fill rate of 90% + * are reclaimed. This will typically shrink the file. + */ + private boolean autoCompact = true; + /** * Indicates whether the MVStore should be opened in recovery mode or not. */ @@ -174,6 +182,7 @@ public MVStoreModule build() { dbConfig.compress(compress()); dbConfig.compressHigh(compressHigh()); dbConfig.autoCommit(autoCommit()); + dbConfig.autoCompact(autoCompact()); dbConfig.recoveryMode(recoveryMode()); dbConfig.cacheSize(cacheSize()); dbConfig.cacheConcurrency(cacheConcurrency()); diff --git a/nitrite-mvstore-adapter/src/main/java/org/dizitart/no2/mvstore/MVStoreUtils.java b/nitrite-mvstore-adapter/src/main/java/org/dizitart/no2/mvstore/MVStoreUtils.java index e5c7cc7d9..e816ddd2e 100644 --- a/nitrite-mvstore-adapter/src/main/java/org/dizitart/no2/mvstore/MVStoreUtils.java +++ b/nitrite-mvstore-adapter/src/main/java/org/dizitart/no2/mvstore/MVStoreUtils.java @@ -16,17 +16,18 @@ package org.dizitart.no2.mvstore; -import lombok.extern.slf4j.Slf4j; +import static org.dizitart.no2.common.util.StringUtils.isNullOrEmpty; + +import java.io.File; +import java.util.Map; + import org.dizitart.no2.exceptions.InvalidOperationException; import org.dizitart.no2.exceptions.NitriteIOException; import org.dizitart.no2.mvstore.compat.v1.UpgradeUtil; import org.h2.mvstore.MVStore; import org.h2.mvstore.MVStoreException; -import java.io.File; -import java.util.Map; - -import static org.dizitart.no2.common.util.StringUtils.isNullOrEmpty; +import lombok.extern.slf4j.Slf4j; /** * @author Anindya Chatterjee. @@ -55,7 +56,7 @@ static MVStore openOrCreate(MVStoreConfig storeConfig) { } } catch (MVStoreException me) { if (me.getMessage().contains("file is locked")) { - throw new NitriteIOException("Database is already opened in other process"); + throw new NitriteIOException("Database is already opened in other process", me); } if (dbFile != null) { @@ -128,8 +129,10 @@ private static MVStore.Builder createBuilder(MVStoreConfig mvStoreConfig) { builder = builder.autoCommitBufferSize(mvStoreConfig.autoCommitBufferSize()); } - // auto compact disabled github issue #41 - builder.autoCompactFillRate(0); + if (!mvStoreConfig.autoCompact()) { + // disables background compaction + builder.autoCompactFillRate(0); + } if (mvStoreConfig.encryptionKey() != null) { builder = builder.encryptionKey(mvStoreConfig.encryptionKey()); diff --git a/nitrite-mvstore-adapter/src/main/java/org/dizitart/no2/mvstore/NitriteMVMap.java b/nitrite-mvstore-adapter/src/main/java/org/dizitart/no2/mvstore/NitriteMVMap.java index 7df7f6c60..8f775083d 100644 --- a/nitrite-mvstore-adapter/src/main/java/org/dizitart/no2/mvstore/NitriteMVMap.java +++ b/nitrite-mvstore-adapter/src/main/java/org/dizitart/no2/mvstore/NitriteMVMap.java @@ -16,45 +16,56 @@ package org.dizitart.no2.mvstore; +import static org.dizitart.no2.common.util.ValidationUtils.notNull; + +import java.lang.ref.Cleaner; +import java.util.Iterator; +import java.util.Map; +import java.util.NoSuchElementException; +import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.function.Supplier; + import org.dizitart.no2.common.RecordStream; import org.dizitart.no2.common.tuples.Pair; +import org.dizitart.no2.exceptions.NitriteIOException; import org.dizitart.no2.store.NitriteMap; import org.dizitart.no2.store.NitriteStore; import org.h2.mvstore.MVMap; import org.h2.mvstore.MVStore; -import java.util.Iterator; -import java.util.Map; -import java.util.concurrent.atomic.AtomicBoolean; - -import static org.dizitart.no2.common.util.ValidationUtils.notNull; - /** - * @since 1.0 * @author Anindya Chatterjee + * @since 1.0 */ class NitriteMVMap implements NitriteMap { + + private static final Cleaner CLEANER = Cleaner.create(); + private final MVMap mvMap; private final NitriteStore nitriteStore; private final MVStore mvStore; private final AtomicBoolean droppedFlag; private final AtomicBoolean closedFlag; + private final Set versionUsages; - NitriteMVMap(MVMap mvMap, NitriteStore nitriteStore) { + NitriteMVMap(final MVMap mvMap, final NitriteStore nitriteStore) { this.mvMap = mvMap; this.nitriteStore = nitriteStore; this.mvStore = mvMap.getStore(); this.closedFlag = new AtomicBoolean(false); this.droppedFlag = new AtomicBoolean(false); + this.versionUsages = ConcurrentHashMap.newKeySet(); } @Override - public boolean containsKey(Key key) { + public boolean containsKey(final Key key) { return mvMap.containsKey(key); } @Override - public Value get(Key key) { + public Value get(final Key key) { return mvMap.get(key); } @@ -65,7 +76,7 @@ public NitriteStore getStore() { @Override public void clear() { - MVStore.TxCounter txCounter = mvStore.registerVersionUsage(); + final MVStore.TxCounter txCounter = mvStore.registerVersionUsage(); try { mvMap.clear(); updateLastModifiedTime(); @@ -81,14 +92,14 @@ public String getName() { @Override public RecordStream values() { - return RecordStream.fromIterable(mvMap.values()); + return () -> versionedIterator(() -> mvMap.values().iterator()); } @Override - public Value remove(Key key) { - MVStore.TxCounter txCounter = mvStore.registerVersionUsage(); + public Value remove(final Key key) { + final MVStore.TxCounter txCounter = mvStore.registerVersionUsage(); try { - Value value = mvMap.remove(key); + final Value value = mvMap.remove(key); updateLastModifiedTime(); return value; } finally { @@ -98,13 +109,13 @@ public Value remove(Key key) { @Override public RecordStream keys() { - return RecordStream.fromIterable(mvMap.keySet()); + return () -> versionedIterator(() -> mvMap.keySet().iterator()); } @Override - public void put(Key key, Value value) { + public void put(final Key key, final Value value) { notNull(value, "value cannot be null"); - MVStore.TxCounter txCounter = mvStore.registerVersionUsage(); + final MVStore.TxCounter txCounter = mvStore.registerVersionUsage(); try { mvMap.put(key, value); updateLastModifiedTime(); @@ -119,11 +130,11 @@ public long size() { } @Override - public Value putIfAbsent(Key key, Value value) { + public Value putIfAbsent(final Key key, final Value value) { notNull(value, "value cannot be null"); - MVStore.TxCounter txCounter = mvStore.registerVersionUsage(); + final MVStore.TxCounter txCounter = mvStore.registerVersionUsage(); try { - Value v = mvMap.putIfAbsent(key, value); + final Value v = mvMap.putIfAbsent(key, value); updateLastModifiedTime(); return v; } finally { @@ -133,8 +144,8 @@ public Value putIfAbsent(Key key, Value value) { @Override public RecordStream> entries() { - return () -> new Iterator<>() { - final Iterator> entryIterator = mvMap.entrySet().iterator(); + return () -> versionedIterator(() -> new Iterator<>() { + private final Iterator> entryIterator = mvMap.entrySet().iterator(); @Override public boolean hasNext() { @@ -143,15 +154,19 @@ public boolean hasNext() { @Override public Pair next() { - Map.Entry entry = entryIterator.next(); + final Map.Entry entry = entryIterator.next(); return new Pair<>(entry.getKey(), entry.getValue()); } - }; + }); } @Override public RecordStream> reversedEntries() { - return () -> new ReverseIterator<>(mvMap); + return () -> versionedIterator(() -> new ReverseIterator<>(mvMap)); + } + + private Iterator versionedIterator(final Supplier> iteratorSupplier) { + return new VersionedIterator<>(mvStore, iteratorSupplier, versionUsages); } @Override @@ -165,22 +180,22 @@ public Key lastKey() { } @Override - public Key higherKey(Key key) { + public Key higherKey(final Key key) { return mvMap.higherKey(key); } @Override - public Key ceilingKey(Key key) { + public Key ceilingKey(final Key key) { return mvMap.ceilingKey(key); } @Override - public Key lowerKey(Key key) { + public Key lowerKey(final Key key) { return mvMap.lowerKey(key); } @Override - public Key floorKey(Key key) { + public Key floorKey(final Key key) { return mvMap.floorKey(key); } @@ -194,11 +209,12 @@ public void drop() { if (!droppedFlag.get()) { droppedFlag.compareAndSet(false, true); closedFlag.compareAndSet(false, true); + releaseVersionUsages(); - MVStore.TxCounter txCounter = mvStore.registerVersionUsage(); + final MVStore.TxCounter txCounter = mvStore.registerVersionUsage(); try { - nitriteStore.closeMap(getName()); - nitriteStore.removeMap(getName()); + nitriteStore.closeMap(mvMap.getName()); + nitriteStore.removeMap(mvMap.getName()); } finally { mvStore.deregisterVersionUsage(txCounter); } @@ -214,7 +230,8 @@ public boolean isDropped() { public void close() { if (!closedFlag.get() && !droppedFlag.get()) { closedFlag.compareAndSet(false, true); - nitriteStore.closeMap(getName()); + releaseVersionUsages(); + nitriteStore.closeMap(mvMap.getName()); } } @@ -222,4 +239,73 @@ public void close() { public boolean isClosed() { return closedFlag.get(); } + + private void releaseVersionUsages() { + for (final VersionUsage versionUsage : versionUsages) { + versionUsage.release(); + } + } + + private static class VersionedIterator implements Iterator { + + private final Iterator iterator; + private final Cleaner.Cleanable cleanable; + private final VersionUsage versionUsage; + private boolean exhausted; + + private VersionedIterator(final MVStore mvStore, + final Supplier> iteratorSupplier, + final Set versionUsages) { + + versionUsage = new VersionUsage(mvStore, mvStore.registerVersionUsage(), versionUsages); + versionUsages.add(versionUsage); + + try { + this.iterator = iteratorSupplier.get(); + this.cleanable = CLEANER.register(this, versionUsage::release); + } catch (final RuntimeException | Error e) { + versionUsage.release(); + throw e; + } + } + + @Override + public boolean hasNext() { + if (exhausted) { + return false; + } + ensureOpen(); + try { + final boolean hasNext = iterator.hasNext(); + if (!hasNext) { + exhausted = true; + cleanable.clean(); + } + return hasNext; + } catch (final RuntimeException | Error e) { + cleanable.clean(); + throw e; + } + } + + @Override + public Element next() { + if (exhausted) { + throw new NoSuchElementException(); + } + ensureOpen(); + try { + return iterator.next(); + } catch (final RuntimeException | Error e) { + cleanable.clean(); + throw e; + } + } + + private void ensureOpen() { + if (versionUsage.isReleased()) { + throw new NitriteIOException("MVStore is closed"); + } + } + } } diff --git a/nitrite-mvstore-adapter/src/main/java/org/dizitart/no2/mvstore/NitriteMVRTreeMap.java b/nitrite-mvstore-adapter/src/main/java/org/dizitart/no2/mvstore/NitriteMVRTreeMap.java index 219d22cd1..8816b9c1b 100644 --- a/nitrite-mvstore-adapter/src/main/java/org/dizitart/no2/mvstore/NitriteMVRTreeMap.java +++ b/nitrite-mvstore-adapter/src/main/java/org/dizitart/no2/mvstore/NitriteMVRTreeMap.java @@ -16,36 +16,47 @@ package org.dizitart.no2.mvstore; +import java.lang.ref.Cleaner; +import java.util.Iterator; +import java.util.NoSuchElementException; +import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; +import java.util.function.Supplier; + import org.dizitart.no2.collection.NitriteId; import org.dizitart.no2.common.RecordStream; +import org.dizitart.no2.exceptions.NitriteIOException; import org.dizitart.no2.index.BoundingBox; import org.dizitart.no2.store.NitriteRTree; import org.dizitart.no2.store.NitriteStore; import org.h2.mvstore.MVStore; import org.h2.mvstore.rtree.MVRTreeMap; -import java.util.Iterator; - /** - * @since 1.0 * @author Anindya Chatterjee + * @since 1.0 */ class NitriteMVRTreeMap implements NitriteRTree { + + private static final Cleaner CLEANER = Cleaner.create(); + private final MVRTreeMap mvMap; private final NitriteStore nitriteStore; private final MVStore mvStore; + private final Set versionUsages; - NitriteMVRTreeMap(MVRTreeMap mvMap, NitriteStore nitriteStore) { + NitriteMVRTreeMap(final MVRTreeMap mvMap, final NitriteStore nitriteStore) { this.mvMap = mvMap; this.nitriteStore = nitriteStore; this.mvStore = mvMap.getStore(); + this.versionUsages = ConcurrentHashMap.newKeySet(); } @Override - public void add(Key key, NitriteId nitriteId) { + public void add(final Key key, final NitriteId nitriteId) { if (nitriteId != null) { - MVSpatialKey spatialKey = getKey(key, nitriteId.getIdValue()); - MVStore.TxCounter txCounter = mvStore.registerVersionUsage(); + final MVSpatialKey spatialKey = getKey(key, nitriteId.getIdValue()); + final MVStore.TxCounter txCounter = mvStore.registerVersionUsage(); try { mvMap.add(spatialKey, key); } finally { @@ -55,10 +66,10 @@ public void add(Key key, NitriteId nitriteId) { } @Override - public void remove(Key key, NitriteId nitriteId) { + public void remove(final Key key, final NitriteId nitriteId) { if (nitriteId != null) { - MVSpatialKey spatialKey = getKey(key, nitriteId.getIdValue()); - MVStore.TxCounter txCounter = mvStore.registerVersionUsage(); + final MVSpatialKey spatialKey = getKey(key, nitriteId.getIdValue()); + final MVStore.TxCounter txCounter = mvStore.registerVersionUsage(); try { mvMap.remove(spatialKey); } finally { @@ -68,17 +79,15 @@ public void remove(Key key, NitriteId nitriteId) { } @Override - public RecordStream findIntersectingKeys(Key key) { - MVSpatialKey spatialKey = getKey(key, 0L); - MVRTreeMap.RTreeCursor treeCursor = mvMap.findIntersectingKeys(spatialKey); - return getRecordStream(treeCursor); + public RecordStream findIntersectingKeys(final Key key) { + final MVSpatialKey spatialKey = getKey(key, 0L); + return getRecordStream(() -> mvMap.findIntersectingKeys(spatialKey)); } @Override - public RecordStream findContainedKeys(Key key) { - MVSpatialKey spatialKey = getKey(key, 0L); - MVRTreeMap.RTreeCursor treeCursor = mvMap.findContainedKeys(spatialKey); - return getRecordStream(treeCursor); + public RecordStream findContainedKeys(final Key key) { + final MVSpatialKey spatialKey = getKey(key, 0L); + return getRecordStream(() -> mvMap.findContainedKeys(spatialKey)); } @Override @@ -86,7 +95,7 @@ public long size() { return mvMap.sizeAsLong(); } - private MVSpatialKey getKey(Key key, long id) { + private MVSpatialKey getKey(final Key key, final long id) { if (key == null || key.equals(BoundingBox.EMPTY)) { return new MVSpatialKey(id); } else { @@ -95,24 +104,14 @@ private MVSpatialKey getKey(Key key, long id) { } } - private RecordStream getRecordStream(MVRTreeMap.RTreeCursor treeCursor) { - //noinspection Convert2Diamond - return RecordStream.fromIterable(() -> new Iterator() { - @Override - public boolean hasNext() { - return treeCursor.hasNext(); - } - - @Override - public NitriteId next() { - MVSpatialKey next = (MVSpatialKey) treeCursor.next(); - return NitriteId.createId(Long.toString(next.getId())); - } - }); + private RecordStream getRecordStream( + final Supplier> cursorSupplier) { + return RecordStream.fromIterable(() -> new VersionedCursor(cursorSupplier)); } @Override public void close() { + releaseVersionUsages(); nitriteStore.closeRTree(mvMap.getName()); } @@ -123,8 +122,82 @@ public void clear() { @Override public void drop() { + releaseVersionUsages(); mvMap.clear(); nitriteStore.closeRTree(mvMap.getName()); nitriteStore.removeRTree(mvMap.getName()); } + + private void releaseVersionUsages() { + for (final VersionUsage versionUsage : versionUsages) { + versionUsage.release(); + } + } + + private class VersionedCursor implements Iterator { + + private final MVRTreeMap.RTreeCursor treeCursor; + private final Cleaner.Cleanable cleanable; + private final VersionUsage versionUsage; + private boolean exhausted; + + private VersionedCursor(final Supplier> cursorSupplier) { + + versionUsage = new VersionUsage(mvStore, mvStore.registerVersionUsage(), versionUsages); + versionUsages.add(versionUsage); + + try { + treeCursor = cursorSupplier.get(); + cleanable = CLEANER.register(this, versionUsage::release); + } catch (final RuntimeException | Error e) { + versionUsage.release(); + throw e; + } + } + + @Override + public boolean hasNext() { + if (exhausted) { + return false; + } + ensureOpen(); + try { + final boolean hasNext = treeCursor.hasNext(); + if (!hasNext) { + exhausted = true; + cleanable.clean(); + } + return hasNext; + } catch (final RuntimeException | Error e) { + cleanable.clean(); + throw e; + } + } + + @Override + public NitriteId next() { + if (exhausted) { + throw new NoSuchElementException(); + } + ensureOpen(); + try { + final MVSpatialKey next = (MVSpatialKey) treeCursor.next(); + if (next == null) { + exhausted = true; + cleanable.clean(); + throw new NoSuchElementException(); + } + return NitriteId.createId(Long.toString(next.getId())); + } catch (final RuntimeException | Error e) { + cleanable.clean(); + throw e; + } + } + + private void ensureOpen() { + if (versionUsage.isReleased()) { + throw new NitriteIOException("MVStore is closed"); + } + } + } } diff --git a/nitrite-mvstore-adapter/src/main/java/org/dizitart/no2/mvstore/NitriteMVStore.java b/nitrite-mvstore-adapter/src/main/java/org/dizitart/no2/mvstore/NitriteMVStore.java index 59fa58515..822e5bf2b 100644 --- a/nitrite-mvstore-adapter/src/main/java/org/dizitart/no2/mvstore/NitriteMVStore.java +++ b/nitrite-mvstore-adapter/src/main/java/org/dizitart/no2/mvstore/NitriteMVStore.java @@ -17,7 +17,17 @@ package org.dizitart.no2.mvstore; -import lombok.extern.slf4j.Slf4j; +import static org.h2.mvstore.DataUtils.ERROR_BLOCK_NOT_FOUND; +import static org.h2.mvstore.DataUtils.ERROR_CHUNK_NOT_FOUND; +import static org.h2.mvstore.DataUtils.ERROR_FILE_CORRUPT; +import static org.h2.mvstore.DataUtils.ERROR_READING_FAILED; +import static org.h2.mvstore.DataUtils.ERROR_SERIALIZATION; +import static org.h2.mvstore.DataUtils.ERROR_WRITING_FAILED; + +import java.util.Map; +import java.util.Properties; +import java.util.concurrent.ConcurrentHashMap; + import org.dizitart.no2.common.util.StringUtils; import org.dizitart.no2.exceptions.NitriteIOException; import org.dizitart.no2.index.BoundingBox; @@ -31,17 +41,15 @@ import org.h2.mvstore.MVStoreException; import org.h2.mvstore.rtree.MVRTreeMap; -import java.util.Map; -import java.util.concurrent.ConcurrentHashMap; - -import static org.h2.mvstore.DataUtils.*; +import lombok.extern.slf4j.Slf4j; /** - * @since 1.0 * @author Anindya Chatterjee + * @since 1.0 */ @Slf4j public class NitriteMVStore extends AbstractNitriteStore { + private MVStore mvStore; private final Map> nitriteMapRegistry; private final Map> nitriteRTreeMapRegistry; @@ -121,7 +129,22 @@ public void close() { nitriteRTreeMapRegistry.clear(); if (getStoreConfig().autoCompact()) { - mvStore.close(-1); + // FIXME: this a a hacky workaround for https://github.com/h2database/h2database/issues/4286 + // and should be removed once mvstore releases the upstream bugfix (probably in version 2.4.241) + final Properties systemProperties = System.getProperties(); + synchronized (systemProperties) { + final String originalCompactThreads = systemProperties.getProperty("h2.compactThreads"); + try { + systemProperties.setProperty("h2.compactThreads", "1"); + mvStore.close(-1); + } finally { + if (originalCompactThreads == null) { + systemProperties.remove("h2.compactThreads"); + } else { + systemProperties.setProperty("h2.compactThreads", originalCompactThreads); + } + } + } } else { mvStore.close(); } diff --git a/nitrite-mvstore-adapter/src/main/java/org/dizitart/no2/mvstore/VersionUsage.java b/nitrite-mvstore-adapter/src/main/java/org/dizitart/no2/mvstore/VersionUsage.java new file mode 100644 index 000000000..41be6319d --- /dev/null +++ b/nitrite-mvstore-adapter/src/main/java/org/dizitart/no2/mvstore/VersionUsage.java @@ -0,0 +1,45 @@ +/* + * Copyright (c) 2019-2020. Nitrite author or authors. + * + * Licensed 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.dizitart.no2.mvstore; + +import java.util.Set; +import java.util.concurrent.atomic.AtomicBoolean; + +import org.h2.mvstore.MVStore; + +import lombok.RequiredArgsConstructor; + +@RequiredArgsConstructor +class VersionUsage { + + private final AtomicBoolean released = new AtomicBoolean(false); + + private final MVStore mvStore; + private final MVStore.TxCounter txCounter; + private final Set versionUsages; + + boolean isReleased() { + return released.get(); + } + + void release() { + if (released.compareAndSet(false, true)) { + versionUsages.remove(this); + mvStore.deregisterVersionUsage(txCounter); + } + } +} diff --git a/nitrite-mvstore-adapter/src/test/java/org/dizitart/no2/integration/NitriteBuilderTest.java b/nitrite-mvstore-adapter/src/test/java/org/dizitart/no2/integration/NitriteBuilderTest.java index f49a6088f..72e0b8802 100644 --- a/nitrite-mvstore-adapter/src/test/java/org/dizitart/no2/integration/NitriteBuilderTest.java +++ b/nitrite-mvstore-adapter/src/test/java/org/dizitart/no2/integration/NitriteBuilderTest.java @@ -17,6 +17,26 @@ package org.dizitart.no2.integration; +import static org.dizitart.no2.collection.Document.createDocument; +import static org.dizitart.no2.common.module.NitriteModule.module; +import static org.dizitart.no2.common.util.StringUtils.isNullOrEmpty; +import static org.dizitart.no2.integration.TestUtil.createDb; +import static org.dizitart.no2.integration.TestUtil.getRandomTempDbFile; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertTrue; + +import java.io.BufferedWriter; +import java.io.File; +import java.io.FileWriter; +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Paths; +import java.util.LinkedHashSet; +import java.util.Random; + import org.dizitart.no2.Nitrite; import org.dizitart.no2.NitriteBuilder; import org.dizitart.no2.NitriteConfig; @@ -45,22 +65,6 @@ import org.junit.Rule; import org.junit.Test; -import java.io.BufferedWriter; -import java.io.File; -import java.io.FileWriter; -import java.io.IOException; -import java.nio.file.Files; -import java.nio.file.Paths; -import java.util.LinkedHashSet; -import java.util.Random; - -import static org.dizitart.no2.collection.Document.createDocument; -import static org.dizitart.no2.common.module.NitriteModule.module; -import static org.dizitart.no2.common.util.StringUtils.isNullOrEmpty; -import static org.dizitart.no2.integration.TestUtil.createDb; -import static org.dizitart.no2.integration.TestUtil.getRandomTempDbFile; -import static org.junit.Assert.*; - /** * @author Anindya Chatterjee. */ @@ -91,7 +95,7 @@ public void cleanup() { TestUtil.deleteDb(filePath); } - if (fakeDb != null && !fakeDb.isClosed()){ + if (fakeDb != null && !fakeDb.isClosed()) { fakeDb.close(); } @@ -119,7 +123,7 @@ public void testConfig() throws IOException { assertEquals(storeConfig.autoCommitBufferSize(), 1); assertEquals(config.findIndexer("Custom").getClass(), CustomIndexer.class); assertFalse(storeConfig.autoCommit()); - assertFalse(storeConfig.autoCompact()); + assertTrue(storeConfig.autoCompact()); assertTrue(storeConfig.compress()); assertFalse(storeConfig.isReadOnly()); assertFalse(storeConfig.isInMemory()); @@ -169,8 +173,8 @@ public void testConfigWithFile() { public void testConfigWithFileNull() { File file = null; MVStoreModule module = MVStoreModule.withConfig() - .filePath(file) - .build(); + .filePath(file) + .build(); db = Nitrite.builder().loadModule(module).openOrCreate(); StoreConfig storeConfig = db.getStore().getStoreConfig(); diff --git a/nitrite-mvstore-adapter/src/test/java/org/dizitart/no2/integration/mvstore/MVStoreFileGrowthTest.java b/nitrite-mvstore-adapter/src/test/java/org/dizitart/no2/integration/mvstore/MVStoreFileGrowthTest.java new file mode 100644 index 000000000..688c7ee2d --- /dev/null +++ b/nitrite-mvstore-adapter/src/test/java/org/dizitart/no2/integration/mvstore/MVStoreFileGrowthTest.java @@ -0,0 +1,118 @@ +/* + * Copyright (c) 2017-2021 Nitrite author or authors. + * + * Licensed 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.dizitart.no2.integration.mvstore; + +import static org.dizitart.no2.collection.Document.createDocument; +import static org.dizitart.no2.index.IndexOptions.indexOptions; +import static org.dizitart.no2.index.IndexType.NON_UNIQUE; +import static org.dizitart.no2.integration.TestUtil.createDb; +import static org.dizitart.no2.integration.TestUtil.getRandomTempDbFile; +import static org.junit.Assert.assertTrue; + +import java.io.File; +import java.time.Instant; +import java.util.concurrent.TimeUnit; + +import org.dizitart.no2.Nitrite; +import org.dizitart.no2.collection.NitriteCollection; +import org.junit.Test; + +public class MVStoreFileGrowthTest { + + @Test(timeout = 60_000) // for github issue #1284 + public void testRepeatedUpdatesReachBoundedFileGrowth() throws InterruptedException { + + final long initialFileSize, fileSizeAfterFirstUpdates, fileSizeAfterSecondUpdates, finalFileSize; + + final String dbPath = getRandomTempDbFile(); + final File dbFile = new File(dbPath); + + System.out.println("Database File lives in: " + dbFile.getAbsolutePath()); + + try (final Nitrite db = createDb(dbPath)) { + + final NitriteCollection collection = db.getCollection("file-growth"); + collection.createIndex("key"); + collection.createIndex(indexOptions(NON_UNIQUE), "revision"); + + System.out.println("Setting up the initial database documents..."); + for (int i = 0; i < 25; i++) { + collection.insert( + createDocument("key", i) + .put("revision", 0) + .put("lastUpdated", Instant.now().toString()) + ); + } + System.out.println("Collection '" + collection.getName() + "' now contains " + collection.size() + " elements."); + + if (db.hasUnsavedChanges()) { + db.commit(); + } + initialFileSize = dbFile.length(); + + System.out.println("Simulating frequent updates (1)..."); + updateDocuments(collection); + commitAndWaitForHousekeeping(db); + fileSizeAfterFirstUpdates = dbFile.length(); + + System.out.println("Simulating frequent updates (2)..."); + updateDocuments(collection); + commitAndWaitForHousekeeping(db); + fileSizeAfterSecondUpdates = dbFile.length(); + } + + finalFileSize = dbFile.length(); + + System.out.println("Initial file size: " + initialFileSize); + System.out.println("File size after first updates: " + fileSizeAfterFirstUpdates); + System.out.println("File size after second updates: " + fileSizeAfterSecondUpdates); + System.out.println("File size after close: " + finalFileSize); + + final long maxDeviationForInitial = Math.round(initialFileSize * 0.25); + assertTrue( + String.format("Initial file size (%d) and file size after close (%d) differ by more than the allowed 25%% (%d bytes)", initialFileSize, finalFileSize, maxDeviationForInitial), + Math.abs(initialFileSize - finalFileSize) <= maxDeviationForInitial + ); + + final long maxDeviationForUpdates = Math.round(fileSizeAfterFirstUpdates * 0.25); + assertTrue( + String.format("File size after second update loop (%d) is greater than file size after first update loop (%d) and differs by more than the allowed 25%% (%d bytes)", fileSizeAfterSecondUpdates, fileSizeAfterFirstUpdates, maxDeviationForUpdates), + fileSizeAfterSecondUpdates < fileSizeAfterFirstUpdates || Math.abs(fileSizeAfterFirstUpdates - fileSizeAfterSecondUpdates) <= maxDeviationForUpdates + ); + + assertTrue(finalFileSize < fileSizeAfterFirstUpdates); + assertTrue(finalFileSize < fileSizeAfterSecondUpdates); + } + + private void updateDocuments(final NitriteCollection collection) { + for (int i = 0; i < 2_000; i++) { + collection.find().forEach(document -> { + document.put("lastUpdated", Instant.now().toString()); + collection.update(document); + }); + } + } + + private void commitAndWaitForHousekeeping(final Nitrite db) throws InterruptedException { + if (db.hasUnsavedChanges()) { + db.commit(); + } + // housekeeping usually runs once every 333ms + // 5 seconds should be more than enough time for MVStore to write the compacted store to disk + TimeUnit.SECONDS.sleep(5); + } +} diff --git a/nitrite-mvstore-adapter/src/test/java/org/dizitart/no2/mvstore/NitriteMVMapTest.java b/nitrite-mvstore-adapter/src/test/java/org/dizitart/no2/mvstore/NitriteMVMapTest.java index 9ee2d3f16..931811d59 100644 --- a/nitrite-mvstore-adapter/src/test/java/org/dizitart/no2/mvstore/NitriteMVMapTest.java +++ b/nitrite-mvstore-adapter/src/test/java/org/dizitart/no2/mvstore/NitriteMVMapTest.java @@ -17,49 +17,137 @@ package org.dizitart.no2.mvstore; -import org.h2.mvstore.MVMap; -import org.junit.Test; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertThrows; +import static org.junit.Assert.assertTrue; +import static org.mockito.Mockito.inOrder; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; +import java.util.Arrays; +import java.util.Collections; import java.util.HashSet; +import java.util.Iterator; +import java.util.NoSuchElementException; -import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertTrue; -import static org.mockito.Mockito.*; +import org.dizitart.no2.exceptions.NitriteIOException; +import org.dizitart.no2.store.NitriteStore; +import org.h2.mvstore.MVMap; +import org.h2.mvstore.MVStore; +import org.junit.Before; +import org.junit.Test; +import org.mockito.InOrder; -@SuppressWarnings("unchecked") public class NitriteMVMapTest { + private MVMap mvMap; + private MVStore mvStore; + private MVStore.TxCounter txCounter; + private NitriteMVMap nitriteMVMap; + + @Before + public void setUp() { + //noinspection unchecked + mvMap = (MVMap) mock(MVMap.class); + mvStore = mock(MVStore.class); + txCounter = mock(MVStore.TxCounter.class); + NitriteStore nitriteStore = mock(NitriteStore.class); + when(mvMap.getStore()).thenReturn(mvStore); + when(mvStore.registerVersionUsage()).thenReturn(txCounter); + nitriteMVMap = new NitriteMVMap<>(mvMap, nitriteStore); + } + @Test public void testValues() { - NitriteMVMap nitriteMVMap = new NitriteMVMap<>( - (MVMap) mock(MVMap.class), null); + when(mvMap.values()).thenReturn(Collections.emptyList()); assertTrue(nitriteMVMap.values().toList().isEmpty()); + + InOrder inOrder = inOrder(mvStore, mvMap); + inOrder.verify(mvStore).registerVersionUsage(); + //noinspection ResultOfMethodCallIgnored + inOrder.verify(mvMap).values(); + inOrder.verify(mvStore).deregisterVersionUsage(txCounter); assertFalse(nitriteMVMap.isEmpty()); } @Test public void testKeys() { - MVMap objectObjectMap = (MVMap) mock(MVMap.class); - when(objectObjectMap.keySet()).thenReturn(new HashSet<>()); - NitriteMVMap nitriteMVMap = new NitriteMVMap<>(objectObjectMap, null); + when(mvMap.keySet()).thenReturn(new HashSet<>()); assertTrue(nitriteMVMap.keys().toList().isEmpty()); - verify(objectObjectMap).keySet(); + + verify(mvMap).keySet(); + verify(mvStore).deregisterVersionUsage(txCounter); assertFalse(nitriteMVMap.isEmpty()); } + @Test + public void testEntries() { + when(mvMap.entrySet()).thenReturn(Collections.singletonMap("key", "value").entrySet()); + assertEquals(1, nitriteMVMap.entries().toList().size()); + + verify(mvStore).registerVersionUsage(); + verify(mvStore).deregisterVersionUsage(txCounter); + } + + @Test + public void testReversedEntries() { + when(mvMap.getVersion()).thenReturn(1L); + when(mvMap.openVersion(1L)).thenReturn(mvMap); + when(mvMap.lastKey()).thenReturn(1); + when(mvMap.floorKey(1)).thenReturn(1); + + assertEquals(1, nitriteMVMap.reversedEntries().toList().size()); + verify(mvStore).registerVersionUsage(); + verify(mvStore).deregisterVersionUsage(txCounter); + } + + @Test + public void testIteratorCreationFailureReleasesVersion() { + when(mvMap.values()).thenThrow(new IllegalStateException()); + + assertThrows(IllegalStateException.class, () -> nitriteMVMap.values().iterator()); + verify(mvStore).registerVersionUsage(); + verify(mvStore).deregisterVersionUsage(txCounter); + } + + @Test + public void testExhaustedIteratorRemainsExhausted() { + when(mvMap.values()).thenReturn(Collections.singletonList("value")); + Iterator iterator = nitriteMVMap.values().iterator(); + + assertTrue(iterator.hasNext()); + assertEquals("value", iterator.next()); + assertFalse(iterator.hasNext()); + assertFalse(iterator.hasNext()); + assertThrows(NoSuchElementException.class, iterator::next); + } + + @Test + public void testAbandonedIteratorReleasesVersionOnClose() { + when(mvMap.values()).thenReturn(Arrays.asList("first", "second")); + Iterator iterator = nitriteMVMap.values().iterator(); + + assertEquals("first", iterator.next()); + verify(mvStore, never()).deregisterVersionUsage(txCounter); + + nitriteMVMap.close(); + verify(mvStore).deregisterVersionUsage(txCounter); + assertThrows(NitriteIOException.class, iterator::hasNext); + } + @Test(expected = NullPointerException.class) public void testConstructor() { - NitriteMVMap actualNitriteMVMap = new NitriteMVMap<>( - (MVMap) mock(MVMap.class), null); + NitriteMVMap actualNitriteMVMap = new NitriteMVMap<>(mvMap, null); actualNitriteMVMap.close(); assertFalse(actualNitriteMVMap.isEmpty()); } @Test public void testIsEmpty() { - MVMap objectObjectMap = (MVMap) mock(MVMap.class); - when(objectObjectMap.isEmpty()).thenReturn(true); - assertTrue((new NitriteMVMap<>(objectObjectMap, null)).isEmpty()); - verify(objectObjectMap).isEmpty(); + when(mvMap.isEmpty()).thenReturn(true); + assertTrue(nitriteMVMap.isEmpty()); + verify(mvMap).isEmpty(); } } - diff --git a/nitrite-mvstore-adapter/src/test/java/org/dizitart/no2/mvstore/NitriteMVRTreeMapTest.java b/nitrite-mvstore-adapter/src/test/java/org/dizitart/no2/mvstore/NitriteMVRTreeMapTest.java new file mode 100644 index 000000000..7a392e1b8 --- /dev/null +++ b/nitrite-mvstore-adapter/src/test/java/org/dizitart/no2/mvstore/NitriteMVRTreeMapTest.java @@ -0,0 +1,115 @@ +/* + * Copyright (c) 2017-2021 Nitrite author or authors. + * + * Licensed 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.dizitart.no2.mvstore; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertThrows; +import static org.junit.Assert.assertTrue; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.clearInvocations; +import static org.mockito.Mockito.inOrder; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.spy; +import static org.mockito.Mockito.verify; + +import java.util.Iterator; + +import org.dizitart.no2.collection.NitriteId; +import org.dizitart.no2.common.RecordStream; +import org.dizitart.no2.exceptions.NitriteIOException; +import org.dizitart.no2.index.BoundingBox; +import org.dizitart.no2.store.NitriteStore; +import org.h2.mvstore.MVStore; +import org.h2.mvstore.rtree.MVRTreeMap; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; +import org.mockito.InOrder; + +public class NitriteMVRTreeMapTest { + private MVStore mvStore; + private MVRTreeMap mvMap; + private NitriteMVRTreeMap nitriteMVRTreeMap; + + @Before + public void setUp() { + mvStore = spy(new MVStore.Builder().open()); + mvMap = spy(mvStore.openMap("test", new MVRTreeMap.Builder<>())); + NitriteStore nitriteStore = mock(NitriteStore.class); + nitriteMVRTreeMap = new NitriteMVRTreeMap<>(mvMap, nitriteStore); + + nitriteMVRTreeMap.add(new BoundingBox(0, 1, 0, 1), NitriteId.createId("1")); + clearInvocations(mvStore, mvMap); + } + + @After + public void tearDown() { + mvStore.closeImmediately(); + } + + @Test + public void testIntersectingCursorRetainsVersionUntilExhausted() { + RecordStream recordStream = + nitriteMVRTreeMap.findIntersectingKeys(new BoundingBox(-1, 2, -1, 2)); + verify(mvMap, never()).findIntersectingKeys(any()); + + Iterator iterator = recordStream.iterator(); + InOrder inOrder = inOrder(mvStore, mvMap); + inOrder.verify(mvStore).registerVersionUsage(); + inOrder.verify(mvMap).findIntersectingKeys(any()); + assertVersionRetainedUntilExhausted(iterator); + } + + @Test + public void testContainedCursorRetainsVersionUntilExhausted() { + RecordStream recordStream = + nitriteMVRTreeMap.findContainedKeys(new BoundingBox(-1, 2, -1, 2)); + verify(mvMap, never()).findContainedKeys(any()); + + Iterator iterator = recordStream.iterator(); + InOrder inOrder = inOrder(mvStore, mvMap); + inOrder.verify(mvStore).registerVersionUsage(); + inOrder.verify(mvMap).findContainedKeys(any()); + assertVersionRetainedUntilExhausted(iterator); + } + + @Test + public void testAbandonedCursorReleasesVersionOnClose() { + Iterator iterator = nitriteMVRTreeMap + .findIntersectingKeys(new BoundingBox(-1, 2, -1, 2)) + .iterator(); + + assertEquals(NitriteId.createId("1"), iterator.next()); + verify(mvStore, never()).deregisterVersionUsage(any()); + + nitriteMVRTreeMap.close(); + verify(mvStore).deregisterVersionUsage(any()); + assertThrows(NitriteIOException.class, iterator::hasNext); + } + + private void assertVersionRetainedUntilExhausted(Iterator iterator) { + assertTrue(iterator.hasNext()); + assertEquals(NitriteId.createId("1"), iterator.next()); + verify(mvStore, never()).deregisterVersionUsage(any()); + + assertFalse(iterator.hasNext()); + verify(mvStore).deregisterVersionUsage(any()); + } +} diff --git a/nitrite-mvstore-adapter/src/test/java/org/dizitart/no2/mvstore/NitriteMVStoreTest.java b/nitrite-mvstore-adapter/src/test/java/org/dizitart/no2/mvstore/NitriteMVStoreTest.java index 955748e22..67f69c3cb 100644 --- a/nitrite-mvstore-adapter/src/test/java/org/dizitart/no2/mvstore/NitriteMVStoreTest.java +++ b/nitrite-mvstore-adapter/src/test/java/org/dizitart/no2/mvstore/NitriteMVStoreTest.java @@ -17,14 +17,37 @@ package org.dizitart.no2.mvstore; -import org.junit.Test; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertThrows; +import static org.junit.Assert.assertTrue; +import static org.mockito.ArgumentMatchers.anyInt; +import static org.mockito.Mockito.doAnswer; +import static org.mockito.Mockito.mock; + +import java.lang.reflect.Field; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.Iterator; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; -import static org.junit.Assert.*; +import org.dizitart.no2.exceptions.NitriteIOException; +import org.dizitart.no2.store.NitriteMap; +import org.h2.mvstore.MVStore; +import org.junit.Test; public class NitriteMVStoreTest { + @Test public void testConstructor() { - NitriteMVStore actualNitriteMVStore = new NitriteMVStore(); + final NitriteMVStore actualNitriteMVStore = new NitriteMVStore(); assertNull(actualNitriteMVStore.getStoreConfig()); assertTrue(actualNitriteMVStore.isClosed()); assertFalse(actualNitriteMVStore.hasUnsavedChanges()); @@ -33,7 +56,7 @@ public void testConstructor() { @Test public void testOpenOrCreate() { - NitriteMVStore nitriteMVStore = new NitriteMVStore(); + final NitriteMVStore nitriteMVStore = new NitriteMVStore(); nitriteMVStore.setStoreConfig(new MVStoreConfig()); nitriteMVStore.openOrCreate(); assertFalse(nitriteMVStore.isReadOnly()); @@ -55,5 +78,113 @@ public void testHasUnsavedChanges() { public void testGetStoreVersion() { assertNotNull((new NitriteMVStore()).getStoreVersion()); } -} + @Test + public void testIteratorCannotReadAfterStoreClose() throws Exception { + + final Path storeFile = Files.createTempFile("nitrite-lifecycle-", ".db"); + Files.delete(storeFile); + final NitriteMVStore nitriteMVStore = new NitriteMVStore(); + final MVStoreConfig config = new MVStoreConfig(); + config.filePath(storeFile.toString()); + config.autoCompact(true); + nitriteMVStore.setStoreConfig(config); + + try { + nitriteMVStore.openOrCreate(); + final NitriteMap map = nitriteMVStore.openMap("test", Integer.class, String.class); + for (int i = 0; i < 100; i++) { + map.put(i, "value-" + i); + } + + final Iterator iterator = map.values().iterator(); + assertTrue(iterator.hasNext()); + iterator.next(); + + nitriteMVStore.close(); + + final NitriteIOException exception = assertThrows(NitriteIOException.class, iterator::hasNext); + assertEquals("MVStore is closed", exception.getMessage()); + } finally { + if (!nitriteMVStore.isClosed()) { + nitriteMVStore.close(); + } + Files.deleteIfExists(storeFile); + } + } + + @Test + public void testCompactingClosesAreSerialized() throws Exception { + + final String originalCompactThreads = System.getProperty("h2.compactThreads"); + final CountDownLatch firstCloseStarted = new CountDownLatch(1); + final CountDownLatch releaseFirstClose = new CountDownLatch(1); + final CountDownLatch secondCloseAttempted = new CountDownLatch(1); + final CountDownLatch secondCloseStarted = new CountDownLatch(1); + final CountDownLatch propertyChangeAttempted = new CountDownLatch(1); + final ExecutorService executorService = Executors.newFixedThreadPool(3); + + try { + System.setProperty("h2.compactThreads", "4"); + final MVStore firstMVStore = mock(MVStore.class); + doAnswer(invocation -> { + assertEquals("1", System.getProperty("h2.compactThreads")); + firstCloseStarted.countDown(); + assertTrue(releaseFirstClose.await(5, TimeUnit.SECONDS)); + return null; + }).when(firstMVStore).close(anyInt()); + + final MVStore secondMVStore = mock(MVStore.class); + doAnswer(invocation -> { + secondCloseStarted.countDown(); + assertEquals("1", System.getProperty("h2.compactThreads")); + return null; + }).when(secondMVStore).close(anyInt()); + + final NitriteMVStore firstStore = createCompactingStore(firstMVStore); + final NitriteMVStore secondStore = createCompactingStore(secondMVStore); + final Future firstClose = executorService.submit(firstStore::close); + assertTrue(firstCloseStarted.await(5, TimeUnit.SECONDS)); + + final Future secondClose = executorService.submit(() -> { + secondCloseAttempted.countDown(); + secondStore.close(); + }); + assertTrue(secondCloseAttempted.await(5, TimeUnit.SECONDS)); + assertFalse(secondCloseStarted.await(200, TimeUnit.MILLISECONDS)); + + final Future propertyChange = executorService.submit(() -> { + propertyChangeAttempted.countDown(); + System.setProperty("h2.compactThreads", "8"); + }); + assertTrue(propertyChangeAttempted.await(5, TimeUnit.SECONDS)); + assertThrows(TimeoutException.class, () -> propertyChange.get(200, TimeUnit.MILLISECONDS)); + + releaseFirstClose.countDown(); + firstClose.get(5, TimeUnit.SECONDS); + secondClose.get(5, TimeUnit.SECONDS); + propertyChange.get(5, TimeUnit.SECONDS); + assertEquals("8", System.getProperty("h2.compactThreads")); + } finally { + releaseFirstClose.countDown(); + executorService.shutdownNow(); + if (originalCompactThreads == null) { + System.clearProperty("h2.compactThreads"); + } else { + System.setProperty("h2.compactThreads", originalCompactThreads); + } + } + } + + private NitriteMVStore createCompactingStore(final MVStore mvStore) throws Exception { + final NitriteMVStore nitriteMVStore = new NitriteMVStore(); + final MVStoreConfig config = new MVStoreConfig(); + config.autoCompact(true); + nitriteMVStore.setStoreConfig(config); + + final Field mvStoreField = NitriteMVStore.class.getDeclaredField("mvStore"); + mvStoreField.setAccessible(true); + mvStoreField.set(nitriteMVStore, mvStore); + return nitriteMVStore; + } +} From e83104b51e61f715521234120d9c0cd2aa3314a4 Mon Sep 17 00:00:00 2001 From: Anindya Chatterjee Date: Mon, 31 Aug 2026 11:37:25 +0530 Subject: [PATCH 2/3] Bound the compaction fix's cost: a JVM-wide lock, a hidden skip, a racy test Three things the fix needed on top of the merge with main: - NitriteMVStore locked on System.getProperties() while compacting on close. Properties is a Hashtable, so that monitor gates every System.getProperty call in the JVM - a compacting close of a large store would stall unrelated code for its whole duration. A private lock serializes nitrite's own closes, which is all the property save/restore actually needs. - main's EntryIterator is a SkippableIterator, and wrapping it for version tracking hid that from BoundedStream, quietly turning indexed paging back into a walk. VersionedIterator now carries skip through. - MVStoreFileGrowthTest compared file sizes round to round with a 25% tolerance after a fixed sleep, which is what failed on the macOS runner (376832 against 212992). Growth is steppy and the housekeeping thread is not on a schedule the test controls, so it now asserts the chunk fill rate the issue itself reports - 37% reclaimed against 14-18% unreclaimed - and that close() compacts the file back to its live size. No sleeps, 2.3s instead of 12.3s, and each half fails on its own when the corresponding fix is reverted. Co-Authored-By: Claude Opus 5 --- .../dizitart/no2/mvstore/NitriteMVStore.java | 62 ++++++--- .../mvstore/MVStoreFileGrowthTest.java | 118 ------------------ .../no2/mvstore/MVStoreFileGrowthTest.java | 115 +++++++++++++++++ .../no2/mvstore/NitriteMVStoreTest.java | 15 +-- 4 files changed, 163 insertions(+), 147 deletions(-) delete mode 100644 nitrite-mvstore-adapter/src/test/java/org/dizitart/no2/integration/mvstore/MVStoreFileGrowthTest.java create mode 100644 nitrite-mvstore-adapter/src/test/java/org/dizitart/no2/mvstore/MVStoreFileGrowthTest.java diff --git a/nitrite-mvstore-adapter/src/main/java/org/dizitart/no2/mvstore/NitriteMVStore.java b/nitrite-mvstore-adapter/src/main/java/org/dizitart/no2/mvstore/NitriteMVStore.java index 822e5bf2b..c6a9c0f7d 100644 --- a/nitrite-mvstore-adapter/src/main/java/org/dizitart/no2/mvstore/NitriteMVStore.java +++ b/nitrite-mvstore-adapter/src/main/java/org/dizitart/no2/mvstore/NitriteMVStore.java @@ -25,7 +25,6 @@ import static org.h2.mvstore.DataUtils.ERROR_WRITING_FAILED; import java.util.Map; -import java.util.Properties; import java.util.concurrent.ConcurrentHashMap; import org.dizitart.no2.common.util.StringUtils; @@ -50,6 +49,9 @@ @Slf4j public class NitriteMVStore extends AbstractNitriteStore { + private static final String COMPACT_THREADS_PROPERTY = "h2.compactThreads"; + private static final Object COMPACT_THREADS_LOCK = new Object(); + private MVStore mvStore; private final Map> nitriteMapRegistry; private final Map> nitriteRTreeMapRegistry; @@ -129,22 +131,7 @@ public void close() { nitriteRTreeMapRegistry.clear(); if (getStoreConfig().autoCompact()) { - // FIXME: this a a hacky workaround for https://github.com/h2database/h2database/issues/4286 - // and should be removed once mvstore releases the upstream bugfix (probably in version 2.4.241) - final Properties systemProperties = System.getProperties(); - synchronized (systemProperties) { - final String originalCompactThreads = systemProperties.getProperty("h2.compactThreads"); - try { - systemProperties.setProperty("h2.compactThreads", "1"); - mvStore.close(-1); - } finally { - if (originalCompactThreads == null) { - systemProperties.remove("h2.compactThreads"); - } else { - systemProperties.setProperty("h2.compactThreads", originalCompactThreads); - } - } - } + compactAndClose(); } else { mvStore.close(); } @@ -219,6 +206,47 @@ public String getStoreVersion() { return "MVStore/" + org.h2.engine.Constants.VERSION; } + /** + * Closes the store with a full compaction, single-threaded. + * + *

MVStore reads h2.compactThreads on each compacting close, defaulting to a + * quarter of the available processors, and parallel compaction races on page references in + * 2.4.240 - see h2#4286. + * Pinning the property to 1 for the duration of the close is the upstream workaround; the + * lock is what keeps two concurrent closes from restoring each other's value. Both can go + * once the fix ships. + * + *

The lock is deliberately private rather than the Properties instance + * itself: Properties is a Hashtable, so holding its monitor across + * a compaction would stall every System.getProperty call in the JVM for as long + * as the compaction runs. + */ + private void compactAndClose() { + synchronized (COMPACT_THREADS_LOCK) { + final String originalCompactThreads = System.getProperty(COMPACT_THREADS_PROPERTY); + try { + System.setProperty(COMPACT_THREADS_PROPERTY, "1"); + mvStore.close(-1); + } finally { + if (originalCompactThreads == null) { + System.clearProperty(COMPACT_THREADS_PROPERTY); + } else { + System.setProperty(COMPACT_THREADS_PROPERTY, originalCompactThreads); + } + } + } + } + + /** + * The underlying MVStore, for tests that need to assert on store internals such as the + * chunk fill rate. + * + * @return the backing {@link MVStore}, or {@code null} before {@link #openOrCreate()} + */ + MVStore getMvStore() { + return mvStore; + } + private void initEventBus() { if (getStoreConfig().eventListeners() != null) { for (StoreEventListener eventListener : getStoreConfig().eventListeners()) { diff --git a/nitrite-mvstore-adapter/src/test/java/org/dizitart/no2/integration/mvstore/MVStoreFileGrowthTest.java b/nitrite-mvstore-adapter/src/test/java/org/dizitart/no2/integration/mvstore/MVStoreFileGrowthTest.java deleted file mode 100644 index 688c7ee2d..000000000 --- a/nitrite-mvstore-adapter/src/test/java/org/dizitart/no2/integration/mvstore/MVStoreFileGrowthTest.java +++ /dev/null @@ -1,118 +0,0 @@ -/* - * Copyright (c) 2017-2021 Nitrite author or authors. - * - * Licensed 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.dizitart.no2.integration.mvstore; - -import static org.dizitart.no2.collection.Document.createDocument; -import static org.dizitart.no2.index.IndexOptions.indexOptions; -import static org.dizitart.no2.index.IndexType.NON_UNIQUE; -import static org.dizitart.no2.integration.TestUtil.createDb; -import static org.dizitart.no2.integration.TestUtil.getRandomTempDbFile; -import static org.junit.Assert.assertTrue; - -import java.io.File; -import java.time.Instant; -import java.util.concurrent.TimeUnit; - -import org.dizitart.no2.Nitrite; -import org.dizitart.no2.collection.NitriteCollection; -import org.junit.Test; - -public class MVStoreFileGrowthTest { - - @Test(timeout = 60_000) // for github issue #1284 - public void testRepeatedUpdatesReachBoundedFileGrowth() throws InterruptedException { - - final long initialFileSize, fileSizeAfterFirstUpdates, fileSizeAfterSecondUpdates, finalFileSize; - - final String dbPath = getRandomTempDbFile(); - final File dbFile = new File(dbPath); - - System.out.println("Database File lives in: " + dbFile.getAbsolutePath()); - - try (final Nitrite db = createDb(dbPath)) { - - final NitriteCollection collection = db.getCollection("file-growth"); - collection.createIndex("key"); - collection.createIndex(indexOptions(NON_UNIQUE), "revision"); - - System.out.println("Setting up the initial database documents..."); - for (int i = 0; i < 25; i++) { - collection.insert( - createDocument("key", i) - .put("revision", 0) - .put("lastUpdated", Instant.now().toString()) - ); - } - System.out.println("Collection '" + collection.getName() + "' now contains " + collection.size() + " elements."); - - if (db.hasUnsavedChanges()) { - db.commit(); - } - initialFileSize = dbFile.length(); - - System.out.println("Simulating frequent updates (1)..."); - updateDocuments(collection); - commitAndWaitForHousekeeping(db); - fileSizeAfterFirstUpdates = dbFile.length(); - - System.out.println("Simulating frequent updates (2)..."); - updateDocuments(collection); - commitAndWaitForHousekeeping(db); - fileSizeAfterSecondUpdates = dbFile.length(); - } - - finalFileSize = dbFile.length(); - - System.out.println("Initial file size: " + initialFileSize); - System.out.println("File size after first updates: " + fileSizeAfterFirstUpdates); - System.out.println("File size after second updates: " + fileSizeAfterSecondUpdates); - System.out.println("File size after close: " + finalFileSize); - - final long maxDeviationForInitial = Math.round(initialFileSize * 0.25); - assertTrue( - String.format("Initial file size (%d) and file size after close (%d) differ by more than the allowed 25%% (%d bytes)", initialFileSize, finalFileSize, maxDeviationForInitial), - Math.abs(initialFileSize - finalFileSize) <= maxDeviationForInitial - ); - - final long maxDeviationForUpdates = Math.round(fileSizeAfterFirstUpdates * 0.25); - assertTrue( - String.format("File size after second update loop (%d) is greater than file size after first update loop (%d) and differs by more than the allowed 25%% (%d bytes)", fileSizeAfterSecondUpdates, fileSizeAfterFirstUpdates, maxDeviationForUpdates), - fileSizeAfterSecondUpdates < fileSizeAfterFirstUpdates || Math.abs(fileSizeAfterFirstUpdates - fileSizeAfterSecondUpdates) <= maxDeviationForUpdates - ); - - assertTrue(finalFileSize < fileSizeAfterFirstUpdates); - assertTrue(finalFileSize < fileSizeAfterSecondUpdates); - } - - private void updateDocuments(final NitriteCollection collection) { - for (int i = 0; i < 2_000; i++) { - collection.find().forEach(document -> { - document.put("lastUpdated", Instant.now().toString()); - collection.update(document); - }); - } - } - - private void commitAndWaitForHousekeeping(final Nitrite db) throws InterruptedException { - if (db.hasUnsavedChanges()) { - db.commit(); - } - // housekeeping usually runs once every 333ms - // 5 seconds should be more than enough time for MVStore to write the compacted store to disk - TimeUnit.SECONDS.sleep(5); - } -} diff --git a/nitrite-mvstore-adapter/src/test/java/org/dizitart/no2/mvstore/MVStoreFileGrowthTest.java b/nitrite-mvstore-adapter/src/test/java/org/dizitart/no2/mvstore/MVStoreFileGrowthTest.java new file mode 100644 index 000000000..aa4bbbca5 --- /dev/null +++ b/nitrite-mvstore-adapter/src/test/java/org/dizitart/no2/mvstore/MVStoreFileGrowthTest.java @@ -0,0 +1,115 @@ +/* + * Copyright (c) 2017-2021 Nitrite author or authors. + * + * Licensed 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.dizitart.no2.mvstore; + +import static org.dizitart.no2.collection.Document.createDocument; +import static org.dizitart.no2.index.IndexOptions.indexOptions; +import static org.dizitart.no2.index.IndexType.NON_UNIQUE; +import static org.dizitart.no2.integration.TestUtil.createDb; +import static org.dizitart.no2.integration.TestUtil.deleteDb; +import static org.dizitart.no2.integration.TestUtil.getRandomTempDbFile; +import static org.junit.Assert.assertTrue; + +import java.io.File; +import java.time.Instant; + +import org.dizitart.no2.Nitrite; +import org.dizitart.no2.collection.NitriteCollection; +import org.junit.Test; + +/** + * A store that is written to far more often than it grows must not grow anyway. + * + *

Repeatedly updating the same handful of documents leaves each chunk holding one live page and + * a great many obsolete ones. Nothing reclaims those chunks unless MVStore is allowed to compact, + * so the file climbs without bound while the live data stays a few kilobytes - the report in + * gh-1284 reached ~800MB around + * 100 live documents. + * + * @author Anindya Chatterjee + */ +public class MVStoreFileGrowthTest { + + private static final int LIVE_DOCUMENTS = 25; + private static final int UPDATES_PER_ROUND = 2_000; + + /** + * The fill rate the reclaimed store settles at, against the ~14-18% an unreclaimed one decays + * to over the same rounds. It is an equilibrium rather than a target: compaction runs on the + * back of write activity, so the rate stops falling but does not climb back once idle. + */ + private static final int MIN_CHUNK_FILL_RATE = 25; + + @Test(timeout = 120_000) + public void testRepeatedUpdatesDoNotStrandObsoleteChunks() { + final String dbPath = getRandomTempDbFile(); + final File dbFile = new File(dbPath); + final long initialFileSize; + final int chunkFillRate; + + try (final Nitrite db = createDb(dbPath)) { + final NitriteCollection collection = db.getCollection("file-growth"); + collection.createIndex("key"); + collection.createIndex(indexOptions(NON_UNIQUE), "revision"); + + for (int i = 0; i < LIVE_DOCUMENTS; i++) { + collection.insert(createDocument("key", i) + .put("revision", 0) + .put("lastUpdated", Instant.now().toString())); + } + db.commit(); + initialFileSize = dbFile.length(); + + // Two rounds, because the first is what fills the chunks and the second is what shows + // whether anything is reclaiming them. + updateEveryDocument(collection); + db.commit(); + updateEveryDocument(collection); + db.commit(); + + chunkFillRate = ((NitriteMVStore) db.getStore()).getMvStore() + .getFileStore().getChunksFillRate(); + } + + // close(-1) compacts synchronously, so this one is not subject to the housekeeping thread + // getting scheduled - the file is back to its live size by the time close() returns. + final long finalFileSize = dbFile.length(); + + try { + assertTrue(String.format( + "chunks are only %d%% live after %d updates - obsolete chunks are not being reclaimed", + chunkFillRate, 2 * UPDATES_PER_ROUND * LIVE_DOCUMENTS), + chunkFillRate >= MIN_CHUNK_FILL_RATE); + + assertTrue(String.format( + "file is %d bytes after close against %d bytes of the same live data - it was not compacted", + finalFileSize, initialFileSize), + finalFileSize <= initialFileSize); + } finally { + deleteDb(dbPath); + } + } + + private void updateEveryDocument(final NitriteCollection collection) { + for (int i = 0; i < UPDATES_PER_ROUND; i++) { + collection.find().forEach(document -> { + document.put("lastUpdated", Instant.now().toString()); + collection.update(document); + }); + } + } +} diff --git a/nitrite-mvstore-adapter/src/test/java/org/dizitart/no2/mvstore/NitriteMVStoreTest.java b/nitrite-mvstore-adapter/src/test/java/org/dizitart/no2/mvstore/NitriteMVStoreTest.java index 67f69c3cb..bda0c1ff0 100644 --- a/nitrite-mvstore-adapter/src/test/java/org/dizitart/no2/mvstore/NitriteMVStoreTest.java +++ b/nitrite-mvstore-adapter/src/test/java/org/dizitart/no2/mvstore/NitriteMVStoreTest.java @@ -36,7 +36,6 @@ import java.util.concurrent.Executors; import java.util.concurrent.Future; import java.util.concurrent.TimeUnit; -import java.util.concurrent.TimeoutException; import org.dizitart.no2.exceptions.NitriteIOException; import org.dizitart.no2.store.NitriteMap; @@ -121,8 +120,7 @@ public void testCompactingClosesAreSerialized() throws Exception { final CountDownLatch releaseFirstClose = new CountDownLatch(1); final CountDownLatch secondCloseAttempted = new CountDownLatch(1); final CountDownLatch secondCloseStarted = new CountDownLatch(1); - final CountDownLatch propertyChangeAttempted = new CountDownLatch(1); - final ExecutorService executorService = Executors.newFixedThreadPool(3); + final ExecutorService executorService = Executors.newFixedThreadPool(2); try { System.setProperty("h2.compactThreads", "4"); @@ -153,18 +151,11 @@ public void testCompactingClosesAreSerialized() throws Exception { assertTrue(secondCloseAttempted.await(5, TimeUnit.SECONDS)); assertFalse(secondCloseStarted.await(200, TimeUnit.MILLISECONDS)); - final Future propertyChange = executorService.submit(() -> { - propertyChangeAttempted.countDown(); - System.setProperty("h2.compactThreads", "8"); - }); - assertTrue(propertyChangeAttempted.await(5, TimeUnit.SECONDS)); - assertThrows(TimeoutException.class, () -> propertyChange.get(200, TimeUnit.MILLISECONDS)); - releaseFirstClose.countDown(); firstClose.get(5, TimeUnit.SECONDS); secondClose.get(5, TimeUnit.SECONDS); - propertyChange.get(5, TimeUnit.SECONDS); - assertEquals("8", System.getProperty("h2.compactThreads")); + // the second close restores what the first one saved, not the "1" it saw in flight + assertEquals("4", System.getProperty("h2.compactThreads")); } finally { releaseFirstClose.countDown(); executorService.shutdownNow(); From ea8e82a0624d2c7cd41a3caf5fe2dcd02665721e Mon Sep 17 00:00:00 2001 From: Anindya Chatterjee Date: Mon, 31 Aug 2026 12:00:57 +0530 Subject: [PATCH 3/3] One Cleaner for the adapter, and drop the sorted-page guard that has no stable ratio The two version-tracking maps each created their own Cleaner, so the adapter started two daemon threads to do one job. VersionUsage owns the one they share. CollectionSortedFindCostTest kept a wall-clock half alongside the clock-free one. It is failing on main's own HEAD (33330631456, Ubuntu): the indexed sort measured 2.596ms against the unindexed control's 2.139ms - slower than the thing it is supposed to beat by 2x, with the optimisation present and working. Its predecessor failed the same way on macOS. A shared runner does not hold a millisecond ratio still and no threshold fixes a measurement that inverts, so what is left is the FindPlan assertion that records the same decision directly and cannot be made to flake by a loaded machine. Co-Authored-By: Claude Opus 5 --- .../dizitart/no2/mvstore/NitriteMVMap.java | 4 +- .../no2/mvstore/NitriteMVRTreeMap.java | 4 +- .../dizitart/no2/mvstore/VersionUsage.java | 7 ++ .../CollectionSortedFindCostTest.java | 83 ++----------------- 4 files changed, 16 insertions(+), 82 deletions(-) diff --git a/nitrite-mvstore-adapter/src/main/java/org/dizitart/no2/mvstore/NitriteMVMap.java b/nitrite-mvstore-adapter/src/main/java/org/dizitart/no2/mvstore/NitriteMVMap.java index c498e4b99..0a2a195d5 100644 --- a/nitrite-mvstore-adapter/src/main/java/org/dizitart/no2/mvstore/NitriteMVMap.java +++ b/nitrite-mvstore-adapter/src/main/java/org/dizitart/no2/mvstore/NitriteMVMap.java @@ -45,8 +45,6 @@ */ class NitriteMVMap implements NitriteMap { - private static final Cleaner CLEANER = Cleaner.create(); - private final MVMap mvMap; private final NitriteStore nitriteStore; private final MVStore mvStore; @@ -312,7 +310,7 @@ private VersionedIterator(final MVStore mvStore, try { this.iterator = iteratorSupplier.get(); - this.cleanable = CLEANER.register(this, versionUsage::release); + this.cleanable = VersionUsage.CLEANER.register(this, versionUsage::release); } catch (final RuntimeException | Error e) { versionUsage.release(); throw e; diff --git a/nitrite-mvstore-adapter/src/main/java/org/dizitart/no2/mvstore/NitriteMVRTreeMap.java b/nitrite-mvstore-adapter/src/main/java/org/dizitart/no2/mvstore/NitriteMVRTreeMap.java index 8816b9c1b..e6ee72f82 100644 --- a/nitrite-mvstore-adapter/src/main/java/org/dizitart/no2/mvstore/NitriteMVRTreeMap.java +++ b/nitrite-mvstore-adapter/src/main/java/org/dizitart/no2/mvstore/NitriteMVRTreeMap.java @@ -38,8 +38,6 @@ */ class NitriteMVRTreeMap implements NitriteRTree { - private static final Cleaner CLEANER = Cleaner.create(); - private final MVRTreeMap mvMap; private final NitriteStore nitriteStore; private final MVStore mvStore; @@ -148,7 +146,7 @@ private VersionedCursor(final Supplier> cursorSuppli try { treeCursor = cursorSupplier.get(); - cleanable = CLEANER.register(this, versionUsage::release); + cleanable = VersionUsage.CLEANER.register(this, versionUsage::release); } catch (final RuntimeException | Error e) { versionUsage.release(); throw e; diff --git a/nitrite-mvstore-adapter/src/main/java/org/dizitart/no2/mvstore/VersionUsage.java b/nitrite-mvstore-adapter/src/main/java/org/dizitart/no2/mvstore/VersionUsage.java index 41be6319d..f8537be0c 100644 --- a/nitrite-mvstore-adapter/src/main/java/org/dizitart/no2/mvstore/VersionUsage.java +++ b/nitrite-mvstore-adapter/src/main/java/org/dizitart/no2/mvstore/VersionUsage.java @@ -16,6 +16,7 @@ package org.dizitart.no2.mvstore; +import java.lang.ref.Cleaner; import java.util.Set; import java.util.concurrent.atomic.AtomicBoolean; @@ -26,6 +27,12 @@ @RequiredArgsConstructor class VersionUsage { + /** + * Shared by every iterator and cursor in the adapter - one daemon thread is enough to release + * the versions of iterators that were abandoned rather than drained. + */ + static final Cleaner CLEANER = Cleaner.create(); + private final AtomicBoolean released = new AtomicBoolean(false); private final MVStore mvStore; diff --git a/nitrite-mvstore-adapter/src/test/java/org/dizitart/no2/integration/collection/CollectionSortedFindCostTest.java b/nitrite-mvstore-adapter/src/test/java/org/dizitart/no2/integration/collection/CollectionSortedFindCostTest.java index 1befebbd7..6bade76ec 100644 --- a/nitrite-mvstore-adapter/src/test/java/org/dizitart/no2/integration/collection/CollectionSortedFindCostTest.java +++ b/nitrite-mvstore-adapter/src/test/java/org/dizitart/no2/integration/collection/CollectionSortedFindCostTest.java @@ -39,7 +39,6 @@ import static org.dizitart.no2.integration.TestUtil.getRandomTempDbFile; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNull; -import static org.junit.Assert.assertTrue; /** * The cost half of {@link CollectionSortedFindTest}, which needs a store that actually @@ -51,13 +50,13 @@ * removes the decode: over 2000 rows carrying a 150-element array the sorted page went from * ~115 ms to ~2 ms. *

- * Both halves of every comparison here are the same collection shape sorted two different - * ways, never a fat collection against a lean one. That earlier framing assumed the index - * walk cancelled between the halves, and it does not: the fat collection's index lives in a - * file two orders of magnitude larger, so walking it costs more I/O for reasons that have - * nothing to do with decoding. Measured cold on macOS, a fat sorted page cost 4.6x a lean one - * with the optimisation present and working - through a 3x threshold, which is why this - * guard failed on that runner and nowhere else. + * The guard here has no clock in it. Two wall-clock versions of it were tried and both + * failed on CI while the optimisation was present and working - the first comparing fat rows + * against lean ones on macOS, the second comparing an indexed sort against an unindexed one on + * Ubuntu, where the indexed half measured slower than its control. A shared runner does + * not hold a millisecond ratio still, and no threshold rescues a measurement that inverts. The + * decision the optimisation actually makes is recorded in the {@link FindPlan}, so that is what + * is asserted. * * @author Anindya Chatterjee */ @@ -65,15 +64,6 @@ public class CollectionSortedFindCostTest { private static final int ROWS = 2000; private static final int PAYLOAD = 150; - /** - * Fresh collections per sample, because the cost under test is only visible on a cold one. - * Re-querying a collection that was just written measures a cache: over these same 2000 fat - * rows a blocking sort cost 353 ms, then 83 ms, then 0.8 ms. The old guard warmed once and - * timed the three runs after it, so it was comparing two numbers from which the decode - - * the whole subject of the test - had already been cached away. - */ - private static final int SAMPLES = 3; - private final String fileName = getRandomTempDbFile(); private Nitrite db; private int collectionSeq; @@ -121,65 +111,6 @@ public void testSortedPageIsPlannedFromTheIndex() { blocking.getFindPlan().getSortIndexDescriptor()); } - /** - * The same query, over the same rows, ordered by an indexed field and by an unindexed one. - * Only the second can be answered from an index; the first is what this optimisation - * exists to make cheap. Everything else - row count, document size, the file, the single - * row returned - is identical between the halves, so what is left is the decode of the - * {@link #ROWS} rows neither query should have returned. - *

- * Cold, and the best of {@link #SAMPLES}. Each half is measured once on a collection - * no query has touched, because a second look at the same collection measures a cache - * rather than a decode. That makes any single sample noisy, so each half is sampled on - * several fresh collections and the quickest is kept: contention can only ever add time, so - * the quickest run is the one least contaminated by the machine. - *

- * The threshold is 2x against a measured 4.5x on the worst run of a laptop under load and - * 45x on the best. A regression does not narrow this ratio - it removes it, because the two - * halves become the same code path and the ratio becomes 1. - */ - @Test - public void testSortedPageDoesNotDecodeTheRowsItDiscards() { - warmTheQueryPath(); - - double indexed = Double.MAX_VALUE; - double blocking = Double.MAX_VALUE; - for (int sample = 0; sample < SAMPLES; sample++) { - // Alternated so that any drift over the run lands on both halves alike. - blocking = Math.min(blocking, coldSortedPageCost("unindexed")); - indexed = Math.min(indexed, coldSortedPageCost("seq")); - } - - assertTrue("a sorted page over an indexed field took " + indexed + "ms against " - + blocking + "ms over an unindexed one, same rows and same documents - it is " - + "still decoding the rows it discards", blocking > indexed * 2); - } - - /** - * JIT only. Run on its own collection so the measured ones stay cache-cold. - */ - private void warmTheQueryPath() { - NitriteCollection collection = fatCollection("warm"); - for (int i = 0; i < 3; i++) { - drain(collection, "seq"); - drain(collection, "unindexed"); - } - } - - private double coldSortedPageCost(String sortField) { - NitriteCollection collection = fatCollection("cost"); - long start = System.nanoTime(); - drain(collection, sortField); - return (System.nanoTime() - start) / 1e6; - } - - private static void drain(NitriteCollection collection, String sortField) { - FindOptions page = FindOptions.orderBy(sortField, SortOrder.Descending).limit(1); - for (Document ignored : collection.find(ALL, page)) { - // force the fetch - } - } - /** * {@link #ROWS} rows carrying a {@link #PAYLOAD}-element array, except the one the page * returns. Descending on either sort field returns an end of the range, and giving that row