Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clean up buffers in case AssertionError #13262

Merged
merged 20 commits into from
May 11, 2023
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion java/src/main/java/ai/rapids/cudf/BitVectorHelper.java
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ private static void shiftSrcLeftAndWriteToDst(HostMemoryBuffer src, HostMemoryBu
/**
* This method returns the length in bytes needed to represent X number of rows
* e.g. getValidityLengthInBytes(5) => 1 byte
* getLengthInBytes(7) => 1 byte
* getValidityLengthInBytes(7) => 1 byte
abellina marked this conversation as resolved.
Show resolved Hide resolved
* getValidityLengthInBytes(14) => 2 bytes
*/
static long getValidityLengthInBytes(long rows) {
Expand Down
4 changes: 2 additions & 2 deletions java/src/main/java/ai/rapids/cudf/ColumnVector.java
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ public ColumnVector(DType type, long rows, Optional<Long> nullCount,
incRefCountInternal(true);
}

private static OffHeapState makeOffHeap(DType type, long rows, Optional<Long> nullCount,
static OffHeapState makeOffHeap(DType type, long rows, Optional<Long> nullCount,
abellina marked this conversation as resolved.
Show resolved Hide resolved
DeviceMemoryBuffer dataBuffer, DeviceMemoryBuffer validityBuffer,
DeviceMemoryBuffer offsetBuffer, List<DeviceMemoryBuffer> toClose, long[] childHandles) {
long viewHandle = initViewHandle(type, (int)rows, nullCount.orElse(UNKNOWN_NULL_COUNT).intValue(),
Expand All @@ -141,7 +141,7 @@ private static OffHeapState makeOffHeap(DType type, long rows, Optional<Long> nu
* @param offsetBuffer a host buffer required for strings and string categories. The column
* vector takes ownership of the buffer. Do not use the buffer after calling
* this.
* @param toClose List of buffers to track adn close once done, usually in case of children
* @param toClose List of buffers to track and close once done, usually in case of children
* @param childHandles array of longs for child column view handles.
*/
public ColumnVector(DType type, long rows, Optional<Long> nullCount,
Expand Down
26 changes: 22 additions & 4 deletions java/src/main/java/ai/rapids/cudf/ColumnView.java
Original file line number Diff line number Diff line change
Expand Up @@ -43,32 +43,50 @@ public class ColumnView implements AutoCloseable, BinaryOperable {
protected final ColumnVector.OffHeapState offHeap;

/**
* Constructs a Column View given a native view address
* Constructs a Column View given a native view address. This asserts that if the ColumnView is
* of nested-type it doesn't contain non-empty nulls
* @param address the view handle
* @throws AssertionError if the address points to a nested-type view with non-empty nulls
*/
ColumnView(long address) {
this.viewHandle = address;
this.type = DType.fromNative(ColumnView.getNativeTypeId(viewHandle), ColumnView.getNativeTypeScale(viewHandle));
this.rows = ColumnView.getNativeRowCount(viewHandle);
this.nullCount = ColumnView.getNativeNullCount(viewHandle);
this.offHeap = null;
AssertEmptyNulls.assertNullsAreEmpty(this);
try {
razajafri marked this conversation as resolved.
Show resolved Hide resolved
AssertEmptyNulls.assertNullsAreEmpty(this);
} catch (AssertionError ae) {
// offHeap state is null, so there is nothing to clean in offHeap
// delete ColumnView to avoid memory leak
deleteColumnView(viewHandle);
viewHandle = 0;
throw ae;
}
}


/**
* Intended to be called from ColumnVector when it is being constructed. Because state creates a
* cudf::column_view instance and will close it in all cases, we don't want to have to double
* close it.
* close it. This asserts that if the offHeapState is of nested-type it doesn't contain non-empty nulls
* @param state the state this view is based off of.
* @throws AssertionError if offHeapState points to a nested-type view with non-empty nulls
*/
protected ColumnView(ColumnVector.OffHeapState state) {
offHeap = state;
viewHandle = state.getViewHandle();
type = DType.fromNative(ColumnView.getNativeTypeId(viewHandle), ColumnView.getNativeTypeScale(viewHandle));
rows = ColumnView.getNativeRowCount(viewHandle);
nullCount = ColumnView.getNativeNullCount(viewHandle);
AssertEmptyNulls.assertNullsAreEmpty(this);
try {
AssertEmptyNulls.assertNullsAreEmpty(this);
} catch (AssertionError ae) {
// cleanup offHeap
offHeap.clean(false);
abellina marked this conversation as resolved.
Show resolved Hide resolved
viewHandle = 0;
throw ae;
}
}

/**
Expand Down
31 changes: 30 additions & 1 deletion java/src/test/java/ai/rapids/cudf/ColumnVectorTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -6677,6 +6677,36 @@ void testApplyBooleanMaskFromListOfStructure() {
}
}

@Test
void testColumnViewWithNonEmptyNullsIsCleared() {
abellina marked this conversation as resolved.
Show resolved Hide resolved
List<Integer> list0 = Arrays.asList(1, 2, 3);
List<Integer> list1 = Arrays.asList(4, 5, null);
List<Integer> list2 = Arrays.asList(7, 8, 9);
List<Integer> list3 = null;
try (ColumnVector input = ColumnVectorTest.makeListsColumn(DType.INT32, list0, list1, list2, list3);
BaseDeviceMemoryBuffer baseValidityBuffer = input.getDeviceBufferFor(BufferType.VALIDITY);
BaseDeviceMemoryBuffer baseOffsetBuffer = input.getDeviceBufferFor(BufferType.OFFSET);
HostMemoryBuffer newValidity = HostMemoryBuffer.allocate(BitVectorHelper.getValidityAllocationSizeInBytes(3))) {

newValidity.copyFromDeviceBuffer(baseValidityBuffer);
BitVectorHelper.setNullAt(newValidity, 1);
DeviceMemoryBuffer validityBuffer = DeviceMemoryBuffer.allocate(BitVectorHelper.getValidityAllocationSizeInBytes(3));
validityBuffer.copyFromHostBuffer(newValidity);
DeviceMemoryBuffer offsetBuffer = DeviceMemoryBuffer.allocate(baseOffsetBuffer.getLength());
offsetBuffer.copyFromMemoryBuffer(0, baseOffsetBuffer, 0,
baseOffsetBuffer.length, Cuda.DEFAULT_STREAM);

ColumnVector.OffHeapState offHeapState = ColumnVector.makeOffHeap(input.type, input.rows, Optional.of(2L),
null, validityBuffer, offsetBuffer,
null, Arrays.stream(input.getChildColumnViews()).mapToLong((c) -> c.viewHandle).toArray());
try {
new ColumnView(offHeapState);
} catch (AssertionError ae) {
assert offHeapState.isClean();
}
}
}

@Test
public void testEventHandlerIsCalledForEachClose() {
final AtomicInteger onClosedWasCalled = new AtomicInteger(0);
Expand All @@ -6700,5 +6730,4 @@ public void testEventHandlerIsNotCalledIfNotSet() {
}
assertEquals(0, onClosedWasCalled.get());
}

}