Skip to content
Closed
Show file tree
Hide file tree
Changes from 1 commit
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
Original file line number Diff line number Diff line change
Expand Up @@ -174,9 +174,12 @@ public void reset() {
if (consumer != null) {
consumer.freeArray(array);
// the call to consumer.allocateArray may trigger a spill
// which in turn access this instance and eventually re-enter this method and try to free the array again.
// by setting the array to null and its length to 0 we effectively make the spill code-path a no-op.
// setting the array to null also indicates that it has already been de-allocated which prevents a double de-allocation in free().
// which in turn access this instance and eventually re-enter this method
// and try to free the array again.
// by setting the array to null and its length to 0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

by -> By?

// we effectively make the spill code-path a no-op.
// setting the array to null also indicates that it has already been
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

setting -> Setting?

// de-allocated which prevents a double de-allocation in free().
array = null;
usableCapacity = 0;
pos = 0;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -470,17 +470,20 @@ public void testOOMDuringSpill() throws Exception {
insertNumber(sorter, i);
}
// we expect the next insert to attempt growing the pointerssArray
// first allocation is expected to fail, then a spill is triggered which attempts another allocation
// first allocation is expected to fail, then a spill is
// triggered which attempts another allocation
// which also fails and we expect to see this OOM here.
// the original code messed with a released array within the spill code
// and ended up with a failed assertion.
// we also expect the location of the OOM to be org.apache.spark.util.collection.unsafe.sort.UnsafeInMemorySorter.reset
// we also expect the location of the OOM to be
// org.apache.spark.util.collection.unsafe.sort.UnsafeInMemorySorter.reset
memoryManager.markconsequentOOM(2);
try {
insertNumber(sorter, 1024);
fail("expected OutOfMmoryError but it seems operation surprisingly succeeded");
}
// we expect an OutOfMemoryError here, anything else (i.e the original NPE is a failure)
// we expect an OutOfMemoryError here, anything else
// (i.e the original NPE is a failure)
catch (OutOfMemoryError oom){
String oomStackTrace = Utils.exceptionString(oom);
assertThat("expected OutOfMemoryError in org.apache.spark.util.collection.unsafe.sort.UnsafeInMemorySorter.reset",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -259,7 +259,7 @@ public int compare(
Object baseObj2,
long baseOff2,
int baseLen2) {
// Note that since ordering doesn't need the total length of the record, we just pass -1
// Note that since ordering doesn't need the total length of the record, we just pass -1
// into the row.
row1.pointTo(baseObj1, baseOff1 + 4, -1);
row2.pointTo(baseObj2, baseOff2 + 4, -1);
Expand Down