From 737acc8ad99ac227ebb6231a995296a8d43a49fc Mon Sep 17 00:00:00 2001 From: Lee Rhodes Date: Fri, 16 Aug 2024 18:26:09 -0700 Subject: [PATCH 1/2] Fix a memory leak in test. --- .../theta/DirectQuickSelectSketchTest.java | 30 +++++++++++++------ .../datasketches/theta/UnionImplTest.java | 14 ++++----- 2 files changed, 28 insertions(+), 16 deletions(-) diff --git a/src/test/java/org/apache/datasketches/theta/DirectQuickSelectSketchTest.java b/src/test/java/org/apache/datasketches/theta/DirectQuickSelectSketchTest.java index ccfbf704f..9b129daaf 100644 --- a/src/test/java/org/apache/datasketches/theta/DirectQuickSelectSketchTest.java +++ b/src/test/java/org/apache/datasketches/theta/DirectQuickSelectSketchTest.java @@ -77,15 +77,17 @@ public void checkBadSerVer() { @Test(expectedExceptions = SketchesArgumentException.class) public void checkConstructorKtooSmall() { int k = 8; - WritableMemory mem = makeNativeMemory(k); - UpdateSketch.builder().setNominalEntries(k).build(mem); + try (WritableMemory mem = makeNativeMemory(k)) { + UpdateSketch.builder().setNominalEntries(k).build(mem); + } } @Test(expectedExceptions = SketchesArgumentException.class) public void checkConstructorMemTooSmall() { int k = 16; - WritableMemory mem = makeNativeMemory(k/2); - UpdateSketch.builder().setNominalEntries(k).build(mem); + try (WritableMemory mem = makeNativeMemory(k/2)) { + UpdateSketch.builder().setNominalEntries(k).build(mem); + } } @Test(expectedExceptions = SketchesArgumentException.class) @@ -589,7 +591,7 @@ public void checkEstModeNativeMemory() { int u = 2*k; int memCapacity = (k << 4) + (Family.QUICKSELECT.getMinPreLongs() << 3); - try(WritableMemory mem = WritableMemory.allocateDirect(memCapacity)) { + try(WritableMemory mem = WritableMemory.allocateDirect(memCapacity)) { //will not request more memory UpdateSketch usk = UpdateSketch.builder().setNominalEntries(k).build(mem); DirectQuickSelectSketch sk1 = (DirectQuickSelectSketch)usk; //for internal checks assertTrue(usk.isEmpty()); @@ -599,6 +601,10 @@ public void checkEstModeNativeMemory() { println(""+est); assertEquals(usk.getEstimate(), u, u*.05); assertTrue(sk1.getRetainedEntries(false) > k); + Memory mem2 = usk.getMemory(); + assertTrue(mem2.isAlive()); + assertTrue(mem2.isDirect()); //still off heap. + assertTrue(mem2.isSameResource(mem)); } } @@ -774,12 +780,14 @@ public void checkMoveAndResize() { int k = 1 << 12; int u = 2 * k; int bytes = Sketches.getMaxUpdateSketchBytes(k); - WritableMemory wmem = WritableMemory.allocateDirect(bytes/2); //will request + WritableMemory wmem = WritableMemory.allocateDirect(bytes/2); //will request more memory UpdateSketch sketch = Sketches.updateSketchBuilder().setNominalEntries(k).build(wmem); assertTrue(sketch.isSameResource(wmem)); for (int i = 0; i < u; i++) { sketch.update(i); } - assertTrue(sketch.getMemory().isAlive()); - assertFalse(wmem.isAlive()); + Memory mem = sketch.getMemory(); + assertTrue(mem.isAlive()); + assertFalse(mem.isDirect()); //now on heap. + assertFalse(wmem.isAlive()); //wmem closed by MemoryRequestServer } @Test @@ -787,7 +795,7 @@ public void checkReadOnlyRebuildResize() { int k = 1 << 12; int u = 2 * k; int bytes = Sketches.getMaxUpdateSketchBytes(k); - WritableMemory wmem = WritableMemory.allocateDirect(bytes/2); //will request + WritableMemory wmem = WritableMemory.allocateDirect(bytes/2); //will request more memory UpdateSketch sketch = Sketches.updateSketchBuilder().setNominalEntries(k).build(wmem); for (int i = 0; i < u; i++) { sketch.update(i); } double est1 = sketch.getEstimate(); @@ -796,6 +804,10 @@ public void checkReadOnlyRebuildResize() { UpdateSketch roSketch = (UpdateSketch) Sketches.wrapSketch(mem); double est2 = roSketch.getEstimate(); assertEquals(est2, est1); + Memory mem2 = sketch.getMemory(); + assertTrue(mem2.isAlive()); + assertFalse(mem2.isDirect()); //now on heap + assertFalse(wmem.isAlive()); //wmem closed by MemoryRequestServer try { roSketch.rebuild(); fail(); diff --git a/src/test/java/org/apache/datasketches/theta/UnionImplTest.java b/src/test/java/org/apache/datasketches/theta/UnionImplTest.java index 31c26e4ec..6fd886835 100644 --- a/src/test/java/org/apache/datasketches/theta/UnionImplTest.java +++ b/src/test/java/org/apache/datasketches/theta/UnionImplTest.java @@ -191,8 +191,8 @@ public void checkMoveAndResizeOffHeap() { final int k = 1 << 12; final int u = 2 * k; final int bytes = Sketches.getMaxUpdateSketchBytes(k); - WritableMemory wmem = WritableMemory.allocateDirect(bytes / 2); //too small, forces new allocation on heap - WritableMemory wmem2 = WritableMemory.allocateDirect(bytes / 2); + WritableMemory wmem = WritableMemory.allocateDirect(bytes / 2); //not really used, except as a reference. + WritableMemory wmem2 = WritableMemory.allocateDirect(bytes / 2); //too small, forces new allocation on heap final UpdateSketch sketch = Sketches.updateSketchBuilder().setNominalEntries(k).build(wmem); assertTrue(sketch.isSameResource(wmem)); //also testing the isSameResource function @@ -200,12 +200,12 @@ public void checkMoveAndResizeOffHeap() { assertTrue(union.isSameResource(wmem2)); for (int i = 0; i < u; i++) { union.update(i); } - assertFalse(union.isSameResource(wmem)); + assertFalse(union.isSameResource(wmem)); //different Memories altogether final Union union2 = SetOperation.builder().buildUnion(); //on-heap union assertFalse(union2.isSameResource(wmem2)); //obviously not - wmem.close(); - //note wmem2 has already been closed by the DefaultMemoryRequestServer + wmem.close(); //empty, but we must close it anyway. + //note wmem2 has already been closed by the DefaultMemoryRequestServer. } @Test @@ -226,13 +226,13 @@ public void checkUnionCompactOrderedSource() { final double est1 = sk.getEstimate(); final int bytes = Sketches.getMaxCompactSketchBytes(sk.getRetainedEntries(true)); - try (WritableMemory wmem = WritableMemory.allocateDirect(bytes)) { + try (WritableMemory wmem = WritableMemory.allocateDirect(bytes)) { //sufficient memory final CompactSketch csk = sk.compact(true, wmem); //ordered, direct final Union union = Sketches.setOperationBuilder().buildUnion(); union.union(csk); final double est2 = union.getResult().getEstimate(); assertEquals(est2, est1); - } + } //wmem is closed here } @Test(expectedExceptions = SketchesArgumentException.class) From 0d75b7ffb7b784c37ae85ff906572f4981168018 Mon Sep 17 00:00:00 2001 From: Lee Rhodes Date: Sun, 18 Aug 2024 16:16:18 -0700 Subject: [PATCH 2/2] Changes suggested from review. --- src/test/java/org/apache/datasketches/theta/UnionImplTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/java/org/apache/datasketches/theta/UnionImplTest.java b/src/test/java/org/apache/datasketches/theta/UnionImplTest.java index 6fd886835..2fbd7bdcf 100644 --- a/src/test/java/org/apache/datasketches/theta/UnionImplTest.java +++ b/src/test/java/org/apache/datasketches/theta/UnionImplTest.java @@ -205,7 +205,7 @@ public void checkMoveAndResizeOffHeap() { final Union union2 = SetOperation.builder().buildUnion(); //on-heap union assertFalse(union2.isSameResource(wmem2)); //obviously not wmem.close(); //empty, but we must close it anyway. - //note wmem2 has already been closed by the DefaultMemoryRequestServer. + assertFalse(wmem2.isAlive());//previously closed via the DefaultMemoryRequestServer. } @Test