[#1096] Copy a value into Persistit once rather than twice, and write the large values of PDBStorageTest through a small buffer pool

[vharseko]

Fixes #1096.

PDBStorageTest.testCanAddLargeValues still ran the 512 MB test JVM out of heap after #1072 - build-maven (ubuntu-latest, 17) of #1057, in toByteArray() of PDBStorage.bytesToValue() on the 63 MB put. This takes both remaining options named under "Left out" of #1072.

Why

On the way into Persistit the 63 MB value still had three humongous arrays live at once: the source, the toByteArray() copy, and the 64 MB value buffer of the exchange - on top of the buffer pool of the test storage (76 MB, 20% of the heap) and the test server. Under -Xmx512m a G1 region is 1 MB, so each 63 MB array needs a free run of 64 regions, which humongous objects already in place can break up: whether it is there is up to the collector's timing.

The change

Main - PDBStorage.bytesToValue(). The bytes are copied once, straight into the encoded bytes of the Value: the header is what Persistit itself writes for an empty byte array (putByteArray(new byte[0])), then ensureFit(len), copyTo(getEncodedBytes(), header), setEncodedSize(header + len). The bytes stored are the same as before - Persistit encodes a byte array as the header followed by the bytes as they are (checked in Value.putByteArray1 of the 3.1.2 in use) - and no private constant of Persistit is copied. update() writes the new value it computes through the same bytesToValue() (round 2): it had kept putByteArray(newValue.toByteArray()). Every put and update of a PDB backend saves one allocation of the size of its value.

Test - PDBStorageTest.

• testCanAddLargeValues reopens its storage with a 16 MB buffer pool (createBackendCfg(LARGE_VALUES_DB_CACHE_SIZE)). The other methods keep db-cache-percent: the memory quota tests (aStorageWhoseOpenFailedGivesBackWhatItTook and its neighbours) are about that branch.
• testValuesReadBackAsWritten (new) pins the encoding: an empty value, a ByteString past the offset of its array, a ByteStringBuilder, and a 64 KB patterned value which outgrows the encoded bytes the value had - so the bytes have to land in the array ensureFit() put in place of the old one.
• testPutValueIsCopiedOnlyOnce and testUpdatedValueIsCopiedOnlyOnce (new, round 2) pin the single copy: the value is a Mockito mock delegating to a 64 KB ByteString, and neither put() nor update() may call its toByteArray().

Measured

testCanAddLargeValues alone, JDK 17.0.20, three runs per cell:

-Xmx	master	bytesToValue() only	this PR
352m	3/3 pass	3/3 pass	3/3 pass
320m	3/3 OOM	3/3 pass	3/3 pass
288m	3/3 OOM	3/3 pass	3/3 pass
256m	3/3 OOM	3/3 OOM	3/3 pass
224m	3/3 OOM	3/3 OOM	3/3 OOM - on the allocation of the 63 MB source itself

putByteArray(bytes.toByteArray()) back in bytesToValue() fails both spy cases, and the same call back in update() fails testUpdatedValueIsCopiedOnlyOnce; the round-trip case stays green on both, which is why the spies are there. Four mutants of bytesToValue() fail testValuesReadBackAsWritten: the bytes written over the header, no setEncodedSize(), the size without the header, and the encoded bytes read before ensureFit() (caught by the 64 KB value only: copyTo() truncates silently into the old array).

The whole class under -Xmx512m: 10/10 under JDK 17, 5/5 under JDK 26 on the first head (5031563).

Rebased onto master after #1066 (1aa253d), which touched the imports of PDBStorageTest as well: the only conflict was ResultCode and ByteStringBuilder added on the same line, and both are kept. The change itself is unchanged (git range-diff differs only in that context). On this head PDBStorageTest passes 28/28, the 13 cache size tests #1066 added included.

Round 2 (review of @maximthomas), rebased onto master after #1052, #1057 and #1091 (none touches PDB): update() goes through bytesToValue(), the two spy cases above, the javadoc of testCanAddLargeValues no longer says the test never fills a 20% pool (every long-record page goes through the pool; what costs the heap is that the pool allocates all its buffers when it is built), and the configuration comes from createBackendCfg(long). PDBStorageTest passes 30/30 under -Xmx512m, JDK 17.

Left out

The -Xmx512m of the test fork in the root pom.xml stays as it is. bytesToKey() keeps its copy: keys are small, and Key.appendByteArray() escapes the bytes rather than taking them as they are.

[vharseko]

@maximthomas I rebased onto master (d0e7584) after #1066 made this PR conflict. The only conflict was in the imports of PDBStorageTest: #1066 added ResultCode and this PR added ByteStringBuilder on the same line, and both are kept. PDBStorage.java merged on its own. The change itself is the same as before, and git range-diff differs only in that import context.

On this head PDBStorageTest passes 28/28. That includes testCanAddLargeValues, testValuesReadBackAsWritten and the 13 cache size tests from #1066. The description is updated to match. There were no review comments yet, so nothing else changed.

[maximthomas]

praise: The copy is removed where the OOME was measured, and the new body is correct against Persistit's own growth rules.

• PDBStorage.bytesToValue() reads getEncodedBytes() only after ensureFit() (PDBStorage.java:1546-1548) and says why. ensureFit(len) grows against _size + len and replaces _bytes, so reading the array first would be the silent-truncation bug.
• The header comes from Persistit itself (putByteArray(EMPTY_BYTES), :1544), so no private type constant is copied, and the stored bytes are identical to BASE's [CLASS_ARRAY, TYPE_BYTE, bytes].
• testValuesReadBackAsWritten's 64 KB value grows past the 256-byte initial buffer, so the case covers the road where ensureFit() puts a new array in place.

---

suggestion (non-blocking): No test pins the single copy that bytesToValue() now makes.

opendj-server-legacy/src/test/java/org/opends/server/backends/pdb/PDBStorageTest.java:195

If BASE's value.clear().putByteArray(bytes.toByteArray()) goes back into bytesToValue(), every test stays green. testValuesReadBackAsWritten only checks the round trip, and both bodies encode the same bytes and grow the buffer the same way. testCanAddLargeValues only checks that nothing throws, and the mutant now runs with a 16 MB pool. By your table, master's own OOME threshold is 320m, well below the fork's -Xmx512m. Both put() roads (PDBStorage.java:374, :444) reach the value only through bytesToValue(), so a spy on the source catches the extra copy. Its imports are org.forgerock.opendj.ldap.ByteSequence and static org.mockito.AdditionalAnswers.delegatesTo (Mockito 1.10.19).

  @Test
  public void testValueIsCopiedOnlyOnce() 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);
  }

