From 24df70ffa03b6670cd6bf998321a3035e18e8515 Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Thu, 24 Sep 2026 15:22:03 +0300 Subject: [PATCH 1/2] [#1096] Copy a value into Persistit once rather than twice, and write the large values of PDBStorageTest through a small buffer pool bytesToValue() copied each value into a fresh array with toByteArray() before Persistit copied it again into the value buffer of the exchange. The bytes now go straight into the encoded bytes of the value, behind the header Persistit itself writes for an empty byte array. For the 63 MB put of PDBStorageTest that is one humongous array fewer live next to the source and the 64 MB value buffer. testCanAddLargeValues also reopens its storage with a 16 MB buffer pool rather than 20% of the heap (76 MB under -Xmx512m); the other methods keep the percentage, which the memory quota tests are about. testCanAddLargeValues alone, JDK 17, three runs per heap: master runs out of heap from 320m down, the bytesToValue() change alone from 256m down, both together first at 224m, on the allocation of the 63 MB source itself. testValuesReadBackAsWritten pins the encoding: an empty value, one past the offset of its array, a ByteStringBuilder and a 64 KB value which outgrows the encoded bytes read back as written. --- .../server/backends/pdb/PDBStorage.java | 16 ++++- .../server/backends/pdb/PDBStorageTest.java | 58 +++++++++++++++++-- 2 files changed, 67 insertions(+), 7 deletions(-) diff --git a/opendj-server-legacy/src/main/java/org/opends/server/backends/pdb/PDBStorage.java b/opendj-server-legacy/src/main/java/org/opends/server/backends/pdb/PDBStorage.java index 916e5a0fb8..407b446ca1 100644 --- a/opendj-server-legacy/src/main/java/org/opends/server/backends/pdb/PDBStorage.java +++ b/opendj-server-legacy/src/main/java/org/opends/server/backends/pdb/PDBStorage.java @@ -137,6 +137,8 @@ public final class PDBStorage implements Storage, Backupable, ConfigurationChang private static final String JOURNAL_NAME = VOLUME_NAME + "_journal"; /** The buffer / page size used by the PersistIt storage. */ private static final int BUFFER_SIZE = 16 * 1024; + /** Encoded by {@link #bytesToValue(Value, ByteSequence)} for the header Persistit puts before a byte array. */ + private static final byte[] EMPTY_BYTES = new byte[0]; /** PersistIt implementation of the {@link Cursor} interface. */ private final class CursorImpl implements Cursor @@ -1530,9 +1532,21 @@ private static Key bytesToKey(final Key key, final ByteSequence bytes) return key.clear().appendByteArray(tmp, 0, tmp.length); } + /** + * Encodes the bytes as a byte array value, copying them once, straight into the encoded bytes of the value. + * {@code putByteArray(bytes.toByteArray())} would copy them twice, and for a value of 63 MB the extra copy is + * one more humongous array live next to the source and the value buffer. Persistit encodes a byte array as a + * header followed by the bytes as they are, so the header is taken from Persistit itself - by encoding an empty + * array - and the bytes are appended behind it. + */ private static Value bytesToValue(final Value value, final ByteSequence bytes) { - value.clear().putByteArray(bytes.toByteArray()); + value.clear().putByteArray(EMPTY_BYTES); + final int headerSize = value.getEncodedSize(); + value.ensureFit(bytes.length()); + // ensureFit() may have replaced the encoded bytes, so they are read only after it + bytes.copyTo(value.getEncodedBytes(), headerSize); + value.setEncodedSize(headerSize + bytes.length()); return value; } diff --git a/opendj-server-legacy/src/test/java/org/opends/server/backends/pdb/PDBStorageTest.java b/opendj-server-legacy/src/test/java/org/opends/server/backends/pdb/PDBStorageTest.java index 49f3929a66..1be50b8cf0 100644 --- a/opendj-server-legacy/src/test/java/org/opends/server/backends/pdb/PDBStorageTest.java +++ b/opendj-server-legacy/src/test/java/org/opends/server/backends/pdb/PDBStorageTest.java @@ -32,6 +32,7 @@ import org.forgerock.opendj.config.server.ConfigChangeResult; import org.forgerock.opendj.config.server.ConfigException; import org.forgerock.opendj.ldap.ByteString; +import org.forgerock.opendj.ldap.ByteStringBuilder; import org.forgerock.opendj.ldap.ResultCode; import org.opends.server.DirectoryServerTestCase; import org.opends.server.TestCaseUtils; @@ -64,6 +65,8 @@ public class PDBStorageTest extends DirectoryServerTestCase private static final long SHORT_RETRY_WINDOW_NANOS = 200L * 1000L * 1000L; //200 ms /** An attempt long enough to outlast {@link #SHORT_RETRY_WINDOW_NANOS} on its own, in milliseconds. */ private static final long ATTEMPT_LONGER_THAN_SHORT_WINDOW_MS = 300; + /** The buffer pool {@link #testCanAddLargeValues()} writes through, well under the 20% of the other methods. */ + private static final long LARGE_VALUES_DB_CACHE_SIZE = 16L * MB; private final TreeName treeName = new TreeName("dc=test", "test"); private ServerContext serverContext; @@ -150,16 +153,22 @@ private void reopenWithReplayBounds(int maxRetries, long retryWindowNanos) throw } /** - * The sources are wrapped rather than copied: a value on its way into Persistit is copied twice more - - * {@code toByteArray()} and the value buffer of the exchange, which doubles up to 64 MB - so with a copy - * here as well the 63 MB value had four copies of itself live at once, on top of the buffer pool and the - * server: about 310 MB left after a collection, in a JVM of 512 MB, which one CI leg ran out of. Wrapped, - * about 165 MB. The three values stay in one transaction on purpose: the value buffer the 32 MB one - * grew fits the 63 MB one without growing again. + * The sources are wrapped rather than copied, and the storage is reopened with a buffer pool of + * {@link #LARGE_VALUES_DB_CACHE_SIZE}: in a JVM of 512 MB each 63 MB array needs a free run of 64 regions, + * and two CI legs ran out of one. A value on its way into Persistit is copied once more, into the value buffer + * of the exchange, which doubles up to 64 MB; the 20% cache of the other methods would add 76 MB of buffers + * which this test never fills. The three values stay in one transaction on purpose: the value buffer the + * 32 MB one grew fits the 63 MB one without growing again. */ @Test public void testCanAddLargeValues() throws Exception { + closeAndRemove(storage); + final PDBBackendCfg cfg = createBackendCfg(); + when(cfg.getDBCacheSize()).thenReturn(LARGE_VALUES_DB_CACHE_SIZE); + storage = new PDBStorage(cfg, serverContext); + storage.open(AccessMode.READ_WRITE); + storage.write(new WriteOperation() { private final TreeName treeName = new TreeName("dc=test", "test"); @@ -176,6 +185,43 @@ public void run(WriteableTransaction txn) throws Exception }); } + /** + * A value is copied straight into the encoded bytes of the Persistit value, behind the header Persistit + * writes for a byte array: each of these reads back as it was written - an empty one, one which starts past + * the offset of the array behind it, one which is not a {@link ByteString} at all, and one which outgrows the + * encoded bytes the value had, so that it is copied into the ones {@code ensureFit()} put in their place. + */ + @Test + public void testValuesReadBackAsWritten() throws Exception + { + final ByteString empty = ByteString.empty(); + final ByteString inTheMiddle = wrap(new byte[] { 9, 1, 2, 3, 9 }, 1, 3); + final ByteStringBuilder builder = new ByteStringBuilder().appendUtf8("built"); + final byte[] patterned = new byte[64 * KB]; + for (int i = 0; i < patterned.length; i++) + { + patterned[i] = (byte) i; + } + final ByteString large = wrap(patterned); + createTree(); + storage.write(new WriteOperation() + { + @Override + public void run(WriteableTransaction txn) throws Exception + { + txn.put(treeName, valueOfUtf8("empty"), empty); + txn.put(treeName, valueOfUtf8("inTheMiddle"), inTheMiddle); + txn.put(treeName, valueOfUtf8("builder"), builder); + txn.put(treeName, valueOfUtf8("large"), large); + } + }); + + assertThat(read("empty")).isEqualTo(empty); + assertThat(read("inTheMiddle")).isEqualTo(valueOfBytes(new byte[] { 1, 2, 3 })); + assertThat(read("builder")).isEqualTo(valueOfUtf8("built")); + assertThat(read("large")).isEqualTo(large); + } + @Test public void testExchangeWithSmallValuesAreReleasedToPool() throws Exception { From 8f608e93e40b63b23d7a07bfbc8e95e8bbf05a49 Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Fri, 25 Sep 2026 10:28:34 +0300 Subject: [PATCH 2/2] [#1096] Copy the value of an update once as well, and pin the single copy of put and update update() wrote the new value it computed with putByteArray(newValue.toByteArray()), a second copy of the value; it now goes through bytesToValue() like the two put() roads. testPutValueIsCopiedOnlyOnce and testUpdatedValueIsCopiedOnlyOnce pass a Mockito mock that delegates to a 64 KB ByteString and check that its toByteArray() is never called: the round-trip case stays green when the extra copy comes back, since both bodies store the same bytes. The javadoc of testCanAddLargeValues no longer says the test never fills a 20% pool: every long-record page goes through the buffer pool; what the 20% would cost is the 76 MB of buffers the pool allocates when it is built. The configuration comes from createBackendCfg(long). --- .../server/backends/pdb/PDBStorage.java | 2 +- .../server/backends/pdb/PDBStorageTest.java | 63 +++++++++++++++++-- 2 files changed, 58 insertions(+), 7 deletions(-) diff --git a/opendj-server-legacy/src/main/java/org/opends/server/backends/pdb/PDBStorage.java b/opendj-server-legacy/src/main/java/org/opends/server/backends/pdb/PDBStorage.java index 407b446ca1..d398e49611 100644 --- a/opendj-server-legacy/src/main/java/org/opends/server/backends/pdb/PDBStorage.java +++ b/opendj-server-legacy/src/main/java/org/opends/server/backends/pdb/PDBStorage.java @@ -578,7 +578,7 @@ public boolean update(final TreeName treeName, final ByteSequence key, final Upd } else { - ex.getValue().clear().putByteArray(newValue.toByteArray()); + bytesToValue(ex.getValue(), newValue); ex.store(); } return true; diff --git a/opendj-server-legacy/src/test/java/org/opends/server/backends/pdb/PDBStorageTest.java b/opendj-server-legacy/src/test/java/org/opends/server/backends/pdb/PDBStorageTest.java index 1be50b8cf0..48bc046fa2 100644 --- a/opendj-server-legacy/src/test/java/org/opends/server/backends/pdb/PDBStorageTest.java +++ b/opendj-server-legacy/src/test/java/org/opends/server/backends/pdb/PDBStorageTest.java @@ -17,6 +17,7 @@ package org.opends.server.backends.pdb; import static org.assertj.core.api.Assertions.*; +import static org.mockito.AdditionalAnswers.delegatesTo; import static org.mockito.Mockito.*; import static org.forgerock.opendj.config.ConfigurationMock.*; import static org.opends.server.util.StaticUtils.*; @@ -31,6 +32,7 @@ import org.forgerock.i18n.LocalizableMessage; import org.forgerock.opendj.config.server.ConfigChangeResult; import org.forgerock.opendj.config.server.ConfigException; +import org.forgerock.opendj.ldap.ByteSequence; import org.forgerock.opendj.ldap.ByteString; import org.forgerock.opendj.ldap.ByteStringBuilder; import org.forgerock.opendj.ldap.ResultCode; @@ -43,6 +45,7 @@ import org.opends.server.backends.pluggable.spi.StorageInUseException; import org.opends.server.backends.pluggable.spi.StorageRuntimeException; import org.opends.server.backends.pluggable.spi.TreeName; +import org.opends.server.backends.pluggable.spi.UpdateFunction; import org.opends.server.backends.pluggable.spi.WriteOperation; import org.opends.server.backends.pluggable.spi.WriteableTransaction; import org.opends.server.core.DirectoryServer; @@ -156,17 +159,15 @@ private void reopenWithReplayBounds(int maxRetries, long retryWindowNanos) throw * The sources are wrapped rather than copied, and the storage is reopened with a buffer pool of * {@link #LARGE_VALUES_DB_CACHE_SIZE}: in a JVM of 512 MB each 63 MB array needs a free run of 64 regions, * and two CI legs ran out of one. A value on its way into Persistit is copied once more, into the value buffer - * of the exchange, which doubles up to 64 MB; the 20% cache of the other methods would add 76 MB of buffers - * which this test never fills. The three values stay in one transaction on purpose: the value buffer the - * 32 MB one grew fits the 63 MB one without growing again. + * of the exchange, which doubles up to 64 MB; the 20% cache of the other methods would allocate 76 MB of + * buffers up front, which this test does not need. The three values stay in one transaction on purpose: + * the value buffer the 32 MB one grew fits the 63 MB one without growing again. */ @Test public void testCanAddLargeValues() throws Exception { closeAndRemove(storage); - final PDBBackendCfg cfg = createBackendCfg(); - when(cfg.getDBCacheSize()).thenReturn(LARGE_VALUES_DB_CACHE_SIZE); - storage = new PDBStorage(cfg, serverContext); + storage = new PDBStorage(createBackendCfg(LARGE_VALUES_DB_CACHE_SIZE), serverContext); storage.open(AccessMode.READ_WRITE); storage.write(new WriteOperation() @@ -222,6 +223,56 @@ public void run(WriteableTransaction txn) throws Exception assertThat(read("large")).isEqualTo(large); } + /** + * A put copies the value once, straight into the encoded bytes of the Persistit value: the source is never + * asked for a copy of its own, which {@code putByteArray(bytes.toByteArray())} would make. + */ + @Test + public void testPutValueIsCopiedOnlyOnce() throws Exception + { + final ByteString large = wrap(new byte[64 * KB]); + final ByteSequence value = mock(ByteSequence.class, delegatesTo(large)); + createTree(); + storage.write(new WriteOperation() + { + @Override + public void run(WriteableTransaction txn) throws Exception + { + txn.put(treeName, valueOfUtf8("large"), value); + } + }); + + verify(value, never()).toByteArray(); + assertThat(read("large")).isEqualTo(large); + } + + /** The new value an update computes is copied once as well, the same way as the value of a put. */ + @Test + public void testUpdatedValueIsCopiedOnlyOnce() throws Exception + { + final ByteString large = wrap(new byte[64 * KB]); + final ByteSequence value = mock(ByteSequence.class, delegatesTo(large)); + createTree(); + storage.write(new WriteOperation() + { + @Override + public void run(WriteableTransaction txn) throws Exception + { + txn.update(treeName, valueOfUtf8("large"), new UpdateFunction() + { + @Override + public ByteSequence computeNewValue(ByteSequence oldValue) + { + return value; + } + }); + } + }); + + verify(value, never()).toByteArray(); + assertThat(read("large")).isEqualTo(large); + } + @Test public void testExchangeWithSmallValuesAreReleasedToPool() throws Exception {