lidavidm commented on code in PR #37723:
URL: https://github.com/apache/arrow/pull/37723#discussion_r1435105272
##########
java/memory/memory-core/src/main/java/org/apache/arrow/memory/BaseAllocator.java:
##########
@@ -64,17 +68,17 @@ 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 Object DEBUG_LOCK = new Object();
private final AllocationListener listener;
- private final BaseAllocator parentAllocator;
+ private final @Nullable BaseAllocator parentAllocator;
private final Map<BaseAllocator, Object> childAllocators;
private final ArrowBuf empty;
// members used purely for debugging
- private final IdentityHashMap<BufferLedger, Object> childLedgers;
- private final IdentityHashMap<Reservation, Object> reservations;
- private final HistoricalLog historicalLog;
+ private final @Nullable IdentityHashMap<BufferLedger, @Nullable Object>
childLedgers;
+ private final @Nullable IdentityHashMap<Reservation, Object> reservations;
+ private final @Nullable HistoricalLog historicalLog;
private final RoundingPolicy roundingPolicy;
- private final AllocationManager.Factory allocationManagerFactory;
+ private final AllocationManager.@NonNull Factory allocationManagerFactory;
Review Comment:
When we have a bunch of fields like this that are all tied to DEBUG, I
wonder if we should instead create a new private static inner class to hold all
the fields. Then we only need a single null check.
##########
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:
Checker framework apparently understands `assert`, can we use that here
instead?
##########
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:
Can't you avoid this by having `if (buffers != null)`?
##########
java/memory/memory-core/pom.xml:
##########
@@ -90,5 +94,46 @@
</plugins>
</build>
</profile>
+
+ <profile>
+ <id>checkerframework-jdk11+</id>
Review Comment:
@davisusanibar can you file a follow up isuse?
##########
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:
Otherwise for stuff like this I would prefer early return to avoid indenting
so much
```
if (oldLedger == null) {
// Should not happen, but satisfy Checker Framework here
return;
}
// continue...
```
##########
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:
It's supported as of 3 weeks ago
https://github.com/typetools/checker-framework/pull/6311
@davisusanibar can you file a follow up issue?
##########
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:
It seems either copyOfRange isn't preserving a NonNull or getStackTrace
isn't annotated properly, but both of those are out of our control
##########
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:
That said the condition here is a little too complex, so I don't think it
will fix this. (Maybe it's smart and can unpack the assertion above, though!)
##########
java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/hash/SimpleHasher.java:
##########
@@ -110,7 +111,7 @@ public int hashCode() {
}
@Override
- public boolean equals(Object obj) {
+ public boolean equals(@Nullable Object obj) {
Review Comment:
equals is defined to work on null.
also see https://github.com/jspecify/jspecify/issues/70
--
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]
