lidavidm commented on code in PR #37723:
URL: https://github.com/apache/arrow/pull/37723#discussion_r1445089336
##########
java/memory/memory-core/src/main/java/org/apache/arrow/memory/Accountant.java:
##########
@@ -73,12 +74,15 @@ public Accountant(Accountant parent, String name, long
reservation, long maxAllo
this.allocationLimit.set(maxAllocation);
if (reservation != 0) {
+ if (parent == null) {
+ throw new IllegalArgumentException(String.valueOf("Accountant(parent)
must not be null"));
+ }
Review Comment:
Why do you use String.valueOf?
```suggestion
Preconditions.checkArgument(parent != null, "parent must not be null");
```
##########
java/memory/memory-core/src/main/java/org/apache/arrow/memory/BaseAllocator.java:
##########
@@ -201,7 +209,7 @@ void dissociateLedger(BufferLedger ledger) {
assertOpen();
if (DEBUG) {
synchronized (DEBUG_LOCK) {
- if (!childLedgers.containsKey(ledger)) {
+ if (!(childLedgers != null && childLedgers.containsKey(ledger))) {
Review Comment:
This makes more sense as an assertion that childLedgers is notnull
##########
java/memory/memory-core/src/main/java/org/apache/arrow/memory/AllocationManager.java:
##########
@@ -133,9 +135,11 @@ void release(final BufferLedger ledger) {
// remove the <BaseAllocator, BufferLedger> 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);
-
+ if (oldLedger == null) {
+ throw new
IllegalArgumentException(String.valueOf("BufferLedger(oldLedger) must not be
null"));
+ }
Review Comment:
I think this should be written as a single check.
```java
final BufferLedger oldLedger = map.remove(allocator);
Preconditions.checkState(oldLedger != null, "Expecting a mapping for
allocator and reference manager");
```
##########
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:
@davisusanibar can you follow up here? File an issue?
##########
java/memory/memory-core/src/main/java/org/apache/arrow/memory/BufferLedger.java:
##########
@@ -271,13 +259,19 @@ ArrowBuf newArrowBuf(final long length, final
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));
-
+ if (buffers == null) {
+ throw new IllegalArgumentException(String.valueOf("IdentityHashMap of
buffers must not be null"));
+ }
Review Comment:
Use Preconditions.checkState here too
##########
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) {
Review Comment:
I don't see any change.
##########
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:
@davisusanibar can you review this?
--
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]
