danepitkin commented on code in PR #37723:
URL: https://github.com/apache/arrow/pull/37723#discussion_r1433019326
##########
java/memory/memory-core/src/main/java/org/apache/arrow/memory/BufferLedger.java:
##########
@@ -122,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) {
+ if (BaseAllocator.DEBUG && historicalLog != null) {
Review Comment:
nit: We can remove BaseAllocator.DEBUG from `if (BaseAllocator.DEBUG &&
historicalLog != null)` everywhere since it is already used to initialize the
HistoricalLog. What do you think?
##########
java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/StackTrace.java:
##########
@@ -19,12 +19,15 @@
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;
Review Comment:
Is this actually nullable? It is assigned as `final` in the constructor.
##########
java/memory/memory-core/src/main/java/org/apache/arrow/memory/BaseAllocator.java:
##########
@@ -928,7 +976,7 @@ public boolean reserve(int nBytes) {
final AllocationOutcome outcome =
BaseAllocator.this.allocateBytes(nBytes);
- if (DEBUG) {
+ if (DEBUG && historicalLog != null) {
Review Comment:
We can get rid of `DEBUG` in these `if` statement since it's used in
constructing the historicalLog.
##########
java/memory/memory-core/src/main/java/org/apache/arrow/memory/ArrowBuf.java:
##########
@@ -93,7 +94,7 @@ public ArrowBuf(
this.capacity = capacity;
this.readerIndex = 0;
this.writerIndex = 0;
- if (BaseAllocator.DEBUG) {
+ if (BaseAllocator.DEBUG && historicalLog != null) {
Review Comment:
We can remove BaseAllocator.DEBUG here.
##########
java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/MemoryUtil.java:
##########
@@ -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;
Review Comment:
I didn't see anything at first glance besides this
https://checkerframework.org/api/org/checkerframework/checker/initialization/InitializationChecker.html
##########
java/memory/memory-core/src/main/java/org/apache/arrow/memory/LowCostIdentityHashMap.java:
##########
@@ -95,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<K, V> 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<K, V> this, int s) {
Review Comment:
nit: name it `newElementArrayUnderInitialized` to match the above API name
##########
java/memory/memory-core/src/main/java/org/apache/arrow/memory/ArrowBuf.java:
##########
@@ -1110,8 +1113,10 @@ public void print(StringBuilder sb, int indent,
Verbosity verbosity) {
CommonUtil.indent(sb, indent).append(toString());
if (BaseAllocator.DEBUG && verbosity.includeHistoricalLog) {
- sb.append("\n");
- historicalLog.buildHistory(sb, indent + 1, verbosity.includeStackTraces);
+ if (historicalLog != null) {
Review Comment:
Replace BaseAllocator.DEBUG with `historicalLog != null` here too.
##########
java/memory/memory-core/src/main/java/org/apache/arrow/memory/Accountant.java:
##########
@@ -72,13 +73,13 @@ public Accountant(Accountant parent, String name, long
reservation, long maxAllo
this.reservation = reservation;
this.allocationLimit.set(maxAllocation);
- if (reservation != 0) {
+ if (reservation != 0 && parent != null) {
Review Comment:
Parent is not allowed to be nullable, can we throw an exception if null is
passed in for this argument? See
https://github.com/apache/arrow/pull/37723#discussion_r1374891623
This means the `protected final Accountant parent;` should not be nullable.
##########
java/memory/memory-core/src/main/java/org/apache/arrow/memory/DefaultAllocationManagerOption.java:
##########
@@ -61,6 +64,7 @@ public enum AllocationManagerType {
Unknown,
}
+ @SuppressWarnings("nullness:argument") //enum types valueOf are implicitly
non-null
Review Comment:
Could we mark these as `@NonNull` instead of suppressing the warning by
chance?
##########
java/memory/memory-core/src/main/java/org/apache/arrow/memory/BufferLedger.java:
##########
@@ -204,6 +205,8 @@ public void retain(int increment) {
* @return derived buffer
*/
@Override
+ @SuppressWarnings({"nullness:dereference.of.nullable",
"nullness:locking.nullable"})
Review Comment:
We can remove the suppression for `nullness:dereference.of.nullable` by
checking for `historicalLog != null` in place of BaseAllocator.DEBUG below.
##########
java/memory/memory-core/src/main/java/org/apache/arrow/memory/AllocationManager.java:
##########
@@ -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;
Review Comment:
Is this nullable because its volatile? It does't look like it should be
nullable based on its usage. maybe we just need to add a null check in
`associate()` to ensure an exception is thrown if `ledger = new
BufferLedger(allocator, this);` returns null:
https://github.com/apache/arrow/pull/37723/files#diff-19d2b0e7588c427fe1b1bdf979a1e1149f94206d72bfba933cd9748681915095R103
##########
java/memory/memory-core/src/main/java/org/apache/arrow/memory/BufferLedger.java:
##########
@@ -333,45 +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.
*/
- boolean transferBalance(final 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.");
+ boolean transferBalance(final @Nullable ReferenceManager
targetReferenceManager) {
+ if (targetReferenceManager != null) {
Review Comment:
nit: can you change this for better code readability to this
```
if (targetReferenceManager != null) {
throw new exception..
}
...
```
##########
java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/MemoryUtil.java:
##########
@@ -63,6 +66,9 @@ public class MemoryUtil {
// try to get the unsafe object
final Object maybeUnsafe = AccessController.doPrivileged(new
PrivilegedAction<Object>() {
@Override
+ @SuppressWarnings({"nullness:argument", "nullness:return"})
Review Comment:
Could we remove the warning suppression for `nullness:return` if we mark the
return type as nullable and/or cast the return value?
##########
java/memory/memory-core/src/main/java/org/apache/arrow/memory/LowCostIdentityHashMap.java:
##########
@@ -35,7 +38,7 @@ public class LowCostIdentityHashMap<K, V extends
ValueWithKeyIncluded<K>> {
/*
* The internal data structure to hold values.
*/
- private Object[] elementData;
+ private @Nullable Object [] elementData; // elementData[index] = null;
Review Comment:
Technically this is not nullable, since the constructor will throw an
exception if elementData is not initialized. Should we change it?
##########
java/memory/memory-core/src/main/java/org/apache/arrow/memory/ArrowBuf.java:
##########
@@ -314,7 +315,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) {
Review Comment:
We can remove BaseAllocator.DEBUG check from above this line.
##########
java/memory/memory-core/pom.xml:
##########
@@ -90,5 +94,46 @@
</plugins>
</build>
</profile>
+
+ <profile>
+ <id>checkerframework-jdk11+</id>
Review Comment:
Would it be possible to add this profile to the top-level java/pom.xml, but
only enable it for a list of modules to make future development easier?
##########
java/memory/memory-core/src/main/java/org/apache/arrow/memory/AllocationManager.java:
##########
@@ -136,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 <allocator, reference manager> 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 <allocator,
reference manager>
- // 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) {
Review Comment:
The logic has been incorrectly changed here. The `else` statement no longer
applies to this if-statement like before `if (oldLedger == owningLedger)`.
Also, can we remove the `oldLedger != null` check if there is already a
Preconditions check? If not, I think it would be better to just have another
Preconditions check:
```
Preconditions.checkState(oldLedger != null,
"Expecting a valid BufferLedger, but received null instead");
```
##########
java/memory/memory-core/src/main/java/org/apache/arrow/memory/BufferLedger.java:
##########
@@ -481,6 +489,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"})
Review Comment:
We can remove suppression for the first warning by checking for
`historicalLog != null` instead of BaseAllocator.DEBUG.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