Pin: BASE's body calls toByteArray() on the mock and turns the case red. The mutant was not run.

---

suggestion (non-blocking): update() still copies its new value twice, so "every put and update" in the description holds only for put.

opendj-server-legacy/src/main/java/org/opends/server/backends/pdb/PDBStorage.java:581

update() still writes ex.getValue().clear().putByteArray(newValue.toByteArray()), the same line as on master. Only the two put() roads go through the new bytesToValue(). This is not a regression: the 63 MB value of the test and ID2Entry both go through put(). newValue cannot share the Value's encoded bytes, because valueToBytes() wraps the decoded array of getByteArray(), so the helper can replace the call directly.

          else
          {
            bytesToValue(ex.getValue(), newValue);
            ex.store();
          }

Or: narrow the description to "every put".

---

nitpick (non-blocking): The javadoc of testCanAddLargeValues says the test never fills a 20% pool, but it does.

opendj-server-legacy/src/test/java/org/opends/server/backends/pdb/PDBStorageTest.java:159-161

Every long-record page goes through the buffer pool: LongRecordHelper calls allocPage() and, inside the transaction, calls writePage() before releasing the page. So the 4 + 32 + 63 MB (about 6,350 pages of 16 KB) cycle through a 76 MB pool, which holds at most about 4,860 buffers. What costs the heap is that BufferPool allocates every buffer when it is built.

   * 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.

---

nitpick (non-blocking): testCanAddLargeValues builds the configuration by hand, but createBackendCfg(long) already exists.

opendj-server-legacy/src/test/java/org/opends/server/backends/pdb/PDBStorageTest.java:167-168

createBackendCfg() is createBackendCfg(0L), and createBackendCfg(long cacheSize) (:916) builds the same mock with the size set. The SMALL_CACHE tests (:620, :660, :733, :756) use it.

    final PDBBackendCfg cfg = createBackendCfg(LARGE_VALUES_DB_CACHE_SIZE);

[vharseko]

@maximthomas Thanks. All four are taken in round 2 (8f608e9). I also rebased onto master first: #1052, #1057 and #1091 had gone in, and none of them touches PDB.

1. No test pinned the single copy. Taken, and extended to update(). testPutValueIsCopiedOnlyOnce is your case. testUpdatedValueIsCopiedOnlyOnce passes the same mock through an UpdateFunction. I ran the mutants you had not run. With putByteArray(bytes.toByteArray()) back in bytesToValue(), both spy cases fail. With the same call back in update(), the update case fails. testValuesReadBackAsWritten stays green on both.

2. update() still copied twice. Taken: it calls bytesToValue(ex.getValue(), newValue) now. On the previous head the update spy case was red, so the description's "every put and update" was wrong; with this change it holds, and the description is updated to say so.

3. The javadoc of testCanAddLargeValues. Taken with your wording. Confirmed in LongRecordHelper.storeLongRecord() of Persistit 3.1.2: every page goes through allocPage(), and inside a transaction through writePage().

4. createBackendCfg(long). Taken. It came in with #1066, after the first head of this PR.

PDBStorageTest passes 30/30 under -Xmx512m on JDK 17.

[maximthomas]

praise: The update road now makes the same single copy as the two put roads, and the new spies turn the old double copy red.

• PDBStorage.update() calls bytesToValue(ex.getValue(), newValue) (PDBStorage.java:581), so all three write roads (:374, :444, :581) share the one copy.
• bytesToValue() (PDBStorage.java:1542) reads getEncodedBytes() only after ensureFit(), and the comment says why.
• testPutValueIsCopiedOnlyOnce and testUpdatedValueIsCopiedOnlyOnce fail on putByteArray(bytes.toByteArray()); PDBStorageTest passes 30/30 at this head in a local failsafe run.