Skip to content
Prev Previous commit
Next Next commit
improve OOM handling
  • Loading branch information
Davies Liu committed Oct 30, 2015
commit 4ee1f42b08b46469d8631a56b01898d251ebf3e7
Original file line number Diff line number Diff line change
Expand Up @@ -320,22 +320,26 @@ public void cleanupResources() {
private void growPointerArrayIfNecessary() throws IOException {
assert(inMemSorter != null);
if (!inMemSorter.hasSpaceForAnotherRecord()) {
logger.debug("Attempting to expand sort pointer array");
long used = inMemSorter.getMemoryUsage();
long needed = inMemSorter.getMemoryToExpand();
long needed = used + inMemSorter.getMemoryToExpand();
try {
acquireMemory(used + needed); // could trigger spilling
if (inMemSorter.hasSpaceForAnotherRecord()) {
releaseMemory(used + needed);
} else {
logger.debug("Expand sort pointer array");
acquireMemory(needed); // could trigger spilling
} catch (OutOfMemoryError e) {
// should have trigger spilling
assert(inMemSorter.hasSpaceForAnotherRecord());
return;
}
// check if spilling is triggered or not
if (inMemSorter.hasSpaceForAnotherRecord()) {
releaseMemory(needed);
} else {
try {
inMemSorter.expandPointerArray();
releaseMemory(used);
}
} catch (OutOfMemoryError oom) {
// spilling should be triggered
if (!inMemSorter.hasSpaceForAnotherRecord()) {
spill(); // Just in case that JVM had run out of memory
} catch (OutOfMemoryError oom) {
// Just in case that JVM had run out of memory
releaseMemory(needed);
spill();
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,7 @@ public void reset() {

private int newLength() {
// Guard against overflow:
return array.length <= Integer.MAX_VALUE / 2 ?
(array.length * 2) : Integer.MAX_VALUE;
return array.length <= Integer.MAX_VALUE / 2 ? (array.length * 2) : Integer.MAX_VALUE;
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -710,13 +710,13 @@ public boolean putNewKey(Object keyBase, long keyOffset, int keyLength,
private boolean acquireNewPage(long required) {
try {
currentPage = allocatePage(required);
dataPages.add(currentPage);
Platform.putInt(currentPage.getBaseObject(), currentPage.getBaseOffset(), 0);
pageCursor = 4;
return true;
} catch (OutOfMemoryError e) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If we want to catch OOME here, I think that we should do it at a much smaller scope (in the assignment to currentPage but not for adding to dataPages or modifying the page cursor. Given the risks of catching OOME that I mentioned above, the scope of the catch should be as narrow as possible.

return false;
}
dataPages.add(currentPage);
Platform.putInt(currentPage.getBaseObject(), currentPage.getBaseOffset(), 0);
pageCursor = 4;
return true;
}

@Override
Expand Down Expand Up @@ -862,7 +862,13 @@ void growAndRehash() {
final int oldCapacity = (int) oldBitSet.capacity();

// Allocate the new data structures
allocate(Math.min(growthStrategy.nextCapacity(oldCapacity), MAX_CAPACITY));
try {
allocate(Math.min(growthStrategy.nextCapacity(oldCapacity), MAX_CAPACITY));
} catch (OutOfMemoryError oom) {
longArray = oldLongArray;
bitset = oldBitSet;
throw oom;
}

// Re-mask (we don't recompute the hashcode because we stored all 32 bits of it)
for (int pos = oldBitSet.nextSetBit(0); pos >= 0; pos = oldBitSet.nextSetBit(pos + 1)) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -290,19 +290,25 @@ private void growPointerArrayIfNecessary() throws IOException {
assert(inMemSorter != null);
if (!inMemSorter.hasSpaceForAnotherRecord()) {
long used = inMemSorter.getMemoryUsage();
long needed = inMemSorter.getMemoryToExpand();
long needed = used + inMemSorter.getMemoryToExpand();
try {
acquireMemory(used + needed); // could trigger spilling
if (inMemSorter.hasSpaceForAnotherRecord()) {
releaseMemory(used + needed);
} else {
acquireMemory(needed); // could trigger spilling
} catch (OutOfMemoryError e) {
// should have trigger spilling
assert(inMemSorter.hasSpaceForAnotherRecord());
return;
}
// check if spilling is triggered or not
if (inMemSorter.hasSpaceForAnotherRecord()) {
releaseMemory(needed);
} else {
try {
inMemSorter.expandPointerArray();
releaseMemory(used);
}
} catch (OutOfMemoryError oom) {
// spilling should be triggered
if (!inMemSorter.hasSpaceForAnotherRecord()) {
spill(); // Just in case that JVM had run out of memory
} catch (OutOfMemoryError oom) {
// Just in case that JVM had run out of memory
releaseMemory(needed);
spill();
}
}
}
Expand Down