Skip to content

Commit

Permalink
fix: addressing reviews v5
Browse files Browse the repository at this point in the history
  • Loading branch information
vibhatha committed Sep 3, 2024
1 parent 3986552 commit 782f923
Show file tree
Hide file tree
Showing 6 changed files with 21 additions and 56 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ public interface AllocationReservation extends AutoCloseable {
* @throws IllegalStateException if called after buffer() is used to allocate the reservation
* @deprecated use {@link #add(long)} instead
*/
@Deprecated
@Deprecated(forRemoval = true)
boolean add(int nBytes);

/**
Expand All @@ -59,7 +59,7 @@ public interface AllocationReservation extends AutoCloseable {
* @return true if the reservation can be satisfied, false otherwise
* @deprecated use {@link #reserve(long)} instead
*/
@Deprecated
@Deprecated(forRemoval = true)
boolean reserve(int nBytes);

/**
Expand All @@ -84,8 +84,7 @@ public interface AllocationReservation extends AutoCloseable {
ArrowBuf allocateBuffer();

/**
* Get the current size of the reservation (the sum of all the add()s). Int version is deprecated,
* use getLongSize instead.
* Get the current size of the reservation (the sum of all the add()s).
*
* @return size of the current reservation
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import org.apache.arrow.memory.util.AssertionUtil;
import org.apache.arrow.memory.util.CommonUtil;
import org.apache.arrow.memory.util.HistoricalLog;
import org.apache.arrow.memory.util.LargeMemoryUtil;
import org.apache.arrow.util.Preconditions;
import org.checkerframework.checker.initialization.qual.Initialized;
import org.checkerframework.checker.nullness.qual.KeyFor;
Expand Down Expand Up @@ -888,36 +889,14 @@ public Reservation() {
}
}

@Deprecated
@Deprecated(forRemoval = true)
@Override
public boolean add(final int nBytes) {
assertOpen();

Preconditions.checkArgument(nBytes >= 0, "nBytes(%d) < 0", nBytes);
Preconditions.checkState(
!closed, "Attempt to increase reservation after reservation has been closed");
Preconditions.checkState(
!used, "Attempt to increase reservation after reservation has been used");

// we round up to next power of two since all reservations are done in powers of two. This
// may overestimate the
// preallocation since someone may perceive additions to be power of two. If this becomes a
// problem, we can look
// at
// modifying this behavior so that we maintain what we reserve and what the user asked for
// and make sure to only
// round to power of two as necessary.
final int nBytesTwo = CommonUtil.nextPowerOfTwo(nBytes);
if (!reserve(nBytesTwo)) {
return false;
}

this.nBytes += nBytesTwo;
return true;
return add((long) nBytes);
}

@Override
public boolean add(long nBytes) {
public boolean add(final long nBytes) {
assertOpen();

Preconditions.checkArgument(nBytes >= 0, "nBytes(%d) < 0", nBytes);
Expand Down Expand Up @@ -957,7 +936,7 @@ public ArrowBuf allocateBuffer() {

@Override
public int getSize() {
return (int) nBytes;
return LargeMemoryUtil.checkedCastToInt(nBytes);
}

@Override
Expand Down Expand Up @@ -1009,15 +988,7 @@ public void close() {
@Deprecated
@Override
public boolean reserve(int nBytes) {
assertOpen();

final AllocationOutcome outcome = BaseAllocator.this.allocateBytes(nBytes);

if (historicalLog != null) {
historicalLog.recordEvent("reserve(%d) => %s", nBytes, Boolean.toString(outcome.isOk()));
}

return outcome.isOk();
return reserve((long) nBytes);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ public class DefaultRoundingPolicy implements RoundingPolicy {
defaultPageSize = 8192;
}

long defaultMaxOrder = Long.getLong("org.apache.memory.allocator.maxOrder", 11);
int defaultMaxOrder = Integer.getInteger("org.apache.memory.allocator.maxOrder", 11);
try {
validateAndCalculateChunkSize(defaultPageSize, defaultMaxOrder);
} catch (Throwable t) {
Expand All @@ -74,7 +74,7 @@ private static long validateAndCalculatePageShifts(long pageSize) {
return Long.SIZE - 1 - Long.numberOfLeadingZeros(pageSize);
}

private static long validateAndCalculateChunkSize(long pageSize, long maxOrder) {
private static long validateAndCalculateChunkSize(long pageSize, int maxOrder) {
if (maxOrder > 14) {
throw new IllegalArgumentException("maxOrder: " + maxOrder + " (expected: 0-14)");
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
*/
package org.apache.arrow.memory.rounding;

import org.apache.arrow.memory.util.LargeMemoryUtil;
import org.apache.arrow.util.Preconditions;

/** The rounding policy that each buffer size must a multiple of the segment size. */
Expand All @@ -38,15 +39,9 @@ public class SegmentRoundingPolicy implements RoundingPolicy {
* SegmentRoundingPolicy#MIN_SEGMENT_SIZE}, or is not a power of 2.
* @deprecated use {@link SegmentRoundingPolicy#SegmentRoundingPolicy(long)} instead.
*/
@Deprecated
@Deprecated(forRemoval = true)
public SegmentRoundingPolicy(int segmentSize) {
Preconditions.checkArgument(
segmentSize >= MIN_SEGMENT_SIZE,
"The segment size cannot be smaller than %s",
MIN_SEGMENT_SIZE);
Preconditions.checkArgument(
(segmentSize & (segmentSize - 1)) == 0, "The segment size must be a power of 2");
this.segmentSize = segmentSize;
this((long) segmentSize);
}

/**
Expand All @@ -71,7 +66,11 @@ public long getRoundedSize(long requestSize) {
return (requestSize + (segmentSize - 1)) / segmentSize * segmentSize;
}

public long getSegmentSize() {
public int getSegmentSize() {
return LargeMemoryUtil.checkedCastToInt(segmentSize);
}

public long getSegmentSizeAsLong() {
return segmentSize;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -52,11 +52,7 @@ public class NettyArrowBuf extends AbstractByteBuf implements AutoCloseable {
@Deprecated
public NettyArrowBuf(
final ArrowBuf arrowBuf, final BufferAllocator bufferAllocator, final int length) {
super(length);
this.arrowBuf = arrowBuf;
this.arrowByteBufAllocator = new ArrowByteBufAllocator(bufferAllocator);
this.length = length;
this.address = arrowBuf.memoryAddress();
this(arrowBuf, bufferAllocator, (long) length);
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ public void defaultAllocatorBenchmark() {
@BenchmarkMode(Mode.AverageTime)
@OutputTimeUnit(TimeUnit.MICROSECONDS)
public void segmentRoundingPolicyBenchmark() {
final int bufferSize = 1024;
final long bufferSize = 1024L;
final int numBuffers = 1024;
final long segmentSize = 1024L;

Expand Down

0 comments on commit 782f923

Please sign in to comment.