Skip to content

Commit

Permalink
fix: addressing PR comments
Browse files Browse the repository at this point in the history
  • Loading branch information
davisusanibar committed Dec 21, 2023
1 parent 4de6d64 commit c7fb710
Show file tree
Hide file tree
Showing 5 changed files with 25 additions and 27 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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 <allocator, reference manager> 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 <allocator, reference manager> mappings
Preconditions.checkState(map.size() > 0,
"The final removal of reference manager should be connected to owning reference manager");
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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()");
}
}
Expand Down Expand Up @@ -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()));
Expand Down Expand Up @@ -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);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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()));
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down Expand Up @@ -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():
Expand Down Expand Up @@ -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 == " +
Expand Down Expand Up @@ -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(),
Expand Down Expand Up @@ -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;
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
Expand Down Expand Up @@ -110,7 +110,7 @@ private Object[] newElementArrayInitialized(@Initialized LowCostIdentityHashMap<
* the number of elements
* @return Reference to the element array
*/
private Object[] newElementArrayUnderInitialization(@UnderInitialization LowCostIdentityHashMap<K, V> this, int s) {
private Object[] newElementArrayUnderInitialized(@UnderInitialization LowCostIdentityHashMap<K, V> this, int s) {
return new Object[s];
}

Expand Down

0 comments on commit c7fb710

Please sign in to comment.