From 428641edda468d1fcfaf627151491240df83b787 Mon Sep 17 00:00:00 2001 From: david dali susanibar arce Date: Tue, 12 Sep 2023 10:00:49 -0500 Subject: [PATCH 01/18] feat: initial checker support --- java/pom.xml | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/java/pom.xml b/java/pom.xml index cbfe723436a..5a9fde4cfd4 100644 --- a/java/pom.xml +++ b/java/pom.xml @@ -46,6 +46,7 @@ 9+181-r4173-1 2.16 3.10.1 + 3.38.0 @@ -376,7 +377,7 @@ org.apache.maven.plugins maven-dependency-plugin - 3.0.1 + 3.6.0 org.apache.rat @@ -687,6 +688,12 @@ test + + org.checkerframework + checker-qual + ${checker.version} + + @@ -822,6 +829,7 @@ 8 UTF-8 + -Awarns -XDcompilePolicy=simple -Xplugin:ErrorProne -XepExcludedPaths:.*/(target/generated-sources)/.* -J--add-exports=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED @@ -841,7 +849,16 @@ error_prone_core ${error_prone_core.version} + + org.checkerframework + checker + ${checker.version} + + + + org.checkerframework.checker.nullness.NullnessChecker + From 4ac200216e8214111c41b2b6d08ab9fffa1fb9a8 Mon Sep 17 00:00:00 2001 From: david dali susanibar arce Date: Thu, 14 Sep 2023 09:29:51 -0500 Subject: [PATCH 02/18] feat: initial checker support --- java/flight/flight-sql-jdbc-driver/pom.xml | 10 ++++++++++ java/performance/pom.xml | 10 ++++++++++ java/pom.xml | 9 +++++---- 3 files changed, 25 insertions(+), 4 deletions(-) diff --git a/java/flight/flight-sql-jdbc-driver/pom.xml b/java/flight/flight-sql-jdbc-driver/pom.xml index 1fd9222be37..34830697a90 100644 --- a/java/flight/flight-sql-jdbc-driver/pom.xml +++ b/java/flight/flight-sql-jdbc-driver/pom.xml @@ -240,6 +240,16 @@ + + + org.apache.maven.plugins + maven-dependency-plugin + + + org.checkerframework:checker-qual + + + diff --git a/java/performance/pom.xml b/java/performance/pom.xml index 8653afca219..753c0e66964 100644 --- a/java/performance/pom.xml +++ b/java/performance/pom.xml @@ -175,6 +175,16 @@ + + + org.apache.maven.plugins + maven-dependency-plugin + + + org.checkerframework:checker-qual + + + diff --git a/java/pom.xml b/java/pom.xml index 5a9fde4cfd4..66d696f3af0 100644 --- a/java/pom.xml +++ b/java/pom.xml @@ -377,7 +377,7 @@ org.apache.maven.plugins maven-dependency-plugin - 3.6.0 + 3.1.2 org.apache.rat @@ -687,13 +687,11 @@ 0.9.44 test - org.checkerframework checker-qual ${checker.version} - @@ -829,7 +827,8 @@ 8 UTF-8 - -Awarns + -Awarns + -AskipDefs=.*Test -XDcompilePolicy=simple -Xplugin:ErrorProne -XepExcludedPaths:.*/(target/generated-sources)/.* -J--add-exports=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED @@ -856,6 +855,8 @@ + + org.immutables.value.internal.$processor$.$Processor org.checkerframework.checker.nullness.NullnessChecker From daaf63d3fcf5b1c872629d5f5989c77ef4fadff6 Mon Sep 17 00:00:00 2001 From: david dali susanibar arce Date: Fri, 15 Sep 2023 16:13:06 -0500 Subject: [PATCH 03/18] fix: tmp --- java/pom.xml | 229 +++++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 212 insertions(+), 17 deletions(-) diff --git a/java/pom.xml b/java/pom.xml index 66d696f3af0..a0c2ad9d819 100644 --- a/java/pom.xml +++ b/java/pom.xml @@ -687,11 +687,6 @@ 0.9.44 test - - org.checkerframework - checker-qual - ${checker.version} - @@ -763,6 +758,53 @@ + + checkerddsa + + + org.checkerframework + checker + ${checker.version} + provided + + + org.checkerframework + jdk8 + 3.3.0 + provided + + + + + + org.apache.maven.plugins + maven-dependency-plugin + + + + properties + + + + + + org.apache.maven.plugins + maven-compiler-plugin + + 8 + 8 + + org.checkerframework.checker.nullness.NullnessChecker + + + -Xbootclasspath/p:${settings.localRepository}/org/checkerframework/jdk8/3.3.0/jdk8-3.3.0.jar + + + + + + + shade-flatbuffers @@ -771,6 +813,55 @@ + + checker + + + org.checkerframework + checker + ${checker.version} + provided + + + org.checkerframework + jdk8 + 3.3.0 + provided + + + + + + org.apache.maven.plugins + maven-dependency-plugin + 2.10 + + + + properties + + + + + + org.apache.maven.plugins + maven-compiler-plugin + 3.3 + + 8 + 8 + + org.checkerframework.checker.nullness.NullnessChecker + + + -Xbootclasspath/p:${settings.localRepository}/org/checkerframework/jdk8/3.3.0/jdk8-3.3.0.jar + + + + + + + error-prone-jdk8 + org.apache.maven.plugins + maven-dependency-plugin + 2.10 + + + + properties + + + + org.apache.maven.plugins maven-compiler-plugin + 3.3 true - - -XDcompilePolicy=simple - -Xplugin:ErrorProne - -J-Xbootclasspath/p:${settings.localRepository}/com/google/errorprone/javac/${errorprone.javac.version}/javac-${errorprone.javac.version}.jar - - - - com.google.errorprone - error_prone_core - 2.4.0 - - + 8 + 8 + + org.checkerframework.checker.nullness.NullnessChecker + + + -Xbootclasspath/p:${settings.localRepository}/org/checkerframework/jdk8/3.3.0/jdk8-3.3.0.jar + + + + + + + + + + + + + + + + + + + + error-prowwwne-jdk8 + + + 1.8 + + !m2e.version + + + + + org.checkerframework + checker + ${checker.version} + provided + + + org.checkerframework + jdk8 + 3.3.0 + provided + + + + + + + org.apache.maven.plugins + maven-dependency-plugin + 2.10 + + + + properties + + + + + + org.apache.maven.plugins + maven-compiler-plugin + + true + 8 + 8 + + -Awarns + -AskipDefs=.*Test + -Xbootclasspath/p:${settings.localRepository}/org/checkerframework/jdk8/3.3.0/jdk8-3.3.0.jar + + + org.checkerframework.checker.nullness.NullnessChecker + + + + + + + error-prone-jdk11+ From 70146d19c415ecfd5700a3363d818c77d1777280 Mon Sep 17 00:00:00 2001 From: david dali susanibar arce Date: Sat, 16 Sep 2023 07:05:43 -0500 Subject: [PATCH 04/18] fix: review weekend --- java/pom.xml | 280 ++++++++++++++++----------------------------------- 1 file changed, 86 insertions(+), 194 deletions(-) diff --git a/java/pom.xml b/java/pom.xml index a0c2ad9d819..37b32557c07 100644 --- a/java/pom.xml +++ b/java/pom.xml @@ -616,6 +616,11 @@ + + org.checkerframework + checker-qual + ${checker.version} + org.slf4j @@ -758,53 +763,6 @@ - - checkerddsa - - - org.checkerframework - checker - ${checker.version} - provided - - - org.checkerframework - jdk8 - 3.3.0 - provided - - - - - - org.apache.maven.plugins - maven-dependency-plugin - - - - properties - - - - - - org.apache.maven.plugins - maven-compiler-plugin - - 8 - 8 - - org.checkerframework.checker.nullness.NullnessChecker - - - -Xbootclasspath/p:${settings.localRepository}/org/checkerframework/jdk8/3.3.0/jdk8-3.3.0.jar - - - - - - - shade-flatbuffers @@ -813,48 +771,81 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + - checker - - - org.checkerframework - checker - ${checker.version} - provided - - - org.checkerframework - jdk8 - 3.3.0 - provided - - + checkerframework + + + [1.8,) + - org.apache.maven.plugins - maven-dependency-plugin - 2.10 - - - - properties - - - - - - org.apache.maven.plugins maven-compiler-plugin - 3.3 + 3.10.1 - 8 - 8 + true + + + org.checkerframework + checker + 3.38.0 + + + com.google.errorprone + error_prone_core + 2.4.0 + + + org.checkerframework.checker.nullness.NullnessChecker - -Xbootclasspath/p:${settings.localRepository}/org/checkerframework/jdk8/3.3.0/jdk8-3.3.0.jar + -XDcompilePolicy=simple + -Xplugin:ErrorProne + -Xmaxerrs + 10000 + -Xmaxwarns + 10000 + @@ -862,124 +853,33 @@ - - error-prone-jdk8 - - - 1.8 - - !m2e.version - - - - - org.checkerframework - checker - ${checker.version} - provided - - - org.checkerframework - jdk8 - 3.3.0 - provided - - - - - - - org.apache.maven.plugins - maven-dependency-plugin - 2.10 - - - - properties - - - - - - org.apache.maven.plugins - maven-compiler-plugin - 3.3 - - true - 8 - 8 - - org.checkerframework.checker.nullness.NullnessChecker - - - -Xbootclasspath/p:${settings.localRepository}/org/checkerframework/jdk8/3.3.0/jdk8-3.3.0.jar - - - - - - - - - - - - - - - - - - - - - - - - - error-prowwwne-jdk8 - + checkerframework-jdk8 1.8 - - !m2e.version - - - - org.checkerframework - checker - ${checker.version} - provided - - - org.checkerframework - jdk8 - 3.3.0 - provided - - + + + 9+181-r4173-1 + - + org.apache.maven.plugins maven-dependency-plugin - 2.10 - properties + copy + process-sources + + com.google.errorprone:javac:${javac.version}:jar + ${project.build.directory}/javac + @@ -987,17 +887,9 @@ org.apache.maven.plugins maven-compiler-plugin - true - 8 - 8 - - -Awarns - -AskipDefs=.*Test - -Xbootclasspath/p:${settings.localRepository}/org/checkerframework/jdk8/3.3.0/jdk8-3.3.0.jar + + -J-Xbootclasspath/p:${project.build.directory}/javac/javac-${javac.version}.jar - - org.checkerframework.checker.nullness.NullnessChecker - From c10b5d5faf2f62e24f9916eefe40bbb104cdf25a Mon Sep 17 00:00:00 2001 From: david dali susanibar arce Date: Mon, 18 Sep 2023 11:46:17 -0500 Subject: [PATCH 05/18] fix: error jdk8 + version properties --- java/pom.xml | 225 ++++++++++++++++++++++----------------------------- 1 file changed, 96 insertions(+), 129 deletions(-) diff --git a/java/pom.xml b/java/pom.xml index 37b32557c07..98eea3f0c04 100644 --- a/java/pom.xml +++ b/java/pom.xml @@ -44,8 +44,10 @@ 2 true 9+181-r4173-1 - 2.16 - 3.10.1 + 2.10.0 + 2.21.1 + 2.9.3 + 3.10.1 3.38.0 @@ -392,7 +394,7 @@ org.apache.maven.plugins maven-compiler-plugin - ${maven-compiler-plugin.version} + ${maven.compiler.plugin.version} @@ -569,7 +571,7 @@ org.immutables value - 2.8.2 + ${immutables.version} provided @@ -771,133 +773,94 @@ - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - checkerframework - - - [1.8,) - - - - - maven-compiler-plugin - 3.10.1 - - true - - - org.checkerframework - checker - 3.38.0 - - - com.google.errorprone - error_prone_core - 2.4.0 - - - - - org.checkerframework.checker.nullness.NullnessChecker - - - -XDcompilePolicy=simple - -Xplugin:ErrorProne - -Xmaxerrs - 10000 - -Xmaxwarns - 10000 - - - - - - - + + error-prone-checkerframework-jdk8 + + 1.8 + + + + + maven-compiler-plugin + + true + + + org.checkerframework + checker + ${checker.version} + + + com.google.errorprone + error_prone_core + ${pinjdk8.errorprone.core.version} + + + org.immutables + value + ${immutables.version} + + + + + org.immutables.value.internal.$processor$.$Processor + + org.checkerframework.checker.nullness.NullnessChecker + + + -XDcompilePolicy=simple + -Xplugin:ErrorProne + -Xmaxerrs + 10000 + -Xmaxwarns + 10000 + -Awarns + -AskipDefs=.*Test + + + + + + - - checkerframework-jdk8 - - 1.8 - - - - 9+181-r4173-1 - - - - - - org.apache.maven.plugins - maven-dependency-plugin - - - - copy - - process-sources - - com.google.errorprone:javac:${javac.version}:jar - ${project.build.directory}/javac - - - - - - org.apache.maven.plugins - maven-compiler-plugin - - - -J-Xbootclasspath/p:${project.build.directory}/javac/javac-${javac.version}.jar - - - - - - + + checkerframework-jdk8 + + 1.8 + + + + + org.apache.maven.plugins + maven-dependency-plugin + + + + copy + + process-sources + + com.google.errorprone:javac:${errorprone.javac.version}:jar + ${project.build.directory}/javac + + + + + + org.apache.maven.plugins + maven-compiler-plugin + + + -J-Xbootclasspath/p:${project.build.directory}/javac/javac-${errorprone.javac.version}.jar + + + + + + - error-prone-jdk11+ + error-prone-checkerframework-jdk11+ [11,] @@ -914,6 +877,10 @@ 8 UTF-8 + -Xmaxerrs + 10000 + -Xmaxwarns + 10000 -Awarns -AskipDefs=.*Test -XDcompilePolicy=simple @@ -933,7 +900,7 @@ com.google.errorprone error_prone_core - ${error_prone_core.version} + ${errorprone.core.version} org.checkerframework From 71aef5323bbe65b6d578c5f34f617d1927f85968 Mon Sep 17 00:00:00 2001 From: david dali susanibar arce Date: Tue, 19 Sep 2023 12:36:40 -0500 Subject: [PATCH 06/18] fix: error JDK11+, update error prone version --- .../org/apache/arrow/memory/Accountant.java | 7 +++-- .../memory/AllocationOutcomeDetails.java | 4 ++- .../org/apache/arrow/memory/ArrowBuf.java | 31 ++++++++++--------- .../apache/arrow/memory/BaseAllocator.java | 23 ++++++++------ .../arrow/memory/util/ArrowBufPointer.java | 12 ++++--- .../apache/arrow/memory/util/MemoryUtil.java | 12 ++++++- .../arrow/memory/util/hash/MurmurHasher.java | 3 +- java/pom.xml | 14 +++++++-- .../vector/dictionary/DictionaryEncoder.java | 3 +- 9 files changed, 70 insertions(+), 39 deletions(-) diff --git a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/Accountant.java b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/Accountant.java index 87769dd122d..492ce06be50 100644 --- a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/Accountant.java +++ b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/Accountant.java @@ -22,6 +22,7 @@ import javax.annotation.concurrent.ThreadSafe; import org.apache.arrow.util.Preconditions; +import org.checkerframework.checker.nullness.qual.Nullable; /** * Provides a concurrent way to manage account for memory usage without locking. Used as basis @@ -59,7 +60,7 @@ class Accountant implements AutoCloseable { */ private final AtomicLong locallyHeldMemory = new AtomicLong(); - public Accountant(Accountant parent, String name, long reservation, long maxAllocation) { + public Accountant(@Nullable Accountant parent, String name, long reservation, long maxAllocation) { Preconditions.checkNotNull(name, "name must not be null"); Preconditions.checkArgument(reservation >= 0, "The initial reservation size must be non-negative."); Preconditions.checkArgument(maxAllocation >= 0, "The maximum allocation limit must be non-negative."); @@ -103,7 +104,7 @@ AllocationOutcome allocateBytes(long size) { } } - private AllocationOutcome.Status allocateBytesInternal(long size, AllocationOutcomeDetails details) { + private AllocationOutcome.Status allocateBytesInternal(long size, @Nullable AllocationOutcomeDetails details) { final AllocationOutcome.Status status = allocate(size, true /*incomingUpdatePeek*/, false /*forceAllocation*/, details); if (!status.isOk()) { @@ -168,7 +169,7 @@ public boolean forceAllocate(long size) { * @return The outcome of the allocation. */ private AllocationOutcome.Status allocate(final long size, final boolean incomingUpdatePeak, - final boolean forceAllocation, AllocationOutcomeDetails details) { + final boolean forceAllocation, @Nullable AllocationOutcomeDetails details) { final long oldLocal = locallyHeldMemory.getAndAdd(size); final long newLocal = oldLocal + size; // Borrowed from Math.addExact (but avoid exception here) diff --git a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/AllocationOutcomeDetails.java b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/AllocationOutcomeDetails.java index 6499ce84b1a..be0258995ab 100644 --- a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/AllocationOutcomeDetails.java +++ b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/AllocationOutcomeDetails.java @@ -20,6 +20,8 @@ import java.util.ArrayDeque; import java.util.Deque; +import org.checkerframework.checker.nullness.qual.Nullable; + /** * Captures details of allocation for each accountant in the hierarchical chain. */ @@ -47,7 +49,7 @@ void pushEntry(Accountant accountant, long totalUsedBeforeAllocation, long reque * Get the allocator that caused the failure. * @return the allocator that caused failure, null if there was no failure. */ - public BufferAllocator getFailedAllocator() { + public @Nullable BufferAllocator getFailedAllocator() { Entry top = allocEntries.peekLast(); if (top != null && top.allocationFailed && (top.accountant instanceof BufferAllocator)) { return (BufferAllocator) top.accountant; diff --git a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/ArrowBuf.java b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/ArrowBuf.java index 5b322b4ff56..cf441bae178 100644 --- a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/ArrowBuf.java +++ b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/ArrowBuf.java @@ -33,6 +33,7 @@ import org.apache.arrow.memory.util.MemoryUtil; import org.apache.arrow.util.Preconditions; import org.apache.arrow.util.VisibleForTesting; +import org.checkerframework.checker.nullness.qual.Nullable; /** * ArrowBuf serves as a facade over underlying memory by providing @@ -68,11 +69,11 @@ public final class ArrowBuf implements AutoCloseable { private static final int LOG_BYTES_PER_ROW = 10; private final long id = idGenerator.incrementAndGet(); private final ReferenceManager referenceManager; - private final BufferManager bufferManager; + private final @Nullable BufferManager bufferManager; private final long addr; private long readerIndex; private long writerIndex; - private final HistoricalLog historicalLog = BaseAllocator.DEBUG ? + private final @Nullable HistoricalLog historicalLog = BaseAllocator.DEBUG ? new HistoricalLog(BaseAllocator.DEBUG_LOG_LENGTH, "ArrowBuf[%d]", id) : null; private volatile long capacity; @@ -84,7 +85,7 @@ public final class ArrowBuf implements AutoCloseable { */ public ArrowBuf( final ReferenceManager referenceManager, - final BufferManager bufferManager, + final @Nullable BufferManager bufferManager, final long capacity, final long memoryAddress) { this.referenceManager = referenceManager; @@ -736,7 +737,7 @@ public void getBytes(long index, byte[] dst, int dstIndex, int length) { if (length != 0) { // copy "length" bytes from this ArrowBuf starting at addr(index) address // into dst byte array at dstIndex onwards - MemoryUtil.UNSAFE.copyMemory(null, addr(index), dst, MemoryUtil.BYTE_ARRAY_BASE_OFFSET + dstIndex, length); + MemoryUtil.copyMemory(null, addr(index), dst, MemoryUtil.BYTE_ARRAY_BASE_OFFSET + dstIndex, length); } } @@ -773,7 +774,7 @@ public void setBytes(long index, byte[] src, int srcIndex, long length) { if (length > 0) { // copy "length" bytes from src byte array at the starting index (srcIndex) // into this ArrowBuf starting at address "addr(index)" - MemoryUtil.UNSAFE.copyMemory(src, MemoryUtil.BYTE_ARRAY_BASE_OFFSET + srcIndex, null, addr(index), length); + MemoryUtil.copyMemory(src, MemoryUtil.BYTE_ARRAY_BASE_OFFSET + srcIndex, null, addr(index), length); } } @@ -799,7 +800,7 @@ public void getBytes(long index, ByteBuffer dst) { // at address srcAddress into the dst ByteBuffer starting at // address dstAddress final long dstAddress = MemoryUtil.getByteBufferAddress(dst) + dst.position(); - MemoryUtil.UNSAFE.copyMemory(null, srcAddress, null, dstAddress, dst.remaining()); + MemoryUtil.copyMemory(null, srcAddress, null, dstAddress, dst.remaining()); // after copy, bump the next write position for the dst ByteBuffer dst.position(dst.position() + dst.remaining()); } else if (dst.hasArray()) { @@ -807,7 +808,7 @@ public void getBytes(long index, ByteBuffer dst) { // at address srcAddress into the dst ByteBuffer starting at // index dstIndex final int dstIndex = dst.arrayOffset() + dst.position(); - MemoryUtil.UNSAFE.copyMemory( + MemoryUtil.copyMemory( null, srcAddress, dst.array(), MemoryUtil.BYTE_ARRAY_BASE_OFFSET + dstIndex, dst.remaining()); // after copy, bump the next write position for the dst ByteBuffer dst.position(dst.position() + dst.remaining()); @@ -836,14 +837,14 @@ public void setBytes(long index, ByteBuffer src) { // copy src.remaining() bytes of data from src ByteBuffer starting at // address srcAddress into this ArrowBuf starting at address dstAddress final long srcAddress = MemoryUtil.getByteBufferAddress(src) + src.position(); - MemoryUtil.UNSAFE.copyMemory(null, srcAddress, null, dstAddress, length); + MemoryUtil.copyMemory(null, srcAddress, null, dstAddress, length); // after copy, bump the next read position for the src ByteBuffer src.position(src.position() + length); } else if (src.hasArray()) { // copy src.remaining() bytes of data from src ByteBuffer starting at // index srcIndex into this ArrowBuf starting at address dstAddress final int srcIndex = src.arrayOffset() + src.position(); - MemoryUtil.UNSAFE.copyMemory( + MemoryUtil.copyMemory( src.array(), MemoryUtil.BYTE_ARRAY_BASE_OFFSET + srcIndex, null, dstAddress, length); // after copy, bump the next read position for the src ByteBuffer src.position(src.position() + length); @@ -896,7 +897,7 @@ public void setBytes(long index, ByteBuffer src, int srcIndex, int length) { // srcAddress into this ArrowBuf at address dstAddress final long srcAddress = MemoryUtil.getByteBufferAddress(src) + srcIndex; final long dstAddress = addr(index); - MemoryUtil.UNSAFE.copyMemory(null, srcAddress, null, dstAddress, length); + MemoryUtil.copyMemory(null, srcAddress, null, dstAddress, length); } else { if (srcIndex == 0 && src.capacity() == length) { // copy the entire ByteBuffer from start to end of length @@ -936,7 +937,7 @@ public void getBytes(long index, ArrowBuf dst, long dstIndex, int length) { // dstAddress final long srcAddress = addr(index); final long dstAddress = dst.memoryAddress() + (long) dstIndex; - MemoryUtil.UNSAFE.copyMemory(null, srcAddress, null, dstAddress, length); + MemoryUtil.copyMemory(null, srcAddress, null, dstAddress, length); } } @@ -966,7 +967,7 @@ public void setBytes(long index, ArrowBuf src, long srcIndex, long length) { // dstAddress final long srcAddress = src.memoryAddress() + srcIndex; final long dstAddress = addr(index); - MemoryUtil.UNSAFE.copyMemory(null, srcAddress, null, dstAddress, length); + MemoryUtil.copyMemory(null, srcAddress, null, dstAddress, length); } } @@ -986,7 +987,7 @@ public void setBytes(long index, ArrowBuf src) { checkIndex(index, length); final long srcAddress = src.memoryAddress() + src.readerIndex; final long dstAddress = addr(index); - MemoryUtil.UNSAFE.copyMemory(null, srcAddress, null, dstAddress, length); + MemoryUtil.copyMemory(null, srcAddress, null, dstAddress, length); src.readerIndex(src.readerIndex + length); } @@ -1011,7 +1012,7 @@ public int setBytes(long index, InputStream in, int length) throws IOException { if (readBytes > 0) { // copy readBytes length of data from the tmp byte array starting // at srcIndex 0 into this ArrowBuf starting at address addr(index) - MemoryUtil.UNSAFE.copyMemory(tmp, MemoryUtil.BYTE_ARRAY_BASE_OFFSET, null, addr(index), readBytes); + MemoryUtil.copyMemory(tmp, MemoryUtil.BYTE_ARRAY_BASE_OFFSET, null, addr(index), readBytes); } } return readBytes; @@ -1033,7 +1034,7 @@ public void getBytes(long index, OutputStream out, int length) throws IOExceptio // copy length bytes of data from this ArrowBuf starting at // address addr(index) into the tmp byte array starting at index 0 byte[] tmp = new byte[length]; - MemoryUtil.UNSAFE.copyMemory(null, addr(index), tmp, MemoryUtil.BYTE_ARRAY_BASE_OFFSET, length); + MemoryUtil.copyMemory(null, addr(index), tmp, MemoryUtil.BYTE_ARRAY_BASE_OFFSET, length); // write the copied data to output stream out.write(tmp); } diff --git a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/BaseAllocator.java b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/BaseAllocator.java index 9337f48b743..7a8d032d15c 100644 --- a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/BaseAllocator.java +++ b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/BaseAllocator.java @@ -30,6 +30,7 @@ import org.apache.arrow.memory.util.CommonUtil; import org.apache.arrow.memory.util.HistoricalLog; import org.apache.arrow.util.Preconditions; +import org.checkerframework.checker.nullness.qual.Nullable; import org.immutables.value.Value; /** @@ -42,7 +43,7 @@ abstract class BaseAllocator extends Accountant implements BufferAllocator { public static final String DEBUG_ALLOCATOR = "arrow.memory.debug.allocator"; public static final int DEBUG_LOG_LENGTH = 6; - public static final boolean DEBUG; + public static final @Nullable boolean DEBUG; private static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(BaseAllocator.class); // Initialize this before DEFAULT_CONFIG as DEFAULT_CONFIG will eventually initialize the allocation manager, @@ -64,15 +65,16 @@ abstract class BaseAllocator extends Accountant implements BufferAllocator { // Package exposed for sharing between AllocatorManger and BaseAllocator objects private final String name; private final RootAllocator root; - private final Object DEBUG_LOCK = DEBUG ? new Object() : null; + + private final @Nullable Object DEBUG_LOCK = DEBUG ? new Object() : null; private final AllocationListener listener; private final BaseAllocator parentAllocator; private final Map childAllocators; private final ArrowBuf empty; // members used purely for debugging - private final IdentityHashMap childLedgers; - private final IdentityHashMap reservations; - private final HistoricalLog historicalLog; + private final @Nullable IdentityHashMap childLedgers; + private final @Nullable IdentityHashMap reservations; + private final @Nullable HistoricalLog historicalLog; private final RoundingPolicy roundingPolicy; private final AllocationManager.Factory allocationManagerFactory; @@ -87,8 +89,9 @@ abstract class BaseAllocator extends Accountant implements BufferAllocator { * * @see Config */ + @SuppressWarnings({"nullness:assignment", "nullness:method.invocation"}) protected BaseAllocator( - final BaseAllocator parentAllocator, + final @Nullable BaseAllocator parentAllocator, final String name, final Config config) throws OutOfMemoryException { super(parentAllocator, name, config.getInitReservation(), config.getMaxAllocation()); @@ -154,7 +157,7 @@ private static String createErrorMsg(final BufferAllocator allocator, final long } } - public static boolean isDebug() { + public static @Nullable boolean isDebug() { return DEBUG; } @@ -183,6 +186,7 @@ public ArrowBuf getEmpty() { * we have a new ledger * associated with this allocator. */ + @SuppressWarnings({"nullness:locking.nullable", "nullness:argument"}) void associateLedger(BufferLedger ledger) { assertOpen(); if (DEBUG) { @@ -285,7 +289,7 @@ private ArrowBuf createEmpty() { } @Override - public ArrowBuf buffer(final long initialRequestSize, BufferManager manager) { + public ArrowBuf buffer(final long initialRequestSize, @Nullable BufferManager manager) { assertOpen(); Preconditions.checkArgument(initialRequestSize >= 0, "the requested size must be non-negative"); @@ -332,7 +336,7 @@ public ArrowBuf buffer(final long initialRequestSize, BufferManager manager) { */ private ArrowBuf bufferWithoutReservation( final long size, - BufferManager bufferManager) throws OutOfMemoryException { + @Nullable BufferManager bufferManager) throws OutOfMemoryException { assertOpen(); final AllocationManager manager = newAllocationManager(size); @@ -824,6 +828,7 @@ public class Reservation implements AllocationReservation { *

If {@linkplain #DEBUG} is true this will capture a historical * log of events relevant to this Reservation. */ + @SuppressWarnings({"nullness:argument"}) public Reservation() { if (DEBUG) { historicalLog = new HistoricalLog("Reservation[allocator[%s], %d]", name, System diff --git a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/ArrowBufPointer.java b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/ArrowBufPointer.java index fa1cfbdb293..d16b2c6d0f2 100644 --- a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/ArrowBufPointer.java +++ b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/ArrowBufPointer.java @@ -21,6 +21,7 @@ import org.apache.arrow.memory.util.hash.ArrowBufHasher; import org.apache.arrow.memory.util.hash.SimpleHasher; import org.apache.arrow.util.Preconditions; +import org.checkerframework.checker.nullness.qual.Nullable; /** * Pointer to a memory region within an {@link ArrowBuf}. @@ -33,7 +34,7 @@ public final class ArrowBufPointer { */ public static final int NULL_HASH_CODE = 0; - private ArrowBuf buf; + private @Nullable ArrowBuf buf; private long offset; @@ -81,7 +82,8 @@ public ArrowBufPointer(ArrowBuf buf, long offset, long length) { * @param length the length off set of the memory region pointed to. * @param hasher the hasher used to calculate the hash code. */ - public ArrowBufPointer(ArrowBuf buf, long offset, long length, ArrowBufHasher hasher) { + @SuppressWarnings("initialization.fields.uninitialized") + public ArrowBufPointer(@Nullable ArrowBuf buf, long offset, long length, ArrowBufHasher hasher) { Preconditions.checkNotNull(hasher); this.hasher = hasher; set(buf, offset, length); @@ -93,7 +95,7 @@ public ArrowBufPointer(ArrowBuf buf, long offset, long length, ArrowBufHasher ha * @param offset the start off set of the memory region pointed to. * @param length the length off set of the memory region pointed to. */ - public void set(ArrowBuf buf, long offset, long length) { + public void set(@Nullable ArrowBuf buf, long offset, long length) { this.buf = buf; this.offset = offset; this.length = length; @@ -105,7 +107,7 @@ public void set(ArrowBuf buf, long offset, long length) { * Gets the underlying buffer, or null if the underlying data is invalid or null. * @return the underlying buffer, if any, or null if the underlying data is invalid or null. */ - public ArrowBuf getBuf() { + public @Nullable ArrowBuf getBuf() { return buf; } @@ -118,7 +120,7 @@ public long getLength() { } @Override - public boolean equals(Object o) { + public boolean equals(@Nullable Object o) { if (this == o) { return true; } diff --git a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/MemoryUtil.java b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/MemoryUtil.java index b83adf9271d..ee839c18b89 100644 --- a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/MemoryUtil.java +++ b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/MemoryUtil.java @@ -25,6 +25,8 @@ import java.security.AccessController; import java.security.PrivilegedAction; +import org.checkerframework.checker.nullness.qual.Nullable; + import sun.misc.Unsafe; /** @@ -33,7 +35,7 @@ public class MemoryUtil { private static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(MemoryUtil.class); - private static final Constructor DIRECT_BUFFER_CONSTRUCTOR; + private static final @Nullable Constructor DIRECT_BUFFER_CONSTRUCTOR; /** * The unsafe object from which to access the off-heap memory. */ @@ -63,6 +65,7 @@ public class MemoryUtil { // try to get the unsafe object final Object maybeUnsafe = AccessController.doPrivileged(new PrivilegedAction() { @Override + @SuppressWarnings({"nullness:argument", "nullness:return"}) public Object run() { try { final Field unsafeField = Unsafe.class.getDeclaredField("theUnsafe"); @@ -179,4 +182,11 @@ public static ByteBuffer directBuffer(long address, int capacity) { throw new UnsupportedOperationException( "sun.misc.Unsafe or java.nio.DirectByteBuffer.(long, int) not available"); } + + @SuppressWarnings("nullness:argument") + public static void copyMemory(@Nullable Object srcBase, long srcOffset, + @Nullable Object destBase, long destOffset, + long bytes) { + UNSAFE.copyMemory(srcBase, srcOffset, destBase, destOffset, bytes); + } } diff --git a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/hash/MurmurHasher.java b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/hash/MurmurHasher.java index ea565dfca67..3dc96d0a90c 100644 --- a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/hash/MurmurHasher.java +++ b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/hash/MurmurHasher.java @@ -19,6 +19,7 @@ import org.apache.arrow.memory.ArrowBuf; import org.apache.arrow.memory.util.MemoryUtil; +import org.checkerframework.checker.nullness.qual.Nullable; /** * Implementation of the Murmur hashing algorithm. @@ -157,7 +158,7 @@ private static int rotateLeft(int value, int count) { } @Override - public boolean equals(Object o) { + public boolean equals(@Nullable Object o) { if (this == o) { return true; } diff --git a/java/pom.xml b/java/pom.xml index 98eea3f0c04..20a72af2022 100644 --- a/java/pom.xml +++ b/java/pom.xml @@ -814,8 +814,14 @@ 10000 -Xmaxwarns 10000 - -Awarns - -AskipDefs=.*Test + + -Awarns + + -AskipDefs=.*Test|\ + ^org.apache.arrow.memory.(LowCostIdentityHashMap.*)|\ + + + -AsuppressWarnings=nullness:nullness.on.primitive,nullness:condition.nullable,nullness:dereference.of.nullable,nullness:locking.nullable @@ -882,7 +888,9 @@ -Xmaxwarns 10000 -Awarns - -AskipDefs=.*Test + -AskipDefs=.*Test|\ + ^org.apache.arrow.memory.util.(MemoryUtil.*)|\ + -XDcompilePolicy=simple -Xplugin:ErrorProne -XepExcludedPaths:.*/(target/generated-sources)/.* -J--add-exports=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED diff --git a/java/vector/src/main/java/org/apache/arrow/vector/dictionary/DictionaryEncoder.java b/java/vector/src/main/java/org/apache/arrow/vector/dictionary/DictionaryEncoder.java index c44d106f536..ee619cf617a 100644 --- a/java/vector/src/main/java/org/apache/arrow/vector/dictionary/DictionaryEncoder.java +++ b/java/vector/src/main/java/org/apache/arrow/vector/dictionary/DictionaryEncoder.java @@ -112,13 +112,14 @@ public static ValueVector decode(ValueVector indices, Dictionary dictionary, Buf * @param valueCount dictionary vector valueCount. * @return index type. */ + @SuppressWarnings("ComparisonOutOfRange") public static ArrowType.Int getIndexType(int valueCount) { Preconditions.checkArgument(valueCount >= 0); if (valueCount <= Byte.MAX_VALUE) { return new ArrowType.Int(8, true); } else if (valueCount <= Character.MAX_VALUE) { return new ArrowType.Int(16, true); - } else if (valueCount <= Integer.MAX_VALUE) { + } else if (valueCount <= Integer.MAX_VALUE) { //this comparison to Integer.MAX_VALUE will always evaluate to true return new ArrowType.Int(32, true); } else { return new ArrowType.Int(64, true); From 2d5f8644248b132d4387a626575404a03924cfeb Mon Sep 17 00:00:00 2001 From: David Susanibar Arce Date: Wed, 27 Sep 2023 09:50:46 -0500 Subject: [PATCH 07/18] fix: error scope --- java/flight/flight-sql-jdbc-driver/pom.xml | 10 -- java/memory/memory-core/pom.xml | 48 ++++++ .../apache/arrow/memory/BaseAllocator.java | 14 +- java/performance/pom.xml | 10 -- java/pom.xml | 159 +++++------------- 5 files changed, 96 insertions(+), 145 deletions(-) diff --git a/java/flight/flight-sql-jdbc-driver/pom.xml b/java/flight/flight-sql-jdbc-driver/pom.xml index 34830697a90..1fd9222be37 100644 --- a/java/flight/flight-sql-jdbc-driver/pom.xml +++ b/java/flight/flight-sql-jdbc-driver/pom.xml @@ -240,16 +240,6 @@ - - - org.apache.maven.plugins - maven-dependency-plugin - - - org.checkerframework:checker-qual - - - diff --git a/java/memory/memory-core/pom.xml b/java/memory/memory-core/pom.xml index 62d74bd0925..38c0c582be7 100644 --- a/java/memory/memory-core/pom.xml +++ b/java/memory/memory-core/pom.xml @@ -35,6 +35,11 @@ org.immutables value + + org.checkerframework + checker-qual + ${checker.framework.version} + @@ -90,5 +95,48 @@ + + + checkerframework-jdk11+ + + [11,] + + + + + org.apache.maven.plugins + maven-compiler-plugin + + 8 + 8 + UTF-8 + + -Xmaxerrs + 10000 + -Xmaxwarns + 10000 + + -AskipDefs=.*Test|\ + ^org.apache.arrow.memory.util.(MemoryUtil.*)|\ + + + + + org.checkerframework + checker + ${checker.framework.version} + + + + + org.immutables.value.internal.$processor$.$Processor + + org.checkerframework.checker.nullness.NullnessChecker + + + + + + diff --git a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/BaseAllocator.java b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/BaseAllocator.java index 7a8d032d15c..f43d88e4ccd 100644 --- a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/BaseAllocator.java +++ b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/BaseAllocator.java @@ -43,7 +43,7 @@ abstract class BaseAllocator extends Accountant implements BufferAllocator { public static final String DEBUG_ALLOCATOR = "arrow.memory.debug.allocator"; public static final int DEBUG_LOG_LENGTH = 6; - public static final @Nullable boolean DEBUG; + public static final boolean DEBUG; private static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(BaseAllocator.class); // Initialize this before DEFAULT_CONFIG as DEFAULT_CONFIG will eventually initialize the allocation manager, @@ -66,13 +66,13 @@ abstract class BaseAllocator extends Accountant implements BufferAllocator { private final String name; private final RootAllocator root; - private final @Nullable Object DEBUG_LOCK = DEBUG ? new Object() : null; + private final Object DEBUG_LOCK = new Object(); private final AllocationListener listener; private final BaseAllocator parentAllocator; private final Map childAllocators; private final ArrowBuf empty; // members used purely for debugging - private final @Nullable IdentityHashMap childLedgers; + private final @Nullable IdentityHashMap childLedgers; private final @Nullable IdentityHashMap reservations; private final @Nullable HistoricalLog historicalLog; private final RoundingPolicy roundingPolicy; @@ -157,7 +157,7 @@ private static String createErrorMsg(final BufferAllocator allocator, final long } } - public static @Nullable boolean isDebug() { + public static boolean isDebug() { return DEBUG; } @@ -186,12 +186,14 @@ public ArrowBuf getEmpty() { * we have a new ledger * associated with this allocator. */ - @SuppressWarnings({"nullness:locking.nullable", "nullness:argument"}) + // @SuppressWarnings({"nullness:locking.nullable", "nullness:argument"}) void associateLedger(BufferLedger ledger) { assertOpen(); if (DEBUG) { synchronized (DEBUG_LOCK) { - childLedgers.put(ledger, null); + if (childLedgers != null) { + childLedgers.put(ledger, null); + } } } } diff --git a/java/performance/pom.xml b/java/performance/pom.xml index 753c0e66964..8653afca219 100644 --- a/java/performance/pom.xml +++ b/java/performance/pom.xml @@ -175,16 +175,6 @@ - - - org.apache.maven.plugins - maven-dependency-plugin - - - org.checkerframework:checker-qual - - - diff --git a/java/pom.xml b/java/pom.xml index 20a72af2022..616ffdb20c2 100644 --- a/java/pom.xml +++ b/java/pom.xml @@ -44,11 +44,9 @@ 2 true 9+181-r4173-1 - 2.10.0 - 2.21.1 - 2.9.3 - 3.10.1 - 3.38.0 + 2.16 + 3.10.1 + 3.38.0 @@ -379,7 +377,7 @@ org.apache.maven.plugins maven-dependency-plugin - 3.1.2 + 3.0.1 org.apache.rat @@ -394,7 +392,7 @@ org.apache.maven.plugins maven-compiler-plugin - ${maven.compiler.plugin.version} + ${maven-compiler-plugin.version} @@ -571,7 +569,7 @@ org.immutables value - ${immutables.version} + 2.8.2 provided @@ -618,11 +616,6 @@ - - org.checkerframework - checker-qual - ${checker.version} - org.slf4j @@ -694,6 +687,7 @@ 0.9.44 test + @@ -774,99 +768,45 @@ - error-prone-checkerframework-jdk8 - - 1.8 - - - - - maven-compiler-plugin - - true - - - org.checkerframework - checker - ${checker.version} - - - com.google.errorprone - error_prone_core - ${pinjdk8.errorprone.core.version} - - - org.immutables - value - ${immutables.version} - - - - - org.immutables.value.internal.$processor$.$Processor - - org.checkerframework.checker.nullness.NullnessChecker - - - -XDcompilePolicy=simple - -Xplugin:ErrorProne - -Xmaxerrs - 10000 - -Xmaxwarns - 10000 - - -Awarns - - -AskipDefs=.*Test|\ - ^org.apache.arrow.memory.(LowCostIdentityHashMap.*)|\ - - - -AsuppressWarnings=nullness:nullness.on.primitive,nullness:condition.nullable,nullness:dereference.of.nullable,nullness:locking.nullable - - - - - - - - - checkerframework-jdk8 + error-prone-jdk8 + - 1.8 + 1.8 + + !m2e.version + - - - org.apache.maven.plugins - maven-dependency-plugin - - - - copy - - process-sources - - com.google.errorprone:javac:${errorprone.javac.version}:jar - ${project.build.directory}/javac - - - - - - org.apache.maven.plugins - maven-compiler-plugin - - - -J-Xbootclasspath/p:${project.build.directory}/javac/javac-${errorprone.javac.version}.jar - - - - + + + org.apache.maven.plugins + maven-compiler-plugin + + true + + -XDcompilePolicy=simple + -Xplugin:ErrorProne + -J-Xbootclasspath/p:${settings.localRepository}/com/google/errorprone/javac/${errorprone.javac.version}/javac-${errorprone.javac.version}.jar + + + + com.google.errorprone + error_prone_core + 2.4.0 + + + + + - error-prone-checkerframework-jdk11+ + error-prone-jdk11+ [11,] @@ -883,14 +823,6 @@ 8 UTF-8 - -Xmaxerrs - 10000 - -Xmaxwarns - 10000 - -Awarns - -AskipDefs=.*Test|\ - ^org.apache.arrow.memory.util.(MemoryUtil.*)|\ - -XDcompilePolicy=simple -Xplugin:ErrorProne -XepExcludedPaths:.*/(target/generated-sources)/.* -J--add-exports=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED @@ -908,20 +840,9 @@ com.google.errorprone error_prone_core - ${errorprone.core.version} - - - org.checkerframework - checker - ${checker.version} + ${error_prone_core.version} - - - org.immutables.value.internal.$processor$.$Processor - - org.checkerframework.checker.nullness.NullnessChecker - From 919b3013059b55342daddc7fab93083012cb65ce Mon Sep 17 00:00:00 2001 From: David Susanibar Arce Date: Mon, 2 Oct 2023 09:28:56 -0500 Subject: [PATCH 08/18] fix: add nullability checker framework --- java/memory/memory-core/pom.xml | 5 ++-- .../org/apache/arrow/memory/Accountant.java | 3 ++- .../arrow/memory/AllocationManager.java | 8 ++++-- .../arrow/memory/AllocationOutcome.java | 7 +++-- .../memory/AllocationOutcomeDetails.java | 1 + .../org/apache/arrow/memory/ArrowBuf.java | 11 +++++--- .../apache/arrow/memory/BaseAllocator.java | 26 ++++++++++--------- .../apache/arrow/memory/BufferAllocator.java | 3 ++- .../org/apache/arrow/memory/BufferLedger.java | 20 +++++++++++--- .../DefaultAllocationManagerOption.java | 10 ++++++- .../arrow/memory/LowCostIdentityHashMap.java | 19 +++++++++----- .../arrow/memory/util/ArrowBufPointer.java | 13 ++++++---- .../arrow/memory/util/HistoricalLog.java | 13 +++++++--- .../apache/arrow/memory/util/MemoryUtil.java | 5 +++- .../apache/arrow/memory/util/StackTrace.java | 1 + .../arrow/memory/util/hash/SimpleHasher.java | 3 ++- java/pom.xml | 6 +++++ .../vector/dictionary/DictionaryEncoder.java | 3 +-- 18 files changed, 108 insertions(+), 49 deletions(-) diff --git a/java/memory/memory-core/pom.xml b/java/memory/memory-core/pom.xml index 38c0c582be7..f0b08a24905 100644 --- a/java/memory/memory-core/pom.xml +++ b/java/memory/memory-core/pom.xml @@ -38,7 +38,6 @@ org.checkerframework checker-qual - ${checker.framework.version} @@ -111,14 +110,14 @@ 8 UTF-8 - -Xmaxerrs + -Xmaxerrs 10000 -Xmaxwarns 10000 -AskipDefs=.*Test|\ - ^org.apache.arrow.memory.util.(MemoryUtil.*)|\ + -AatfDoNotCache diff --git a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/Accountant.java b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/Accountant.java index 492ce06be50..ea631f711d8 100644 --- a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/Accountant.java +++ b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/Accountant.java @@ -60,6 +60,7 @@ class Accountant implements AutoCloseable { */ private final AtomicLong locallyHeldMemory = new AtomicLong(); + @SuppressWarnings("nullness:dereference.of.nullable") //dereference of possibly-null reference parent public Accountant(@Nullable Accountant parent, String name, long reservation, long maxAllocation) { Preconditions.checkNotNull(name, "name must not be null"); Preconditions.checkArgument(reservation >= 0, "The initial reservation size must be non-negative."); @@ -287,7 +288,7 @@ public void setLimit(long newLimit) { * * @return Currently allocate memory in bytes. */ - public long getAllocatedMemory() { + public long getAllocatedMemory(Accountant this) { return locallyHeldMemory.get(); } diff --git a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/AllocationManager.java b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/AllocationManager.java index 3071c02f30a..57da5f5fd47 100644 --- a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/AllocationManager.java +++ b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/AllocationManager.java @@ -18,6 +18,7 @@ package org.apache.arrow.memory; import org.apache.arrow.util.Preconditions; +import org.checkerframework.checker.nullness.qual.Nullable; /** * An AllocationManager is the implementation of a physical memory allocation. @@ -48,8 +49,9 @@ public abstract class AllocationManager { // This is mostly a semantic constraint on the API user: if the reference count reaches 0 in the owningLedger, then // there are not supposed to be any references through other allocators. In practice, this doesn't do anything // as the implementation just forces ownership to be transferred to one of the other extant references. - private volatile BufferLedger owningLedger; + private volatile @Nullable BufferLedger owningLedger; + @SuppressWarnings("nullness:method.invocation") //call to associate(a, b) not allowed on the given receiver protected AllocationManager(BufferAllocator accountingAllocator) { Preconditions.checkNotNull(accountingAllocator); accountingAllocator.assertOpen(); @@ -61,7 +63,7 @@ protected AllocationManager(BufferAllocator accountingAllocator) { this.owningLedger = associate(accountingAllocator, false); } - BufferLedger getOwningLedger() { + @Nullable BufferLedger getOwningLedger() { return owningLedger; } @@ -126,6 +128,8 @@ private BufferLedger associate(final BufferAllocator allocator, final boolean re * It is called when the shared refcount of all the ArrowBufs managed by the * calling ReferenceManager drops to 0. */ + + @SuppressWarnings("nullness:dereference.of.nullable") //dereference of possibly-null reference oldLedger void release(final BufferLedger ledger) { final BufferAllocator allocator = ledger.getAllocator(); allocator.assertOpen(); diff --git a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/AllocationOutcome.java b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/AllocationOutcome.java index 2977775e6ce..21a57eee49b 100644 --- a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/AllocationOutcome.java +++ b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/AllocationOutcome.java @@ -19,15 +19,18 @@ import java.util.Optional; +import org.checkerframework.checker.nullness.qual.Nullable; + + /** * Describes the type of outcome that occurred when trying to account for allocation of memory. */ public class AllocationOutcome { private final Status status; - private final AllocationOutcomeDetails details; + private final @Nullable AllocationOutcomeDetails details; static final AllocationOutcome SUCCESS_INSTANCE = new AllocationOutcome(Status.SUCCESS); - AllocationOutcome(Status status, AllocationOutcomeDetails details) { + AllocationOutcome(Status status, @Nullable AllocationOutcomeDetails details) { this.status = status; this.details = details; } diff --git a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/AllocationOutcomeDetails.java b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/AllocationOutcomeDetails.java index be0258995ab..3ceda71cce0 100644 --- a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/AllocationOutcomeDetails.java +++ b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/AllocationOutcomeDetails.java @@ -22,6 +22,7 @@ import org.checkerframework.checker.nullness.qual.Nullable; + /** * Captures details of allocation for each accountant in the hierarchical chain. */ diff --git a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/ArrowBuf.java b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/ArrowBuf.java index cf441bae178..d28f80b9cfb 100644 --- a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/ArrowBuf.java +++ b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/ArrowBuf.java @@ -83,6 +83,7 @@ public final class ArrowBuf implements AutoCloseable { * @param referenceManager The memory manager to track memory usage and reference count of this buffer * @param capacity The capacity in bytes of this buffer */ + @SuppressWarnings("nullness:dereference.of.nullable") //dereference of possibly-null reference historicalLog public ArrowBuf( final ReferenceManager referenceManager, final @Nullable BufferManager bufferManager, @@ -245,7 +246,7 @@ public int hashCode() { } @Override - public boolean equals(Object obj) { + public boolean equals(@Nullable Object obj) { // identity equals only. return this == obj; } @@ -315,7 +316,9 @@ private void checkIndexD(long index, long fieldLength) { Preconditions.checkArgument(fieldLength >= 0, "expecting non-negative data length"); if (index < 0 || index > capacity() - fieldLength) { if (BaseAllocator.DEBUG) { - historicalLog.logHistory(logger); + if (historicalLog != null) { + historicalLog.logHistory(logger); + } } throw new IndexOutOfBoundsException(String.format( "index: %d, length: %d (expected: range(0, %d))", index, fieldLength, capacity())); @@ -1112,7 +1115,9 @@ public void print(StringBuilder sb, int indent, Verbosity verbosity) { if (BaseAllocator.DEBUG && verbosity.includeHistoricalLog) { sb.append("\n"); - historicalLog.buildHistory(sb, indent + 1, verbosity.includeStackTraces); + if (historicalLog != null) { + historicalLog.buildHistory(sb, indent + 1, verbosity.includeStackTraces); + } } } diff --git a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/BaseAllocator.java b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/BaseAllocator.java index f43d88e4ccd..e8d7bee4af0 100644 --- a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/BaseAllocator.java +++ b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/BaseAllocator.java @@ -30,6 +30,8 @@ import org.apache.arrow.memory.util.CommonUtil; import org.apache.arrow.memory.util.HistoricalLog; import org.apache.arrow.util.Preconditions; +import org.checkerframework.checker.initialization.qual.Initialized; +import org.checkerframework.checker.initialization.qual.UnderInitialization; import org.checkerframework.checker.nullness.qual.Nullable; import org.immutables.value.Value; @@ -39,6 +41,9 @@ *

The class is abstract to enforce usage of {@linkplain RootAllocator}/{@linkplain ChildAllocator} * facades. */ +@SuppressWarnings("nullness:dereference.of.nullable") +//dereference of possibly-null reference allocationManagerFactory, historicalLog, childLedgers, reservations, +//historicalLog, childAllocator.historicalLog, reservation.historicalLog, abstract class BaseAllocator extends Accountant implements BufferAllocator { public static final String DEBUG_ALLOCATOR = "arrow.memory.debug.allocator"; @@ -65,10 +70,9 @@ abstract class BaseAllocator extends Accountant implements BufferAllocator { // Package exposed for sharing between AllocatorManger and BaseAllocator objects private final String name; private final RootAllocator root; - private final Object DEBUG_LOCK = new Object(); private final AllocationListener listener; - private final BaseAllocator parentAllocator; + private final @Nullable BaseAllocator parentAllocator; private final Map childAllocators; private final ArrowBuf empty; // members used purely for debugging @@ -89,7 +93,8 @@ abstract class BaseAllocator extends Accountant implements BufferAllocator { * * @see Config */ - @SuppressWarnings({"nullness:assignment", "nullness:method.invocation"}) + @SuppressWarnings({"nullness:method.invocation", "nullness:cast.unsafe"}) + //{"call to hist(,...) not allowed on the given receiver.", "cast cannot be statically verified"} protected BaseAllocator( final @Nullable BaseAllocator parentAllocator, final String name, @@ -103,7 +108,7 @@ protected BaseAllocator( this.root = parentAllocator.root; empty = parentAllocator.empty; } else if (this instanceof RootAllocator) { - this.root = (RootAllocator) this; + this.root = (@Initialized RootAllocator) this; empty = createEmpty(); } else { throw new IllegalStateException("An parent allocator must either carry a root or be the " + @@ -134,7 +139,7 @@ public AllocationListener getListener() { } @Override - public BaseAllocator getParentAllocator() { + public @Nullable BaseAllocator getParentAllocator() { return parentAllocator; } @@ -186,14 +191,11 @@ public ArrowBuf getEmpty() { * we have a new ledger * associated with this allocator. */ - // @SuppressWarnings({"nullness:locking.nullable", "nullness:argument"}) void associateLedger(BufferLedger ledger) { assertOpen(); if (DEBUG) { synchronized (DEBUG_LOCK) { - if (childLedgers != null) { - childLedgers.put(ledger, null); - } + childLedgers.put(ledger, null); } } } @@ -286,7 +288,7 @@ public ArrowBuf buffer(final long initialRequestSize) { return buffer(initialRequestSize, null); } - private ArrowBuf createEmpty() { + private ArrowBuf createEmpty(@UnderInitialization BaseAllocator this) { return allocationManagerFactory.empty(); } @@ -819,7 +821,7 @@ RoundingPolicy getRoundingPolicy() { */ public class Reservation implements AllocationReservation { - private final HistoricalLog historicalLog; + private final @Nullable HistoricalLog historicalLog; private int nBytes = 0; private boolean used = false; private boolean closed = false; @@ -830,7 +832,7 @@ public class Reservation implements AllocationReservation { *

If {@linkplain #DEBUG} is true this will capture a historical * log of events relevant to this Reservation. */ - @SuppressWarnings({"nullness:argument"}) + @SuppressWarnings("nullness:argument") //incompatible argument for parameter arg0 of System.identityHashCode. public Reservation() { if (DEBUG) { historicalLog = new HistoricalLog("Reservation[allocator[%s], %d]", name, System diff --git a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/BufferAllocator.java b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/BufferAllocator.java index 90a4ef26fb4..c279e18f1ea 100644 --- a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/BufferAllocator.java +++ b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/BufferAllocator.java @@ -21,6 +21,7 @@ import org.apache.arrow.memory.rounding.DefaultRoundingPolicy; import org.apache.arrow.memory.rounding.RoundingPolicy; +import org.checkerframework.checker.nullness.qual.Nullable; /** * Wrapper class to deal with byte buffer allocation. Ensures users only use designated methods. @@ -166,7 +167,7 @@ BufferAllocator newChildAllocator( * * @return parent allocator */ - BufferAllocator getParentAllocator(); + @Nullable BufferAllocator getParentAllocator(); /** * Returns the set of child allocators. diff --git a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/BufferLedger.java b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/BufferLedger.java index 48b3e183d5a..a38aa107c43 100644 --- a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/BufferLedger.java +++ b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/BufferLedger.java @@ -24,6 +24,7 @@ import org.apache.arrow.memory.util.CommonUtil; import org.apache.arrow.memory.util.HistoricalLog; import org.apache.arrow.util.Preconditions; +import org.checkerframework.checker.nullness.qual.Nullable; /** * The reference manager that binds an {@link AllocationManager} to @@ -32,7 +33,7 @@ * fate (same reference count). */ public class BufferLedger implements ValueWithKeyIncluded, ReferenceManager { - private final IdentityHashMap buffers = + private final @Nullable IdentityHashMap buffers = BaseAllocator.DEBUG ? new IdentityHashMap<>() : null; private static final AtomicLong LEDGER_ID_GENERATOR = new AtomicLong(0); // unique ID assigned to each ledger @@ -43,7 +44,7 @@ public class BufferLedger implements ValueWithKeyIncluded, Refe private final long lCreationTime = System.nanoTime(); private final BufferAllocator allocator; private final AllocationManager allocationManager; - private final HistoricalLog historicalLog = + private final @Nullable HistoricalLog historicalLog = BaseAllocator.DEBUG ? new HistoricalLog(BaseAllocator.DEBUG_LOG_LENGTH, "BufferLedger[%d]", 1) : null; private volatile long lDestructionTime = 0; @@ -117,6 +118,7 @@ public boolean release() { * @return true if the new ref count has dropped to 0, false otherwise */ @Override + @SuppressWarnings("nullness:dereference.of.nullable") //dereference of possibly-null reference historicalLog public boolean release(int decrement) { Preconditions.checkState(decrement >= 1, "ref count decrement should be greater than or equal to 1"); @@ -176,6 +178,7 @@ public void retain() { * @param increment amount to increase the reference count by */ @Override + @SuppressWarnings("nullness:dereference.of.nullable") //dereference of possibly-null reference historicalLog public void retain(int increment) { Preconditions.checkArgument(increment > 0, "retain(%s) argument is not positive", increment); if (BaseAllocator.DEBUG) { @@ -204,6 +207,8 @@ public void retain(int increment) { * @return derived buffer */ @Override + @SuppressWarnings({"nullness:dereference.of.nullable", "nullness:locking.nullable"}) + //{"dereference of possibly-null reference historicalLog", "synchronizing over a possibly-null lock (buffers)"} public ArrowBuf deriveBuffer(final ArrowBuf sourceBuffer, long index, long length) { /* * Usage type 1 for deriveBuffer(): @@ -261,7 +266,9 @@ public ArrowBuf deriveBuffer(final ArrowBuf sourceBuffer, long index, long lengt * @return A new ArrowBuf that shares references with all ArrowBufs associated * with this BufferLedger */ - ArrowBuf newArrowBuf(final long length, final BufferManager manager) { + @SuppressWarnings({"nullness:dereference.of.nullable", "nullness:locking.nullable"}) + //{"dereference of possibly-null reference historicalLog,buffers","synchronizing over a possibly-null lock (buffers)"} + ArrowBuf newArrowBuf(final long length, final @Nullable BufferManager manager) { allocator.assertOpen(); // the start virtual address of the ArrowBuf will be same as address of memory chunk @@ -304,6 +311,7 @@ ArrowBuf newArrowBuf(final long length, final BufferManager manager) { * @return A new ArrowBuf which shares the same underlying memory as the provided ArrowBuf. */ @Override + @SuppressWarnings("nullness:dereference.of.nullable") //dereference of possibly-null reference historicalLog public ArrowBuf retain(final ArrowBuf srcBuffer, BufferAllocator target) { if (BaseAllocator.DEBUG) { @@ -333,7 +341,9 @@ public ArrowBuf retain(final ArrowBuf srcBuffer, BufferAllocator target) { * @param targetReferenceManager The ledger to transfer ownership account to. * @return Whether transfer fit within target ledgers limits. */ - boolean transferBalance(final ReferenceManager targetReferenceManager) { + @SuppressWarnings("nullness:dereference.of.nullable") + //dereference of possibly-null reference historicalLog, targetReferenceManager + boolean transferBalance(final @Nullable ReferenceManager targetReferenceManager) { Preconditions.checkArgument(targetReferenceManager != null, "Expecting valid target reference manager"); final BufferAllocator targetAllocator = targetReferenceManager.getAllocator(); @@ -481,6 +491,8 @@ public long getAccountedSize() { * @param indent The level of indentation to position the data. * @param verbosity The level of verbosity to print. */ + @SuppressWarnings({"nullness:dereference.of.nullable", "nullness:locking.nullable"}) + //{"dereference of possibly-null reference buffers", "synchronizing over a possibly-null lock (buffers)"} void print(StringBuilder sb, int indent, BaseAllocator.Verbosity verbosity) { CommonUtil.indent(sb, indent) .append("ledger[") diff --git a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/DefaultAllocationManagerOption.java b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/DefaultAllocationManagerOption.java index 15120c252fc..81bc06813ba 100644 --- a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/DefaultAllocationManagerOption.java +++ b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/DefaultAllocationManagerOption.java @@ -19,6 +19,9 @@ import java.lang.reflect.Field; +import org.checkerframework.checker.nullness.qual.NonNull; + + /** * A class for choosing the default allocation manager. */ @@ -39,6 +42,7 @@ public class DefaultAllocationManagerOption { /** * The default allocation manager factory. */ + @SuppressWarnings("nullness:assignment") //static member qualifying type may not be annotated private static AllocationManager.Factory DEFAULT_ALLOCATION_MANAGER_FACTORY = null; /** @@ -61,7 +65,8 @@ public enum AllocationManagerType { Unknown, } - static AllocationManagerType getDefaultAllocationManagerType() { + @SuppressWarnings("nullness:argument") //enum types are implicitly non-null + static @NonNull AllocationManagerType getDefaultAllocationManagerType() { AllocationManagerType ret = AllocationManagerType.Unknown; try { @@ -103,6 +108,9 @@ static AllocationManager.Factory getDefaultAllocationManagerFactory() { return DEFAULT_ALLOCATION_MANAGER_FACTORY; } + @SuppressWarnings({"nullness:argument", "nullness:return"}) + //incompatible argument for parameter obj of Field.get + // Static member qualifying type may not be annotated private static AllocationManager.Factory getFactory(String clazzName) { try { Field field = Class.forName(clazzName).getDeclaredField("FACTORY"); diff --git a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/LowCostIdentityHashMap.java b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/LowCostIdentityHashMap.java index edfa82392a4..de5bf9f42d2 100644 --- a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/LowCostIdentityHashMap.java +++ b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/LowCostIdentityHashMap.java @@ -19,6 +19,7 @@ import org.apache.arrow.util.Preconditions; import org.apache.arrow.util.VisibleForTesting; +import org.checkerframework.checker.nullness.qual.Nullable; /** * Highly specialized IdentityHashMap that implements only partial @@ -35,7 +36,7 @@ public class LowCostIdentityHashMap> { /* * The internal data structure to hold values. */ - private Object[] elementData; + private @Nullable Object [] elementData; // elementData[index] = null; /* Actual number of values. */ private int size; @@ -65,6 +66,7 @@ public LowCostIdentityHashMap() { * The estimated maximum number of entries that will be put in * this map. */ + @SuppressWarnings("nullness:method.invocation") //call to getThreshold, newElementArray not allowed the given receiver public LowCostIdentityHashMap(int maxSize) { if (maxSize >= 0) { this.size = 0; @@ -152,7 +154,8 @@ public boolean containsValue(V value) { * @param key the key. * @return the value of the mapping with the specified key. */ - public V get(K key) { + @SuppressWarnings("nullness:cast.unsafe") //cast cannot be statically verified + public @Nullable V get(K key) { Preconditions.checkNotNull(key); int index = findIndex(key, elementData); @@ -166,7 +169,7 @@ public V get(K key) { * empty spot if the key is not found in this table. */ @VisibleForTesting - int findIndex(Object key, Object[] array) { + int findIndex(@Nullable Object key, @Nullable Object[] array) { int length = array.length; int index = getModuloHash(key, length); int last = (index + length - 1) % length; @@ -184,7 +187,7 @@ int findIndex(Object key, Object[] array) { } @VisibleForTesting - static int getModuloHash(Object key, int length) { + static int getModuloHash(@Nullable Object key, int length) { return ((System.identityHashCode(key) & 0x7FFFFFFF) % length); } @@ -195,6 +198,7 @@ static int getModuloHash(Object key, int length) { * @return the value of any previous mapping with the specified key or * {@code null} if there was no such mapping. */ + @SuppressWarnings("nullness:cast.unsafe") //cast cannot be statically verified public V put(V value) { Preconditions.checkNotNull(value); K key = value.getKey(); @@ -226,7 +230,7 @@ void rehash() { if (newlength == 0) { newlength = 1; } - Object[] newData = newElementArray(newlength); + @Nullable Object[] newData = newElementArray(newlength); for (int i = 0; i < elementData.length; i++) { Object key = (elementData[i] == null) ? null : ((V) elementData[i]).getKey(); if (key != null) { @@ -250,7 +254,8 @@ private void computeMaxSize() { * @return the value of the removed mapping, or {@code null} if no mapping * for the specified key was found. */ - public V remove(K key) { + @SuppressWarnings("nullness:cast.unsafe") //cast cannot be statically verified + public @Nullable V remove(K key) { Preconditions.checkNotNull(key); boolean hashedOk; @@ -325,7 +330,7 @@ public int size() { * * @return next available value or null if none available */ - public V getNextValue() { + public @Nullable V getNextValue() { for (int i = 0; i < elementData.length; i++) { if (elementData[i] != null) { return (V) elementData[i]; diff --git a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/ArrowBufPointer.java b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/ArrowBufPointer.java index d16b2c6d0f2..f843b45c487 100644 --- a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/ArrowBufPointer.java +++ b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/ArrowBufPointer.java @@ -34,7 +34,7 @@ public final class ArrowBufPointer { */ public static final int NULL_HASH_CODE = 0; - private @Nullable ArrowBuf buf; + private ArrowBuf buf; private long offset; @@ -60,6 +60,7 @@ public ArrowBufPointer() { * Constructs an arrow buffer pointer with the specified hasher. * @param hasher the hasher to use. */ + @SuppressWarnings("nullness:initialization.fields.uninitialized") //the constructor does not initialize fields: buf public ArrowBufPointer(ArrowBufHasher hasher) { Preconditions.checkNotNull(hasher); this.hasher = hasher; @@ -82,8 +83,9 @@ public ArrowBufPointer(ArrowBuf buf, long offset, long length) { * @param length the length off set of the memory region pointed to. * @param hasher the hasher used to calculate the hash code. */ - @SuppressWarnings("initialization.fields.uninitialized") - public ArrowBufPointer(@Nullable ArrowBuf buf, long offset, long length, ArrowBufHasher hasher) { + @SuppressWarnings({"nullness:initialization.fields.uninitialized", "nullness:method.invocation"}) + // {"the constructor does not initialize fields: buf", "call to set not allowed on the given receiver."} + public ArrowBufPointer(ArrowBuf buf, long offset, long length, ArrowBufHasher hasher) { Preconditions.checkNotNull(hasher); this.hasher = hasher; set(buf, offset, length); @@ -95,7 +97,8 @@ public ArrowBufPointer(@Nullable ArrowBuf buf, long offset, long length, ArrowBu * @param offset the start off set of the memory region pointed to. * @param length the length off set of the memory region pointed to. */ - public void set(@Nullable ArrowBuf buf, long offset, long length) { + + public void set(ArrowBuf buf, long offset, long length) { this.buf = buf; this.offset = offset; this.length = length; @@ -107,7 +110,7 @@ public void set(@Nullable ArrowBuf buf, long offset, long length) { * Gets the underlying buffer, or null if the underlying data is invalid or null. * @return the underlying buffer, if any, or null if the underlying data is invalid or null. */ - public @Nullable ArrowBuf getBuf() { + public ArrowBuf getBuf() { return buf; } diff --git a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/HistoricalLog.java b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/HistoricalLog.java index f02539a8a3d..21f063c939e 100644 --- a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/HistoricalLog.java +++ b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/HistoricalLog.java @@ -20,6 +20,7 @@ import java.util.Arrays; import java.util.LinkedList; +import org.checkerframework.checker.nullness.qual.Nullable; import org.slf4j.Logger; /** @@ -32,7 +33,7 @@ public class HistoricalLog { private final LinkedList history = new LinkedList<>(); private final String idString; // the formatted id string private final int limit; // the limit on the number of events kept - private Event firstEvent; // the first stack trace recorded + private @Nullable Event firstEvent; // the first stack trace recorded /** * Constructor. The format string will be formatted and have its arguments @@ -68,6 +69,7 @@ public HistoricalLog(final String idStringFormat, Object... args) { public HistoricalLog(final int limit, final String idStringFormat, Object... args) { this.limit = limit; this.idString = String.format(idStringFormat, args); + this.firstEvent = null; } /** @@ -122,13 +124,16 @@ public synchronized void buildHistory( .append('\n'); if (firstEvent != null) { + long time = firstEvent.time; + String note = firstEvent.note; + final StackTrace stackTrace = firstEvent.stackTrace; sb.append(innerIndentation) - .append(firstEvent.time) + .append(time) .append(' ') - .append(firstEvent.note) + .append(note) .append('\n'); if (includeStackTrace) { - firstEvent.stackTrace.writeToBuilder(sb, indent + 2); + stackTrace.writeToBuilder(sb, indent + 2); } for (final Event event : history) { diff --git a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/MemoryUtil.java b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/MemoryUtil.java index ee839c18b89..6f619610dfd 100644 --- a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/MemoryUtil.java +++ b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/MemoryUtil.java @@ -29,6 +29,7 @@ import sun.misc.Unsafe; + /** * Utilities for memory related operations. */ @@ -66,6 +67,8 @@ public class MemoryUtil { final Object maybeUnsafe = AccessController.doPrivileged(new PrivilegedAction() { @Override @SuppressWarnings({"nullness:argument", "nullness:return"}) + // incompatible argument for parameter obj of Field.get + // incompatible types in return public Object run() { try { final Field unsafeField = Unsafe.class.getDeclaredField("theUnsafe"); @@ -183,7 +186,7 @@ public static ByteBuffer directBuffer(long address, int capacity) { "sun.misc.Unsafe or java.nio.DirectByteBuffer.(long, int) not available"); } - @SuppressWarnings("nullness:argument") + @SuppressWarnings("nullness:argument") //to handle null assignment on third party dependency public static void copyMemory(@Nullable Object srcBase, long srcOffset, @Nullable Object destBase, long destOffset, long bytes) { diff --git a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/StackTrace.java b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/StackTrace.java index cd864eb9985..c6d7d7e0494 100644 --- a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/StackTrace.java +++ b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/StackTrace.java @@ -29,6 +29,7 @@ public class StackTrace { /** * Constructor. Captures the current stack trace. */ + @SuppressWarnings("nullness:assignment") //incompatible types in assignment public StackTrace() { final StackTraceElement[] stack = Thread.currentThread().getStackTrace(); // Skip first two elements to remove getStackTrace/StackTrace. diff --git a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/hash/SimpleHasher.java b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/hash/SimpleHasher.java index da0ee482997..3bf3c2a8283 100644 --- a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/hash/SimpleHasher.java +++ b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/hash/SimpleHasher.java @@ -20,6 +20,7 @@ import org.apache.arrow.memory.ArrowBuf; import org.apache.arrow.memory.util.MemoryUtil; +import org.checkerframework.checker.nullness.qual.Nullable; /** * A simple hasher that calculates the hash code of integers as is, @@ -110,7 +111,7 @@ public int hashCode() { } @Override - public boolean equals(Object obj) { + public boolean equals(@Nullable Object obj) { return obj != null && (obj instanceof SimpleHasher); } } diff --git a/java/pom.xml b/java/pom.xml index 616ffdb20c2..066c1e5d4cd 100644 --- a/java/pom.xml +++ b/java/pom.xml @@ -352,6 +352,7 @@ javax.annotation:javax.annotation-api:* org.apache.hadoop:hadoop-client-api + org.checkerframework:checker-qual @@ -540,6 +541,11 @@ + + org.checkerframework + checker-qual + ${checker.framework.version} + com.google.flatbuffers flatbuffers-java diff --git a/java/vector/src/main/java/org/apache/arrow/vector/dictionary/DictionaryEncoder.java b/java/vector/src/main/java/org/apache/arrow/vector/dictionary/DictionaryEncoder.java index ee619cf617a..c44d106f536 100644 --- a/java/vector/src/main/java/org/apache/arrow/vector/dictionary/DictionaryEncoder.java +++ b/java/vector/src/main/java/org/apache/arrow/vector/dictionary/DictionaryEncoder.java @@ -112,14 +112,13 @@ public static ValueVector decode(ValueVector indices, Dictionary dictionary, Buf * @param valueCount dictionary vector valueCount. * @return index type. */ - @SuppressWarnings("ComparisonOutOfRange") public static ArrowType.Int getIndexType(int valueCount) { Preconditions.checkArgument(valueCount >= 0); if (valueCount <= Byte.MAX_VALUE) { return new ArrowType.Int(8, true); } else if (valueCount <= Character.MAX_VALUE) { return new ArrowType.Int(16, true); - } else if (valueCount <= Integer.MAX_VALUE) { //this comparison to Integer.MAX_VALUE will always evaluate to true + } else if (valueCount <= Integer.MAX_VALUE) { return new ArrowType.Int(32, true); } else { return new ArrowType.Int(64, true); From 0230b91221481bc115550b61eef92e50df75ac17 Mon Sep 17 00:00:00 2001 From: David Susanibar Arce Date: Mon, 2 Oct 2023 09:34:51 -0500 Subject: [PATCH 09/18] fix: required nullable mark --- .../src/main/java/org/apache/arrow/memory/Accountant.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/Accountant.java b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/Accountant.java index ea631f711d8..3d4d82feabd 100644 --- a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/Accountant.java +++ b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/Accountant.java @@ -35,7 +35,7 @@ class Accountant implements AutoCloseable { /** * The parent allocator. */ - protected final Accountant parent; + protected final @Nullable Accountant parent; private final String name; From 46dd019852529e8700cc6abe016203f24e3d498f Mon Sep 17 00:00:00 2001 From: David Susanibar Arce Date: Mon, 16 Oct 2023 17:52:10 -0500 Subject: [PATCH 10/18] fix: synch last changes to reduce suppress warnings --- .../org/apache/arrow/memory/Accountant.java | 17 +- .../arrow/memory/AllocationManager.java | 56 +++--- .../org/apache/arrow/memory/ArrowBuf.java | 5 +- .../apache/arrow/memory/BaseAllocator.java | 171 +++++++++++------- .../org/apache/arrow/memory/BufferLedger.java | 76 ++++---- .../DefaultAllocationManagerOption.java | 9 +- .../arrow/memory/LowCostIdentityHashMap.java | 28 ++- .../arrow/memory/util/ArrowBufPointer.java | 6 +- .../apache/arrow/memory/util/MemoryUtil.java | 2 +- .../apache/arrow/memory/util/StackTrace.java | 28 +-- 10 files changed, 225 insertions(+), 173 deletions(-) diff --git a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/Accountant.java b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/Accountant.java index 3d4d82feabd..3eab535d22e 100644 --- a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/Accountant.java +++ b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/Accountant.java @@ -60,7 +60,6 @@ class Accountant implements AutoCloseable { */ private final AtomicLong locallyHeldMemory = new AtomicLong(); - @SuppressWarnings("nullness:dereference.of.nullable") //dereference of possibly-null reference parent public Accountant(@Nullable Accountant parent, String name, long reservation, long maxAllocation) { Preconditions.checkNotNull(name, "name must not be null"); Preconditions.checkArgument(reservation >= 0, "The initial reservation size must be non-negative."); @@ -75,12 +74,14 @@ public Accountant(@Nullable Accountant parent, String name, long reservation, lo this.allocationLimit.set(maxAllocation); if (reservation != 0) { - // we will allocate a reservation from our parent. - final AllocationOutcome outcome = parent.allocateBytes(reservation); - if (!outcome.isOk()) { - throw new OutOfMemoryException(String.format( - "Failure trying to allocate initial reservation for Allocator. " + - "Attempted to allocate %d bytes.", reservation), outcome.getDetails()); + if (parent != null) { + // we will allocate a reservation from our parent. + final AllocationOutcome outcome = parent.allocateBytes(reservation); + if (!outcome.isOk()) { + throw new OutOfMemoryException(String.format( + "Failure trying to allocate initial reservation for Allocator. " + + "Attempted to allocate %d bytes.", reservation), outcome.getDetails()); + } } } } @@ -288,7 +289,7 @@ public void setLimit(long newLimit) { * * @return Currently allocate memory in bytes. */ - public long getAllocatedMemory(Accountant this) { + public long getAllocatedMemory() { return locallyHeldMemory.get(); } diff --git a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/AllocationManager.java b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/AllocationManager.java index 57da5f5fd47..503452e5fbe 100644 --- a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/AllocationManager.java +++ b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/AllocationManager.java @@ -128,8 +128,6 @@ private BufferLedger associate(final BufferAllocator allocator, final boolean re * It is called when the shared refcount of all the ArrowBufs managed by the * calling ReferenceManager drops to 0. */ - - @SuppressWarnings("nullness:dereference.of.nullable") //dereference of possibly-null reference oldLedger void release(final BufferLedger ledger) { final BufferAllocator allocator = ledger.getAllocator(); allocator.assertOpen(); @@ -140,33 +138,35 @@ void release(final BufferLedger ledger) { "Expecting a mapping for allocator and reference manager"); final BufferLedger oldLedger = map.remove(allocator); - BufferAllocator oldAllocator = oldLedger.getAllocator(); - if (oldAllocator instanceof BaseAllocator) { - // needed for debug only: tell the allocator that AllocationManager is removing a - // reference manager associated with this particular allocator - ((BaseAllocator) oldAllocator).dissociateLedger(oldLedger); - } - if (oldLedger == owningLedger) { - // the release call was made by the owning reference manager - if (map.isEmpty()) { - // the only mapping was for the owner - // which now has been removed, it implies we can safely destroy the - // underlying memory chunk as it is no longer being referenced - oldAllocator.releaseBytes(getSize()); - // free the memory chunk associated with the allocation manager - release0(); - oldAllocator.getListener().onRelease(getSize()); - owningLedger = null; - } else { - // since the refcount dropped to 0 for the owning reference manager and allocation - // manager will no longer keep a mapping for it, we need to change the owning - // reference manager to whatever the next available - // mapping exists. - BufferLedger newOwningLedger = map.getNextValue(); - // we'll forcefully transfer the ownership and not worry about whether we - // exceeded the limit since this consumer can't do anything with this. - oldLedger.transferBalance(newOwningLedger); + if (oldLedger != null) { + BufferAllocator oldAllocator = oldLedger.getAllocator(); + if (oldAllocator instanceof BaseAllocator) { + // needed for debug only: tell the allocator that AllocationManager is removing a + // reference manager associated with this particular allocator + ((BaseAllocator) oldAllocator).dissociateLedger(oldLedger); + } + if (oldLedger == owningLedger) { + // the release call was made by the owning reference manager + if (map.isEmpty()) { + // the only mapping was for the owner + // which now has been removed, it implies we can safely destroy the + // underlying memory chunk as it is no longer being referenced + oldAllocator.releaseBytes(getSize()); + // free the memory chunk associated with the allocation manager + release0(); + oldAllocator.getListener().onRelease(getSize()); + owningLedger = null; + } else { + // since the refcount dropped to 0 for the owning reference manager and allocation + // manager will no longer keep a mapping for it, we need to change the owning + // reference manager to whatever the next available + // mapping exists. + BufferLedger newOwningLedger = map.getNextValue(); + // we'll forcefully transfer the ownership and not worry about whether we + // exceeded the limit since this consumer can't do anything with this. + oldLedger.transferBalance(newOwningLedger); + } } } else { // the release call was made by a non-owning reference manager, so after remove there have diff --git a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/ArrowBuf.java b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/ArrowBuf.java index d28f80b9cfb..7033f718b50 100644 --- a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/ArrowBuf.java +++ b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/ArrowBuf.java @@ -83,7 +83,6 @@ public final class ArrowBuf implements AutoCloseable { * @param referenceManager The memory manager to track memory usage and reference count of this buffer * @param capacity The capacity in bytes of this buffer */ - @SuppressWarnings("nullness:dereference.of.nullable") //dereference of possibly-null reference historicalLog public ArrowBuf( final ReferenceManager referenceManager, final @Nullable BufferManager bufferManager, @@ -96,7 +95,9 @@ public ArrowBuf( this.readerIndex = 0; this.writerIndex = 0; if (BaseAllocator.DEBUG) { - historicalLog.recordEvent("create()"); + if (historicalLog != null) { + historicalLog.recordEvent("create()"); + } } } diff --git a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/BaseAllocator.java b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/BaseAllocator.java index e8d7bee4af0..e26066fc40c 100644 --- a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/BaseAllocator.java +++ b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/BaseAllocator.java @@ -31,7 +31,8 @@ import org.apache.arrow.memory.util.HistoricalLog; import org.apache.arrow.util.Preconditions; import org.checkerframework.checker.initialization.qual.Initialized; -import org.checkerframework.checker.initialization.qual.UnderInitialization; +import org.checkerframework.checker.nullness.qual.KeyFor; +import org.checkerframework.checker.nullness.qual.NonNull; import org.checkerframework.checker.nullness.qual.Nullable; import org.immutables.value.Value; @@ -41,9 +42,6 @@ *

The class is abstract to enforce usage of {@linkplain RootAllocator}/{@linkplain ChildAllocator} * facades. */ -@SuppressWarnings("nullness:dereference.of.nullable") -//dereference of possibly-null reference allocationManagerFactory, historicalLog, childLedgers, reservations, -//historicalLog, childAllocator.historicalLog, reservation.historicalLog, abstract class BaseAllocator extends Accountant implements BufferAllocator { public static final String DEBUG_ALLOCATOR = "arrow.memory.debug.allocator"; @@ -80,7 +78,7 @@ abstract class BaseAllocator extends Accountant implements BufferAllocator { private final @Nullable IdentityHashMap reservations; private final @Nullable HistoricalLog historicalLog; private final RoundingPolicy roundingPolicy; - private final AllocationManager.Factory allocationManagerFactory; + private final AllocationManager.@NonNull Factory allocationManagerFactory; private volatile boolean isClosed = false; // the allocator has been closed @@ -195,7 +193,9 @@ void associateLedger(BufferLedger ledger) { assertOpen(); if (DEBUG) { synchronized (DEBUG_LOCK) { - childLedgers.put(ledger, null); + if (childLedgers != null) { + childLedgers.put(ledger, null); + } } } } @@ -209,7 +209,7 @@ void dissociateLedger(BufferLedger ledger) { assertOpen(); if (DEBUG) { synchronized (DEBUG_LOCK) { - if (!childLedgers.containsKey(ledger)) { + if (!(childLedgers != null && childLedgers.containsKey(ledger))) { throw new IllegalStateException("Trying to remove a child ledger that doesn't exist."); } childLedgers.remove(ledger); @@ -231,7 +231,9 @@ private void childClosed(final BaseAllocator childAllocator) { synchronized (DEBUG_LOCK) { final Object object = childAllocators.remove(childAllocator); if (object == null) { - childAllocator.historicalLog.logHistory(logger); + if (childAllocator.historicalLog != null) { + childAllocator.historicalLog.logHistory(logger); + } throw new IllegalStateException("Child allocator[" + childAllocator.name + "] not found in parent allocator[" + name + "]'s childAllocators"); } @@ -288,7 +290,8 @@ public ArrowBuf buffer(final long initialRequestSize) { return buffer(initialRequestSize, null); } - private ArrowBuf createEmpty(@UnderInitialization BaseAllocator this) { + @SuppressWarnings("nullness:dereference.of.nullable")//dereference of possibly-null reference allocationManagerFactory + private ArrowBuf createEmpty() { return allocationManagerFactory.empty(); } @@ -396,8 +399,10 @@ public BufferAllocator newChildAllocator( if (DEBUG) { synchronized (DEBUG_LOCK) { childAllocators.put(childAllocator, childAllocator); - historicalLog.recordEvent("allocator[%s] created new child allocator[%s]", name, - childAllocator.getName()); + if (historicalLog != null) { + historicalLog.recordEvent("allocator[%s] created new child allocator[%s]", name, + childAllocator.getName()); + } } } else { childAllocators.put(childAllocator, childAllocator); @@ -447,14 +452,14 @@ public synchronized void close() { } // are there outstanding buffers? - final int allocatedCount = childLedgers.size(); + final int allocatedCount = childLedgers != null ? childLedgers.size() : 0; if (allocatedCount > 0) { throw new IllegalStateException( String.format("Allocator[%s] closed with outstanding buffers allocated (%d).\n%s", name, allocatedCount, toString())); } - if (reservations.size() != 0) { + if (reservations != null && reservations.size() != 0) { throw new IllegalStateException( String.format("Allocator[%s] closed with outstanding reservations (%d).\n%s", name, reservations.size(), @@ -494,7 +499,9 @@ public synchronized void close() { } if (DEBUG) { - historicalLog.recordEvent("closed"); + if (historicalLog != null) { + historicalLog.recordEvent("closed"); + } logger.debug(String.format("closed allocator[%s].", name)); } @@ -525,7 +532,9 @@ public String toVerboseString() { } private void hist(String noteFormat, Object... args) { - historicalLog.recordEvent(noteFormat, args); + if (historicalLog != null) { + historicalLog.recordEvent(noteFormat, args); + } } /** @@ -575,10 +584,14 @@ private void verifyAllocator( childTotal += Math.max(childAllocator.getAllocatedMemory(), childAllocator.reservation); } if (childTotal > getAllocatedMemory()) { - historicalLog.logHistory(logger); + if (historicalLog != null) { + historicalLog.logHistory(logger); + } logger.debug("allocator[" + name + "] child event logs BEGIN"); for (final BaseAllocator childAllocator : childSet) { - childAllocator.historicalLog.logHistory(logger); + if (childAllocator.historicalLog != null) { + childAllocator.historicalLog.logHistory(logger); + } } logger.debug("allocator[" + name + "] child event logs END"); throw new IllegalStateException( @@ -589,33 +602,39 @@ private void verifyAllocator( // Furthermore, the amount I've allocated should be that plus buffers I've allocated. long bufferTotal = 0; - final Set ledgerSet = childLedgers.keySet(); - for (final BufferLedger ledger : ledgerSet) { - if (!ledger.isOwningLedger()) { - continue; - } + final Set<@KeyFor("this.childLedgers") BufferLedger> ledgerSet = childLedgers != null ? + childLedgers.keySet() : null; + if (ledgerSet != null) { + for (final BufferLedger ledger : ledgerSet) { + if (!ledger.isOwningLedger()) { + continue; + } - final AllocationManager am = ledger.getAllocationManager(); - /* - * Even when shared, ArrowBufs are rewrapped, so we should never see the same instance - * twice. - */ - final BaseAllocator otherOwner = buffersSeen.get(am); - if (otherOwner != null) { - throw new IllegalStateException("This allocator's ArrowBuf already owned by another " + - "allocator"); - } - buffersSeen.put(am, this); + final AllocationManager am = ledger.getAllocationManager(); + /* + * Even when shared, ArrowBufs are rewrapped, so we should never see the same instance + * twice. + */ + final BaseAllocator otherOwner = buffersSeen.get(am); + if (otherOwner != null) { + throw new IllegalStateException("This allocator's ArrowBuf already owned by another " + + "allocator"); + } + buffersSeen.put(am, this); - bufferTotal += am.getSize(); + bufferTotal += am.getSize(); + } } // Preallocated space has to be accounted for - final Set reservationSet = reservations.keySet(); + final Set<@KeyFor("this.reservations") Reservation> reservationSet = reservations != null ? + reservations.keySet() : null; long reservedTotal = 0; - for (final Reservation reservation : reservationSet) { - if (!reservation.isUsed()) { - reservedTotal += reservation.getSize(); + if (reservationSet != null) { + for (final Reservation reservation : reservationSet) { + if (!reservation.isUsed()) { + reservedTotal += reservation.getSize(); + } } } @@ -652,9 +671,13 @@ private void verifyAllocator( if (reservedTotal != 0) { sb.append(String.format("reserved total : %d bytes.", reservedTotal)); - for (final Reservation reservation : reservationSet) { - reservation.historicalLog.buildHistory(sb, 0, true); - sb.append('\n'); + if (reservationSet != null) { + for (final Reservation reservation : reservationSet) { + if (reservation.historicalLog != null) { + reservation.historicalLog.buildHistory(sb, 0, true); + } + sb.append('\n'); + } } } @@ -697,16 +720,25 @@ void print(StringBuilder sb, int level, Verbosity verbosity) { child.print(sb, level + 2, verbosity); } - CommonUtil.indent(sb, level + 1).append(String.format("ledgers: %d\n", childLedgers.size())); - for (BufferLedger ledger : childLedgers.keySet()) { - ledger.print(sb, level + 2, verbosity); + CommonUtil.indent(sb, level + 1).append(String.format("ledgers: %d\n", childLedgers != null ? + childLedgers.size() : 0)); + if (childLedgers != null) { + for (BufferLedger ledger : childLedgers.keySet()) { + ledger.print(sb, level + 2, verbosity); + } } - final Set reservations = this.reservations.keySet(); - CommonUtil.indent(sb, level + 1).append(String.format("reservations: %d\n", reservations.size())); - for (final Reservation reservation : reservations) { - if (verbosity.includeHistoricalLog) { - reservation.historicalLog.buildHistory(sb, level + 3, true); + final Set<@KeyFor("this.reservations") Reservation> reservations = this.reservations != null ? + this.reservations.keySet() : null; + CommonUtil.indent(sb, level + 1).append(String.format("reservations: %d\n", + reservations != null ? reservations.size() : 0)); + if (reservations != null) { + for (final Reservation reservation : reservations) { + if (verbosity.includeHistoricalLog) { + if (reservation.historicalLog != null) { + reservation.historicalLog.buildHistory(sb, level + 3, true); + } + } } } @@ -714,17 +746,20 @@ void print(StringBuilder sb, int level, Verbosity verbosity) { } - private void dumpBuffers(final StringBuilder sb, final Set ledgerSet) { - for (final BufferLedger ledger : ledgerSet) { - if (!ledger.isOwningLedger()) { - continue; + private void dumpBuffers(final StringBuilder sb, + final @Nullable Set<@KeyFor("this.childLedgers") BufferLedger> ledgerSet) { + if (ledgerSet != null) { + for (final BufferLedger ledger : ledgerSet) { + if (!ledger.isOwningLedger()) { + continue; + } + final AllocationManager am = ledger.getAllocationManager(); + sb.append("UnsafeDirectLittleEndian[identityHashCode == "); + sb.append(Integer.toString(System.identityHashCode(am))); + sb.append("] size "); + sb.append(Long.toString(am.getSize())); + sb.append('\n'); } - final AllocationManager am = ledger.getAllocationManager(); - sb.append("UnsafeDirectLittleEndian[identityHashCode == "); - sb.append(Integer.toString(System.identityHashCode(am))); - sb.append("] size "); - sb.append(Long.toString(am.getSize())); - sb.append('\n'); } } @@ -832,14 +867,16 @@ public class Reservation implements AllocationReservation { *

If {@linkplain #DEBUG} is true this will capture a historical * log of events relevant to this Reservation. */ - @SuppressWarnings("nullness:argument") //incompatible argument for parameter arg0 of System.identityHashCode. + @SuppressWarnings("nullness:argument")//to handle null assignment on third party dependency: System.identityHashCode public Reservation() { if (DEBUG) { historicalLog = new HistoricalLog("Reservation[allocator[%s], %d]", name, System .identityHashCode(this)); historicalLog.recordEvent("created"); synchronized (DEBUG_LOCK) { - reservations.put(this, this); + if (reservations != null) { + reservations.put(this, this); + } } } else { historicalLog = null; @@ -910,7 +947,7 @@ public void close() { if (!isClosed()) { final Object object; synchronized (DEBUG_LOCK) { - object = reservations.remove(this); + object = reservations != null ? reservations.remove(this) : null; } if (object == null) { final StringBuilder sb = new StringBuilder(); @@ -920,7 +957,9 @@ public void close() { System.identityHashCode(this))); } - historicalLog.recordEvent("closed"); + if (historicalLog != null) { + historicalLog.recordEvent("closed"); + } } } @@ -937,7 +976,7 @@ public boolean reserve(int nBytes) { final AllocationOutcome outcome = BaseAllocator.this.allocateBytes(nBytes); - if (DEBUG) { + if (DEBUG && historicalLog != null) { historicalLog.recordEvent("reserve(%d) => %s", nBytes, Boolean.toString(outcome.isOk())); } @@ -968,7 +1007,7 @@ private ArrowBuf allocate(int nBytes) { final ArrowBuf arrowBuf = BaseAllocator.this.bufferWithoutReservation(nBytes, null); listener.onAllocation(nBytes); - if (DEBUG) { + if (DEBUG && historicalLog != null) { historicalLog.recordEvent("allocate() => %s", String.format("ArrowBuf[%d]", arrowBuf .getId())); } @@ -991,7 +1030,7 @@ private void releaseReservation(int nBytes) { releaseBytes(nBytes); - if (DEBUG) { + if (DEBUG && historicalLog != null) { historicalLog.recordEvent("releaseReservation(%d)", nBytes); } } diff --git a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/BufferLedger.java b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/BufferLedger.java index a38aa107c43..53e268ac3d0 100644 --- a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/BufferLedger.java +++ b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/BufferLedger.java @@ -118,13 +118,12 @@ public boolean release() { * @return true if the new ref count has dropped to 0, false otherwise */ @Override - @SuppressWarnings("nullness:dereference.of.nullable") //dereference of possibly-null reference historicalLog public boolean release(int decrement) { Preconditions.checkState(decrement >= 1, "ref count decrement should be greater than or equal to 1"); // decrement the ref count final int refCnt = decrement(decrement); - if (BaseAllocator.DEBUG) { + if (BaseAllocator.DEBUG && historicalLog != null) { historicalLog.recordEvent("release(%d). original value: %d", decrement, refCnt + decrement); } @@ -178,10 +177,9 @@ public void retain() { * @param increment amount to increase the reference count by */ @Override - @SuppressWarnings("nullness:dereference.of.nullable") //dereference of possibly-null reference historicalLog public void retain(int increment) { Preconditions.checkArgument(increment > 0, "retain(%s) argument is not positive", increment); - if (BaseAllocator.DEBUG) { + if (BaseAllocator.DEBUG && historicalLog != null) { historicalLog.recordEvent("retain(%d)", increment); } final int originalReferenceCount = bufRefCnt.getAndAdd(increment); @@ -311,10 +309,9 @@ ArrowBuf newArrowBuf(final long length, final @Nullable BufferManager manager) { * @return A new ArrowBuf which shares the same underlying memory as the provided ArrowBuf. */ @Override - @SuppressWarnings("nullness:dereference.of.nullable") //dereference of possibly-null reference historicalLog public ArrowBuf retain(final ArrowBuf srcBuffer, BufferAllocator target) { - if (BaseAllocator.DEBUG) { + if (BaseAllocator.DEBUG && historicalLog != null) { historicalLog.recordEvent("retain(%s)", target.getName()); } @@ -341,47 +338,48 @@ public ArrowBuf retain(final ArrowBuf srcBuffer, BufferAllocator target) { * @param targetReferenceManager The ledger to transfer ownership account to. * @return Whether transfer fit within target ledgers limits. */ - @SuppressWarnings("nullness:dereference.of.nullable") - //dereference of possibly-null reference historicalLog, targetReferenceManager boolean transferBalance(final @Nullable ReferenceManager targetReferenceManager) { - Preconditions.checkArgument(targetReferenceManager != null, - "Expecting valid target reference manager"); - final BufferAllocator targetAllocator = targetReferenceManager.getAllocator(); - Preconditions.checkArgument(allocator.getRoot() == targetAllocator.getRoot(), - "You can only transfer between two allocators that share the same root."); + if (targetReferenceManager != null) { + final BufferAllocator targetAllocator = targetReferenceManager.getAllocator(); + Preconditions.checkArgument(allocator.getRoot() == targetAllocator.getRoot(), + "You can only transfer between two allocators that share the same root."); - allocator.assertOpen(); - targetReferenceManager.getAllocator().assertOpen(); + allocator.assertOpen(); + targetReferenceManager.getAllocator().assertOpen(); - // if we're transferring to ourself, just return. - if (targetReferenceManager == this) { - return true; - } - - // since two balance transfers out from the allocation manager could cause incorrect - // accounting, we need to ensure - // that this won't happen by synchronizing on the allocation manager instance. - synchronized (allocationManager) { - if (allocationManager.getOwningLedger() != this) { - // since the calling reference manager is not the owning - // reference manager for the underlying memory, transfer is - // a NO-OP + // if we're transferring to ourself, just return. + if (targetReferenceManager == this) { return true; } - if (BaseAllocator.DEBUG) { - this.historicalLog.recordEvent("transferBalance(%s)", - targetReferenceManager.getAllocator().getName()); - } + // since two balance transfers out from the allocation manager could cause incorrect + // accounting, we need to ensure + // that this won't happen by synchronizing on the allocation manager instance. + synchronized (allocationManager) { + if (allocationManager.getOwningLedger() != this) { + // since the calling reference manager is not the owning + // reference manager for the underlying memory, transfer is + // a NO-OP + return true; + } + + if (BaseAllocator.DEBUG && this.historicalLog != null) { + this.historicalLog.recordEvent("transferBalance(%s)", + targetReferenceManager.getAllocator().getName()); + } - boolean overlimit = targetAllocator.forceAllocate(allocationManager.getSize()); - allocator.releaseBytes(allocationManager.getSize()); - // since the transfer can only happen from the owning reference manager, - // we need to set the target ref manager as the new owning ref manager - // for the chunk of memory in allocation manager - allocationManager.setOwningLedger((BufferLedger) targetReferenceManager); - return overlimit; + boolean overlimit = targetAllocator.forceAllocate(allocationManager.getSize()); + allocator.releaseBytes(allocationManager.getSize()); + // since the transfer can only happen from the owning reference manager, + // we need to set the target ref manager as the new owning ref manager + // for the chunk of memory in allocation manager + allocationManager.setOwningLedger((BufferLedger) targetReferenceManager); + return overlimit; + } + } else { + throw new IllegalArgumentException("Expecting valid target reference manager"); } + } /** diff --git a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/DefaultAllocationManagerOption.java b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/DefaultAllocationManagerOption.java index 81bc06813ba..d57b72ba415 100644 --- a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/DefaultAllocationManagerOption.java +++ b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/DefaultAllocationManagerOption.java @@ -19,7 +19,7 @@ import java.lang.reflect.Field; -import org.checkerframework.checker.nullness.qual.NonNull; +import org.checkerframework.checker.nullness.qual.Nullable; /** @@ -42,8 +42,7 @@ public class DefaultAllocationManagerOption { /** * The default allocation manager factory. */ - @SuppressWarnings("nullness:assignment") //static member qualifying type may not be annotated - private static AllocationManager.Factory DEFAULT_ALLOCATION_MANAGER_FACTORY = null; + private static AllocationManager.@Nullable Factory DEFAULT_ALLOCATION_MANAGER_FACTORY = null; /** * The allocation manager type. @@ -65,8 +64,8 @@ public enum AllocationManagerType { Unknown, } - @SuppressWarnings("nullness:argument") //enum types are implicitly non-null - static @NonNull AllocationManagerType getDefaultAllocationManagerType() { + @SuppressWarnings("nullness:argument") //enum types valueOf are implicitly non-null + static AllocationManagerType getDefaultAllocationManagerType() { AllocationManagerType ret = AllocationManagerType.Unknown; try { diff --git a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/LowCostIdentityHashMap.java b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/LowCostIdentityHashMap.java index de5bf9f42d2..91752775466 100644 --- a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/LowCostIdentityHashMap.java +++ b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/LowCostIdentityHashMap.java @@ -19,6 +19,8 @@ import org.apache.arrow.util.Preconditions; import org.apache.arrow.util.VisibleForTesting; +import org.checkerframework.checker.initialization.qual.Initialized; +import org.checkerframework.checker.initialization.qual.UnderInitialization; import org.checkerframework.checker.nullness.qual.Nullable; /** @@ -66,24 +68,24 @@ public LowCostIdentityHashMap() { * The estimated maximum number of entries that will be put in * this map. */ - @SuppressWarnings("nullness:method.invocation") //call to getThreshold, newElementArray not allowed the given receiver public LowCostIdentityHashMap(int maxSize) { if (maxSize >= 0) { this.size = 0; threshold = getThreshold(maxSize); - elementData = newElementArray(computeElementArraySize()); + elementData = newElementArrayUnderInitialization(computeElementArraySize()); } else { throw new IllegalArgumentException(); } } - private int getThreshold(int maxSize) { + private int getThreshold(@UnderInitialization LowCostIdentityHashMap this, + int maxSize) { // assign the threshold to maxSize initially, this will change to a // higher value if rehashing occurs. return maxSize > 2 ? maxSize : 2; } - private int computeElementArraySize() { + private int computeElementArraySize(@UnderInitialization LowCostIdentityHashMap this) { int arraySize = (int) (((long) threshold * 10000) / LOAD_FACTOR); // ensure arraySize is positive, the above cast from long to int type // leads to overflow and negative arraySize if threshold is too big @@ -97,7 +99,18 @@ private int computeElementArraySize() { * the number of elements * @return Reference to the element array */ - private Object[] newElementArray(int s) { + private Object[] newElementArrayInitialized(@Initialized LowCostIdentityHashMap this, int s) { + return new Object[s]; + } + + /** + * Create a new element array. + * + * @param s + * the number of elements + * @return Reference to the element array + */ + private Object[] newElementArrayUnderInitialization(@UnderInitialization LowCostIdentityHashMap this, int s) { return new Object[s]; } @@ -154,7 +167,6 @@ public boolean containsValue(V value) { * @param key the key. * @return the value of the mapping with the specified key. */ - @SuppressWarnings("nullness:cast.unsafe") //cast cannot be statically verified public @Nullable V get(K key) { Preconditions.checkNotNull(key); @@ -198,7 +210,6 @@ static int getModuloHash(@Nullable Object key, int length) { * @return the value of any previous mapping with the specified key or * {@code null} if there was no such mapping. */ - @SuppressWarnings("nullness:cast.unsafe") //cast cannot be statically verified public V put(V value) { Preconditions.checkNotNull(value); K key = value.getKey(); @@ -230,7 +241,7 @@ void rehash() { if (newlength == 0) { newlength = 1; } - @Nullable Object[] newData = newElementArray(newlength); + @Nullable Object[] newData = newElementArrayInitialized(newlength); for (int i = 0; i < elementData.length; i++) { Object key = (elementData[i] == null) ? null : ((V) elementData[i]).getKey(); if (key != null) { @@ -254,7 +265,6 @@ private void computeMaxSize() { * @return the value of the removed mapping, or {@code null} if no mapping * for the specified key was found. */ - @SuppressWarnings("nullness:cast.unsafe") //cast cannot be statically verified public @Nullable V remove(K key) { Preconditions.checkNotNull(key); diff --git a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/ArrowBufPointer.java b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/ArrowBufPointer.java index f843b45c487..89309c2a80a 100644 --- a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/ArrowBufPointer.java +++ b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/ArrowBufPointer.java @@ -34,7 +34,7 @@ public final class ArrowBufPointer { */ public static final int NULL_HASH_CODE = 0; - private ArrowBuf buf; + private @Nullable ArrowBuf buf; private long offset; @@ -60,10 +60,10 @@ public ArrowBufPointer() { * Constructs an arrow buffer pointer with the specified hasher. * @param hasher the hasher to use. */ - @SuppressWarnings("nullness:initialization.fields.uninitialized") //the constructor does not initialize fields: buf public ArrowBufPointer(ArrowBufHasher hasher) { Preconditions.checkNotNull(hasher); this.hasher = hasher; + this.buf = null; } /** @@ -110,7 +110,7 @@ public void set(ArrowBuf buf, long offset, long length) { * Gets the underlying buffer, or null if the underlying data is invalid or null. * @return the underlying buffer, if any, or null if the underlying data is invalid or null. */ - public ArrowBuf getBuf() { + public @Nullable ArrowBuf getBuf() { return buf; } diff --git a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/MemoryUtil.java b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/MemoryUtil.java index 6f619610dfd..2abec842c7b 100644 --- a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/MemoryUtil.java +++ b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/MemoryUtil.java @@ -186,7 +186,7 @@ public static ByteBuffer directBuffer(long address, int capacity) { "sun.misc.Unsafe or java.nio.DirectByteBuffer.(long, int) not available"); } - @SuppressWarnings("nullness:argument") //to handle null assignment on third party dependency + @SuppressWarnings("nullness:argument") //to handle null assignment on third party dependency: Unsafe public static void copyMemory(@Nullable Object srcBase, long srcOffset, @Nullable Object destBase, long destOffset, long bytes) { diff --git a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/StackTrace.java b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/StackTrace.java index c6d7d7e0494..2e88c6b8376 100644 --- a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/StackTrace.java +++ b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/StackTrace.java @@ -19,17 +19,19 @@ import java.util.Arrays; +import org.checkerframework.checker.nullness.qual.Nullable; + + /** * Convenient way of obtaining and manipulating stack traces for debugging. */ public class StackTrace { - private final StackTraceElement[] stackTraceElements; + private final @Nullable StackTraceElement [] stackTraceElements; /** * Constructor. Captures the current stack trace. */ - @SuppressWarnings("nullness:assignment") //incompatible types in assignment public StackTrace() { final StackTraceElement[] stack = Thread.currentThread().getStackTrace(); // Skip first two elements to remove getStackTrace/StackTrace. @@ -49,16 +51,18 @@ public void writeToBuilder(final StringBuilder sb, final int indent) { // write the stack trace in standard Java format for (StackTraceElement ste : stackTraceElements) { - sb.append(indentation) - .append("at ") - .append(ste.getClassName()) - .append('.') - .append(ste.getMethodName()) - .append('(') - .append(ste.getFileName()) - .append(':') - .append(Integer.toString(ste.getLineNumber())) - .append(")\n"); + if (ste != null) { + sb.append(indentation) + .append("at ") + .append(ste.getClassName()) + .append('.') + .append(ste.getMethodName()) + .append('(') + .append(ste.getFileName()) + .append(':') + .append(Integer.toString(ste.getLineNumber())) + .append(")\n"); + } } } From 6e2876a0f00a7e739d455d087cf4ee21e73f9762 Mon Sep 17 00:00:00 2001 From: David Susanibar Arce Date: Mon, 16 Oct 2023 18:03:45 -0500 Subject: [PATCH 11/18] fix: synch last changes to reduce suppress warnings --- .../arrow/memory/util/ArrowBufPointer.java | 2 -- .../apache/arrow/memory/util/StackTrace.java | 18 +++++++++--------- 2 files changed, 9 insertions(+), 11 deletions(-) diff --git a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/ArrowBufPointer.java b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/ArrowBufPointer.java index 89309c2a80a..b41576847d6 100644 --- a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/ArrowBufPointer.java +++ b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/ArrowBufPointer.java @@ -83,8 +83,6 @@ public ArrowBufPointer(ArrowBuf buf, long offset, long length) { * @param length the length off set of the memory region pointed to. * @param hasher the hasher used to calculate the hash code. */ - @SuppressWarnings({"nullness:initialization.fields.uninitialized", "nullness:method.invocation"}) - // {"the constructor does not initialize fields: buf", "call to set not allowed on the given receiver."} public ArrowBufPointer(ArrowBuf buf, long offset, long length, ArrowBufHasher hasher) { Preconditions.checkNotNull(hasher); this.hasher = hasher; diff --git a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/StackTrace.java b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/StackTrace.java index 2e88c6b8376..a533edd7937 100644 --- a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/StackTrace.java +++ b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/StackTrace.java @@ -53,15 +53,15 @@ public void writeToBuilder(final StringBuilder sb, final int indent) { for (StackTraceElement ste : stackTraceElements) { if (ste != null) { sb.append(indentation) - .append("at ") - .append(ste.getClassName()) - .append('.') - .append(ste.getMethodName()) - .append('(') - .append(ste.getFileName()) - .append(':') - .append(Integer.toString(ste.getLineNumber())) - .append(")\n"); + .append("at ") + .append(ste.getClassName()) + .append('.') + .append(ste.getMethodName()) + .append('(') + .append(ste.getFileName()) + .append(':') + .append(Integer.toString(ste.getLineNumber())) + .append(")\n"); } } } From 4de6d6440ce217d2e74534ab999c70be42179dda Mon Sep 17 00:00:00 2001 From: david dali susanibar arce Date: Wed, 13 Dec 2023 18:56:36 -0500 Subject: [PATCH 12/18] fix: windows env error --- java/memory/memory-core/pom.xml | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/java/memory/memory-core/pom.xml b/java/memory/memory-core/pom.xml index adbcbf1abbb..b914b1fa10d 100644 --- a/java/memory/memory-core/pom.xml +++ b/java/memory/memory-core/pom.xml @@ -114,9 +114,7 @@ 10000 -Xmaxwarns 10000 - - -AskipDefs=.*Test|\ - + -AskipDefs=.*Test -AatfDoNotCache From c7fb710ea40e519898a3eef8f040aba4c2e40106 Mon Sep 17 00:00:00 2001 From: david dali susanibar arce Date: Thu, 21 Dec 2023 13:30:57 -0500 Subject: [PATCH 13/18] fix: addressing PR comments --- .../apache/arrow/memory/AllocationManager.java | 14 ++++++++------ .../java/org/apache/arrow/memory/ArrowBuf.java | 16 ++++++---------- .../org/apache/arrow/memory/BaseAllocator.java | 2 +- .../org/apache/arrow/memory/BufferLedger.java | 16 ++++++++-------- .../arrow/memory/LowCostIdentityHashMap.java | 4 ++-- 5 files changed, 25 insertions(+), 27 deletions(-) diff --git a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/AllocationManager.java b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/AllocationManager.java index 503452e5fbe..013803bf939 100644 --- a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/AllocationManager.java +++ b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/AllocationManager.java @@ -137,7 +137,9 @@ void release(final BufferLedger ledger) { Preconditions.checkState(map.containsKey(allocator), "Expecting a mapping for allocator and reference manager"); final BufferLedger oldLedger = map.remove(allocator); - + // to cover current NPE logic before Nullability evaluations + Preconditions.checkState(oldLedger != null, + "Expecting a valid BufferLedger, but received null instead"); if (oldLedger != null) { BufferAllocator oldAllocator = oldLedger.getAllocator(); @@ -167,12 +169,12 @@ void release(final BufferLedger ledger) { // exceeded the limit since this consumer can't do anything with this. oldLedger.transferBalance(newOwningLedger); } + } else { + // the release call was made by a non-owning reference manager, so after remove there have + // to be 1 or more mappings + Preconditions.checkState(map.size() > 0, + "The final removal of reference manager should be connected to owning reference manager"); } - } else { - // the release call was made by a non-owning reference manager, so after remove there have - // to be 1 or more mappings - Preconditions.checkState(map.size() > 0, - "The final removal of reference manager should be connected to owning reference manager"); } } diff --git a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/ArrowBuf.java b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/ArrowBuf.java index 988f4ca5bfd..4da5fb6fd43 100644 --- a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/ArrowBuf.java +++ b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/ArrowBuf.java @@ -94,7 +94,7 @@ public ArrowBuf( this.capacity = capacity; this.readerIndex = 0; this.writerIndex = 0; - if (BaseAllocator.DEBUG && historicalLog != null) { + if (historicalLog != null) { historicalLog.recordEvent("create()"); } } @@ -314,10 +314,8 @@ private void checkIndexD(long index, long fieldLength) { // check bounds Preconditions.checkArgument(fieldLength >= 0, "expecting non-negative data length"); if (index < 0 || index > capacity() - fieldLength) { - if (BaseAllocator.DEBUG) { - if (historicalLog != null) { - historicalLog.logHistory(logger); - } + if (historicalLog != null) { + historicalLog.logHistory(logger); } throw new IndexOutOfBoundsException(String.format( "index: %d, length: %d (expected: range(0, %d))", index, fieldLength, capacity())); @@ -1112,11 +1110,9 @@ public long getId() { public void print(StringBuilder sb, int indent, Verbosity verbosity) { CommonUtil.indent(sb, indent).append(toString()); - if (BaseAllocator.DEBUG && verbosity.includeHistoricalLog) { - if (historicalLog != null) { - sb.append("\n"); - historicalLog.buildHistory(sb, indent + 1, verbosity.includeStackTraces); - } + if (historicalLog != null && verbosity.includeHistoricalLog) { + sb.append("\n"); + historicalLog.buildHistory(sb, indent + 1, verbosity.includeStackTraces); } } diff --git a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/BaseAllocator.java b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/BaseAllocator.java index e26066fc40c..9453a43ac2a 100644 --- a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/BaseAllocator.java +++ b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/BaseAllocator.java @@ -976,7 +976,7 @@ public boolean reserve(int nBytes) { final AllocationOutcome outcome = BaseAllocator.this.allocateBytes(nBytes); - if (DEBUG && historicalLog != null) { + if (historicalLog != null) { historicalLog.recordEvent("reserve(%d) => %s", nBytes, Boolean.toString(outcome.isOk())); } diff --git a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/BufferLedger.java b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/BufferLedger.java index 53e268ac3d0..18ff872bad8 100644 --- a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/BufferLedger.java +++ b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/BufferLedger.java @@ -123,7 +123,7 @@ public boolean release(int decrement) { "ref count decrement should be greater than or equal to 1"); // decrement the ref count final int refCnt = decrement(decrement); - if (BaseAllocator.DEBUG && historicalLog != null) { + if (historicalLog != null) { historicalLog.recordEvent("release(%d). original value: %d", decrement, refCnt + decrement); } @@ -206,7 +206,7 @@ public void retain(int increment) { */ @Override @SuppressWarnings({"nullness:dereference.of.nullable", "nullness:locking.nullable"}) - //{"dereference of possibly-null reference historicalLog", "synchronizing over a possibly-null lock (buffers)"} + //{"dereference of possibly-null reference buffers", "synchronizing over a possibly-null lock (buffers)"} public ArrowBuf deriveBuffer(final ArrowBuf sourceBuffer, long index, long length) { /* * Usage type 1 for deriveBuffer(): @@ -236,7 +236,7 @@ public ArrowBuf deriveBuffer(final ArrowBuf sourceBuffer, long index, long lengt ); // logging - if (BaseAllocator.DEBUG) { + if (historicalLog != null) { historicalLog.recordEvent( "ArrowBuf(BufferLedger, BufferAllocator[%s], " + "UnsafeDirectLittleEndian[identityHashCode == " + @@ -339,6 +339,9 @@ public ArrowBuf retain(final ArrowBuf srcBuffer, BufferAllocator target) { * @return Whether transfer fit within target ledgers limits. */ boolean transferBalance(final @Nullable ReferenceManager targetReferenceManager) { + Preconditions.checkArgument(targetReferenceManager != null, + "Expecting valid target reference manager"); + boolean overlimit = false; if (targetReferenceManager != null) { final BufferAllocator targetAllocator = targetReferenceManager.getAllocator(); Preconditions.checkArgument(allocator.getRoot() == targetAllocator.getRoot(), @@ -368,18 +371,15 @@ boolean transferBalance(final @Nullable ReferenceManager targetReferenceManager) targetReferenceManager.getAllocator().getName()); } - boolean overlimit = targetAllocator.forceAllocate(allocationManager.getSize()); + overlimit = targetAllocator.forceAllocate(allocationManager.getSize()); allocator.releaseBytes(allocationManager.getSize()); // since the transfer can only happen from the owning reference manager, // we need to set the target ref manager as the new owning ref manager // for the chunk of memory in allocation manager allocationManager.setOwningLedger((BufferLedger) targetReferenceManager); - return overlimit; } - } else { - throw new IllegalArgumentException("Expecting valid target reference manager"); } - + return overlimit; } /** diff --git a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/LowCostIdentityHashMap.java b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/LowCostIdentityHashMap.java index 91752775466..740233ef411 100644 --- a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/LowCostIdentityHashMap.java +++ b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/LowCostIdentityHashMap.java @@ -72,7 +72,7 @@ public LowCostIdentityHashMap(int maxSize) { if (maxSize >= 0) { this.size = 0; threshold = getThreshold(maxSize); - elementData = newElementArrayUnderInitialization(computeElementArraySize()); + elementData = newElementArrayUnderInitialized(computeElementArraySize()); } else { throw new IllegalArgumentException(); } @@ -110,7 +110,7 @@ private Object[] newElementArrayInitialized(@Initialized LowCostIdentityHashMap< * the number of elements * @return Reference to the element array */ - private Object[] newElementArrayUnderInitialization(@UnderInitialization LowCostIdentityHashMap this, int s) { + private Object[] newElementArrayUnderInitialized(@UnderInitialization LowCostIdentityHashMap this, int s) { return new Object[s]; } From d6534b4b06a83a8f5232db5360f54c609510777f Mon Sep 17 00:00:00 2001 From: david dali susanibar arce Date: Thu, 21 Dec 2023 21:44:19 -0500 Subject: [PATCH 14/18] fix: addressing PR comments --- .../src/main/java/org/apache/arrow/memory/BufferLedger.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/BufferLedger.java b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/BufferLedger.java index 18ff872bad8..672efbf0181 100644 --- a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/BufferLedger.java +++ b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/BufferLedger.java @@ -179,7 +179,7 @@ public void retain() { @Override public void retain(int increment) { Preconditions.checkArgument(increment > 0, "retain(%s) argument is not positive", increment); - if (BaseAllocator.DEBUG && historicalLog != null) { + if (historicalLog != null) { historicalLog.recordEvent("retain(%d)", increment); } final int originalReferenceCount = bufRefCnt.getAndAdd(increment); @@ -311,7 +311,7 @@ ArrowBuf newArrowBuf(final long length, final @Nullable BufferManager manager) { @Override public ArrowBuf retain(final ArrowBuf srcBuffer, BufferAllocator target) { - if (BaseAllocator.DEBUG && historicalLog != null) { + if (historicalLog != null) { historicalLog.recordEvent("retain(%s)", target.getName()); } From f2371bac3be186d78450df81cdaeef5dadc932f5 Mon Sep 17 00:00:00 2001 From: david dali susanibar arce Date: Fri, 22 Dec 2023 12:47:45 -0500 Subject: [PATCH 15/18] fix: adding support with @AssertMethod --- .../org/apache/arrow/memory/Accountant.java | 4 +- .../arrow/memory/AllocationManager.java | 71 +++++++++---------- .../org/apache/arrow/memory/BufferLedger.java | 31 +++----- .../memory/util/CheckerFrameworkUtil.java | 39 ++++++++++ java/pom.xml | 2 +- 5 files changed, 86 insertions(+), 61 deletions(-) create mode 100644 java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/CheckerFrameworkUtil.java diff --git a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/Accountant.java b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/Accountant.java index c9675536ae3..becefc4f112 100644 --- a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/Accountant.java +++ b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/Accountant.java @@ -21,6 +21,7 @@ import javax.annotation.concurrent.ThreadSafe; +import org.apache.arrow.memory.util.CheckerFrameworkUtil; import org.apache.arrow.util.Preconditions; import org.checkerframework.checker.nullness.qual.Nullable; @@ -73,7 +74,8 @@ public Accountant(@Nullable Accountant parent, String name, long reservation, lo this.reservation = reservation; this.allocationLimit.set(maxAllocation); - if (reservation != 0 && parent != null) { + if (reservation != 0) { + CheckerFrameworkUtil.assertMethod(parent != null); // we will allocate a reservation from our parent. final AllocationOutcome outcome = parent.allocateBytes(reservation); if (!outcome.isOk()) { diff --git a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/AllocationManager.java b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/AllocationManager.java index 013803bf939..4a604d0b291 100644 --- a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/AllocationManager.java +++ b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/AllocationManager.java @@ -17,6 +17,7 @@ package org.apache.arrow.memory; +import org.apache.arrow.memory.util.CheckerFrameworkUtil; import org.apache.arrow.util.Preconditions; import org.checkerframework.checker.nullness.qual.Nullable; @@ -135,46 +136,42 @@ void release(final BufferLedger ledger) { // remove the mapping for the allocator // of calling BufferLedger Preconditions.checkState(map.containsKey(allocator), - "Expecting a mapping for allocator and reference manager"); + "Expecting a mapping for allocator and reference manager"); final BufferLedger oldLedger = map.remove(allocator); - // to cover current NPE logic before Nullability evaluations - Preconditions.checkState(oldLedger != null, - "Expecting a valid BufferLedger, but received null instead"); - - if (oldLedger != null) { - BufferAllocator oldAllocator = oldLedger.getAllocator(); - if (oldAllocator instanceof BaseAllocator) { - // needed for debug only: tell the allocator that AllocationManager is removing a - // reference manager associated with this particular allocator - ((BaseAllocator) oldAllocator).dissociateLedger(oldLedger); - } - if (oldLedger == owningLedger) { - // the release call was made by the owning reference manager - if (map.isEmpty()) { - // the only mapping was for the owner - // which now has been removed, it implies we can safely destroy the - // underlying memory chunk as it is no longer being referenced - oldAllocator.releaseBytes(getSize()); - // free the memory chunk associated with the allocation manager - release0(); - oldAllocator.getListener().onRelease(getSize()); - owningLedger = null; - } else { - // since the refcount dropped to 0 for the owning reference manager and allocation - // manager will no longer keep a mapping for it, we need to change the owning - // reference manager to whatever the next available - // mapping exists. - BufferLedger newOwningLedger = map.getNextValue(); - // we'll forcefully transfer the ownership and not worry about whether we - // exceeded the limit since this consumer can't do anything with this. - oldLedger.transferBalance(newOwningLedger); - } + CheckerFrameworkUtil.assertMethod(oldLedger != null); + BufferAllocator oldAllocator = oldLedger.getAllocator(); + if (oldAllocator instanceof BaseAllocator) { + // needed for debug only: tell the allocator that AllocationManager is removing a + // reference manager associated with this particular allocator + ((BaseAllocator) oldAllocator).dissociateLedger(oldLedger); + } + + if (oldLedger == owningLedger) { + // the release call was made by the owning reference manager + if (map.isEmpty()) { + // the only mapping was for the owner + // which now has been removed, it implies we can safely destroy the + // underlying memory chunk as it is no longer being referenced + oldAllocator.releaseBytes(getSize()); + // free the memory chunk associated with the allocation manager + release0(); + oldAllocator.getListener().onRelease(getSize()); + owningLedger = null; } else { - // the release call was made by a non-owning reference manager, so after remove there have - // to be 1 or more mappings - Preconditions.checkState(map.size() > 0, - "The final removal of reference manager should be connected to owning reference manager"); + // since the refcount dropped to 0 for the owning reference manager and allocation + // manager will no longer keep a mapping for it, we need to change the owning + // reference manager to whatever the next available + // mapping exists. + BufferLedger newOwningLedger = map.getNextValue(); + // we'll forcefully transfer the ownership and not worry about whether we + // exceeded the limit since this consumer can't do anything with this. + oldLedger.transferBalance(newOwningLedger); } + } else { + // the release call was made by a non-owning reference manager, so after remove there have + // to be 1 or more mappings + Preconditions.checkState(map.size() > 0, + "The final removal of reference manager should be connected to owning reference manager"); } } diff --git a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/BufferLedger.java b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/BufferLedger.java index 672efbf0181..56ab33abc84 100644 --- a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/BufferLedger.java +++ b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/BufferLedger.java @@ -21,6 +21,7 @@ import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicLong; +import org.apache.arrow.memory.util.CheckerFrameworkUtil; import org.apache.arrow.memory.util.CommonUtil; import org.apache.arrow.memory.util.HistoricalLog; import org.apache.arrow.util.Preconditions; @@ -205,8 +206,6 @@ public void retain(int increment) { * @return derived buffer */ @Override - @SuppressWarnings({"nullness:dereference.of.nullable", "nullness:locking.nullable"}) - //{"dereference of possibly-null reference buffers", "synchronizing over a possibly-null lock (buffers)"} public ArrowBuf deriveBuffer(final ArrowBuf sourceBuffer, long index, long length) { /* * Usage type 1 for deriveBuffer(): @@ -236,20 +235,7 @@ public ArrowBuf deriveBuffer(final ArrowBuf sourceBuffer, long index, long lengt ); // logging - if (historicalLog != null) { - historicalLog.recordEvent( - "ArrowBuf(BufferLedger, BufferAllocator[%s], " + - "UnsafeDirectLittleEndian[identityHashCode == " + - "%d](%s)) => ledger hc == %d", - allocator.getName(), System.identityHashCode(derivedBuf), derivedBuf.toString(), - System.identityHashCode(this)); - - synchronized (buffers) { - buffers.put(derivedBuf, null); - } - } - - return derivedBuf; + return loggingArrowBufHistoricalLog(derivedBuf); } /** @@ -264,8 +250,6 @@ public ArrowBuf deriveBuffer(final ArrowBuf sourceBuffer, long index, long lengt * @return A new ArrowBuf that shares references with all ArrowBufs associated * with this BufferLedger */ - @SuppressWarnings({"nullness:dereference.of.nullable", "nullness:locking.nullable"}) - //{"dereference of possibly-null reference historicalLog,buffers","synchronizing over a possibly-null lock (buffers)"} ArrowBuf newArrowBuf(final long length, final @Nullable BufferManager manager) { allocator.assertOpen(); @@ -276,13 +260,17 @@ ArrowBuf newArrowBuf(final long length, final @Nullable BufferManager manager) { final ArrowBuf buf = new ArrowBuf(this, manager, length, startAddress); // logging - if (BaseAllocator.DEBUG) { + return loggingArrowBufHistoricalLog(buf); + } + + private ArrowBuf loggingArrowBufHistoricalLog(ArrowBuf buf) { + if (historicalLog != null) { historicalLog.recordEvent( "ArrowBuf(BufferLedger, BufferAllocator[%s], " + "UnsafeDirectLittleEndian[identityHashCode == " + "%d](%s)) => ledger hc == %d", allocator.getName(), System.identityHashCode(buf), buf.toString(), System.identityHashCode(this)); - + CheckerFrameworkUtil.assertMethod(buffers != null); synchronized (buffers) { buffers.put(buf, null); } @@ -489,8 +477,6 @@ public long getAccountedSize() { * @param indent The level of indentation to position the data. * @param verbosity The level of verbosity to print. */ - @SuppressWarnings({"nullness:dereference.of.nullable", "nullness:locking.nullable"}) - //{"dereference of possibly-null reference buffers", "synchronizing over a possibly-null lock (buffers)"} void print(StringBuilder sb, int indent, BaseAllocator.Verbosity verbosity) { CommonUtil.indent(sb, indent) .append("ledger[") @@ -511,6 +497,7 @@ void print(StringBuilder sb, int indent, BaseAllocator.Verbosity verbosity) { if (!BaseAllocator.DEBUG) { sb.append("]\n"); } else { + CheckerFrameworkUtil.assertMethod(buffers != null); synchronized (buffers) { sb.append("] holds ") .append(buffers.size()) diff --git a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/CheckerFrameworkUtil.java b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/CheckerFrameworkUtil.java new file mode 100644 index 00000000000..cc1655a46ac --- /dev/null +++ b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/CheckerFrameworkUtil.java @@ -0,0 +1,39 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.arrow.memory.util; + +import org.checkerframework.dataflow.qual.AssertMethod; + +/** + * Utility methods for handling global configuration into CheckerFramework library. + */ +public class CheckerFrameworkUtil { + + /** + * Method annotation @AssertMethod indicates that a method checks a value and possibly + * throws an assertion. Using it can make flow-sensitive type refinement more effective. + * + * @param eval The assertion to be evaluated at compile time + */ + @AssertMethod + public static void assertMethod(boolean eval) { + if (!eval) { + throw new NullPointerException(); + } + } +} diff --git a/java/pom.xml b/java/pom.xml index b19f3f6f910..4366f35683b 100644 --- a/java/pom.xml +++ b/java/pom.xml @@ -48,7 +48,7 @@ 3.11.0 5.5.0 5.2.0 - 3.41.0 + 3.42.0 From 1a8edf51984b606663af9df67b6fb16e0e21ed97 Mon Sep 17 00:00:00 2001 From: david dali susanibar arce Date: Mon, 8 Jan 2024 12:18:14 -0500 Subject: [PATCH 16/18] fix: clean code of AssestionError, reuse annotations logic --- .../org/apache/arrow/memory/Accountant.java | 5 ++- .../arrow/memory/AllocationManager.java | 5 ++- .../org/apache/arrow/memory/BufferLedger.java | 9 +++-- .../memory/util/CheckerFrameworkUtil.java | 39 ------------------- 4 files changed, 12 insertions(+), 46 deletions(-) delete mode 100644 java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/CheckerFrameworkUtil.java diff --git a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/Accountant.java b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/Accountant.java index becefc4f112..3e2e2a073e7 100644 --- a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/Accountant.java +++ b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/Accountant.java @@ -21,7 +21,6 @@ import javax.annotation.concurrent.ThreadSafe; -import org.apache.arrow.memory.util.CheckerFrameworkUtil; import org.apache.arrow.util.Preconditions; import org.checkerframework.checker.nullness.qual.Nullable; @@ -75,7 +74,9 @@ public Accountant(@Nullable Accountant parent, String name, long reservation, lo this.allocationLimit.set(maxAllocation); if (reservation != 0) { - CheckerFrameworkUtil.assertMethod(parent != null); + if (parent == null) { + throw new IllegalArgumentException(String.valueOf("Accountant(parent) must not be null")); + } // we will allocate a reservation from our parent. final AllocationOutcome outcome = parent.allocateBytes(reservation); if (!outcome.isOk()) { diff --git a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/AllocationManager.java b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/AllocationManager.java index 4a604d0b291..a3c28970510 100644 --- a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/AllocationManager.java +++ b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/AllocationManager.java @@ -17,7 +17,6 @@ package org.apache.arrow.memory; -import org.apache.arrow.memory.util.CheckerFrameworkUtil; import org.apache.arrow.util.Preconditions; import org.checkerframework.checker.nullness.qual.Nullable; @@ -138,7 +137,9 @@ void release(final BufferLedger ledger) { Preconditions.checkState(map.containsKey(allocator), "Expecting a mapping for allocator and reference manager"); final BufferLedger oldLedger = map.remove(allocator); - CheckerFrameworkUtil.assertMethod(oldLedger != null); + if (oldLedger == null) { + throw new IllegalArgumentException(String.valueOf("BufferLedger(oldLedger) must not be null")); + } BufferAllocator oldAllocator = oldLedger.getAllocator(); if (oldAllocator instanceof BaseAllocator) { // needed for debug only: tell the allocator that AllocationManager is removing a diff --git a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/BufferLedger.java b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/BufferLedger.java index 56ab33abc84..0b5944e26cf 100644 --- a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/BufferLedger.java +++ b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/BufferLedger.java @@ -21,7 +21,6 @@ import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicLong; -import org.apache.arrow.memory.util.CheckerFrameworkUtil; import org.apache.arrow.memory.util.CommonUtil; import org.apache.arrow.memory.util.HistoricalLog; import org.apache.arrow.util.Preconditions; @@ -270,7 +269,9 @@ private ArrowBuf loggingArrowBufHistoricalLog(ArrowBuf buf) { "UnsafeDirectLittleEndian[identityHashCode == " + "%d](%s)) => ledger hc == %d", allocator.getName(), System.identityHashCode(buf), buf.toString(), System.identityHashCode(this)); - CheckerFrameworkUtil.assertMethod(buffers != null); + if (buffers == null) { + throw new IllegalArgumentException(String.valueOf("IdentityHashMap of buffers must not be null")); + } synchronized (buffers) { buffers.put(buf, null); } @@ -497,7 +498,9 @@ void print(StringBuilder sb, int indent, BaseAllocator.Verbosity verbosity) { if (!BaseAllocator.DEBUG) { sb.append("]\n"); } else { - CheckerFrameworkUtil.assertMethod(buffers != null); + if (buffers == null) { + throw new IllegalArgumentException(String.valueOf("buffers must not be null")); + } synchronized (buffers) { sb.append("] holds ") .append(buffers.size()) diff --git a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/CheckerFrameworkUtil.java b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/CheckerFrameworkUtil.java deleted file mode 100644 index cc1655a46ac..00000000000 --- a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/CheckerFrameworkUtil.java +++ /dev/null @@ -1,39 +0,0 @@ -/* - * Licensed to the Apache Software Foundation (ASF) under one or more - * contributor license agreements. See the NOTICE file distributed with - * this work for additional information regarding copyright ownership. - * The ASF licenses this file to You under the Apache License, Version 2.0 - * (the "License"); you may not use this file except in compliance with - * the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package org.apache.arrow.memory.util; - -import org.checkerframework.dataflow.qual.AssertMethod; - -/** - * Utility methods for handling global configuration into CheckerFramework library. - */ -public class CheckerFrameworkUtil { - - /** - * Method annotation @AssertMethod indicates that a method checks a value and possibly - * throws an assertion. Using it can make flow-sensitive type refinement more effective. - * - * @param eval The assertion to be evaluated at compile time - */ - @AssertMethod - public static void assertMethod(boolean eval) { - if (!eval) { - throw new NullPointerException(); - } - } -} From 4e011c05c22024e535e74ac26c1ca9664949b1ff Mon Sep 17 00:00:00 2001 From: david dali susanibar arce Date: Mon, 8 Jan 2024 14:21:14 -0500 Subject: [PATCH 17/18] fix: add checkerframewrok inside custom preconditions --- .../src/main/java/org/apache/arrow/memory/Accountant.java | 4 +--- .../java/org/apache/arrow/memory/AllocationManager.java | 4 +--- .../main/java/org/apache/arrow/memory/BaseAllocator.java | 3 ++- .../main/java/org/apache/arrow/memory/BufferLedger.java | 8 ++------ .../main/java/org/apache/arrow/util/Preconditions.java | 6 ++++++ 5 files changed, 12 insertions(+), 13 deletions(-) diff --git a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/Accountant.java b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/Accountant.java index 3e2e2a073e7..b87f1345a51 100644 --- a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/Accountant.java +++ b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/Accountant.java @@ -74,9 +74,7 @@ public Accountant(@Nullable Accountant parent, String name, long reservation, lo this.allocationLimit.set(maxAllocation); if (reservation != 0) { - if (parent == null) { - throw new IllegalArgumentException(String.valueOf("Accountant(parent) must not be null")); - } + Preconditions.checkArgument(parent != null, "parent must not be null"); // we will allocate a reservation from our parent. final AllocationOutcome outcome = parent.allocateBytes(reservation); if (!outcome.isOk()) { diff --git a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/AllocationManager.java b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/AllocationManager.java index a3c28970510..6ccefdd9c16 100644 --- a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/AllocationManager.java +++ b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/AllocationManager.java @@ -137,9 +137,7 @@ void release(final BufferLedger ledger) { Preconditions.checkState(map.containsKey(allocator), "Expecting a mapping for allocator and reference manager"); final BufferLedger oldLedger = map.remove(allocator); - if (oldLedger == null) { - throw new IllegalArgumentException(String.valueOf("BufferLedger(oldLedger) must not be null")); - } + Preconditions.checkState(oldLedger != null, "Expecting a mapping for allocator and reference manager"); BufferAllocator oldAllocator = oldLedger.getAllocator(); if (oldAllocator instanceof BaseAllocator) { // needed for debug only: tell the allocator that AllocationManager is removing a diff --git a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/BaseAllocator.java b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/BaseAllocator.java index 9453a43ac2a..bea660187a3 100644 --- a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/BaseAllocator.java +++ b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/BaseAllocator.java @@ -209,7 +209,8 @@ void dissociateLedger(BufferLedger ledger) { assertOpen(); if (DEBUG) { synchronized (DEBUG_LOCK) { - if (!(childLedgers != null && childLedgers.containsKey(ledger))) { + Preconditions.checkState(childLedgers != null, "childLedgers must not be null"); + if (!childLedgers.containsKey(ledger)) { throw new IllegalStateException("Trying to remove a child ledger that doesn't exist."); } childLedgers.remove(ledger); diff --git a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/BufferLedger.java b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/BufferLedger.java index 0b5944e26cf..1ca3e08ecf0 100644 --- a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/BufferLedger.java +++ b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/BufferLedger.java @@ -269,9 +269,7 @@ private ArrowBuf loggingArrowBufHistoricalLog(ArrowBuf buf) { "UnsafeDirectLittleEndian[identityHashCode == " + "%d](%s)) => ledger hc == %d", allocator.getName(), System.identityHashCode(buf), buf.toString(), System.identityHashCode(this)); - if (buffers == null) { - throw new IllegalArgumentException(String.valueOf("IdentityHashMap of buffers must not be null")); - } + Preconditions.checkState(buffers != null, "IdentityHashMap of buffers must not be null"); synchronized (buffers) { buffers.put(buf, null); } @@ -498,9 +496,7 @@ void print(StringBuilder sb, int indent, BaseAllocator.Verbosity verbosity) { if (!BaseAllocator.DEBUG) { sb.append("]\n"); } else { - if (buffers == null) { - throw new IllegalArgumentException(String.valueOf("buffers must not be null")); - } + Preconditions.checkArgument(buffers != null, "IdentityHashMap of buffers must not be null"); synchronized (buffers) { sb.append("] holds ") .append(buffers.size()) diff --git a/java/memory/memory-core/src/main/java/org/apache/arrow/util/Preconditions.java b/java/memory/memory-core/src/main/java/org/apache/arrow/util/Preconditions.java index 0ffc9447e43..8083033007d 100644 --- a/java/memory/memory-core/src/main/java/org/apache/arrow/util/Preconditions.java +++ b/java/memory/memory-core/src/main/java/org/apache/arrow/util/Preconditions.java @@ -33,6 +33,8 @@ package org.apache.arrow.util; +import org.checkerframework.dataflow.qual.AssertMethod; + /** * Static convenience methods that help a method or constructor check whether it was invoked * correctly (whether its preconditions have been met). These methods generally accept a @@ -117,6 +119,7 @@ private Preconditions() {} * @param expression a boolean expression * @throws IllegalArgumentException if {@code expression} is false */ + @AssertMethod public static void checkArgument(boolean expression) { if (!expression) { throw new IllegalArgumentException(); @@ -131,6 +134,7 @@ public static void checkArgument(boolean expression) { * string using {@link String#valueOf(Object)} * @throws IllegalArgumentException if {@code expression} is false */ + @AssertMethod public static void checkArgument(boolean expression, Object errorMessage) { if (!expression) { throw new IllegalArgumentException(String.valueOf(errorMessage)); @@ -438,6 +442,7 @@ public static void checkArgument( * @param expression a boolean expression * @throws IllegalStateException if {@code expression} is false */ + @AssertMethod public static void checkState(boolean expression) { if (!expression) { throw new IllegalStateException(); @@ -453,6 +458,7 @@ public static void checkState(boolean expression) { * string using {@link String#valueOf(Object)} * @throws IllegalStateException if {@code expression} is false */ + @AssertMethod public static void checkState(boolean expression, Object errorMessage) { if (!expression) { throw new IllegalStateException(String.valueOf(errorMessage)); From f92f3761afb7193ce65c26a62f630c89301ae38a Mon Sep 17 00:00:00 2001 From: david dali susanibar arce Date: Mon, 8 Jan 2024 15:28:48 -0500 Subject: [PATCH 18/18] fix: addressing PR comments --- .../src/main/java/org/apache/arrow/memory/BaseAllocator.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/BaseAllocator.java b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/BaseAllocator.java index bea660187a3..8779c7a3434 100644 --- a/java/memory/memory-core/src/main/java/org/apache/arrow/memory/BaseAllocator.java +++ b/java/memory/memory-core/src/main/java/org/apache/arrow/memory/BaseAllocator.java @@ -1008,7 +1008,7 @@ private ArrowBuf allocate(int nBytes) { final ArrowBuf arrowBuf = BaseAllocator.this.bufferWithoutReservation(nBytes, null); listener.onAllocation(nBytes); - if (DEBUG && historicalLog != null) { + if (historicalLog != null) { historicalLog.recordEvent("allocate() => %s", String.format("ArrowBuf[%d]", arrowBuf .getId())); } @@ -1031,7 +1031,7 @@ private void releaseReservation(int nBytes) { releaseBytes(nBytes); - if (DEBUG && historicalLog != null) { + if (historicalLog != null) { historicalLog.recordEvent("releaseReservation(%d)", nBytes); } }