Repository: carbondata Updated Branches: refs/heads/master 8ea7272d9 -> 289232607
[CARBONDATA-1393] Avoid to throw NPE when execute 'freeMemory' of UnsafeMemoryManager/UnsafeSortMemoryManager UnsafeMemoryManager.freeMemoryAll(long taskId) may run before freeMemory(long taskId, MemoryBlock memoryBlock), so taskIdToMemoryBlockMap.get(taskId) will return null and then throw NPE. This closes #1266 Project: http://git-wip-us.apache.org/repos/asf/carbondata/repo Commit: http://git-wip-us.apache.org/repos/asf/carbondata/commit/28923260 Tree: http://git-wip-us.apache.org/repos/asf/carbondata/tree/28923260 Diff: http://git-wip-us.apache.org/repos/asf/carbondata/diff/28923260 Branch: refs/heads/master Commit: 289232607ae51f350c0dc13dcbb9da7b95491c11 Parents: 8ea7272 Author: Zhang Zhichao <[email protected]> Authored: Fri Aug 18 17:04:50 2017 +0800 Committer: Ravindra Pesala <[email protected]> Committed: Wed Aug 30 16:45:52 2017 +0530 ---------------------------------------------------------------------- .../core/memory/HeapMemoryAllocator.java | 3 +++ .../carbondata/core/memory/MemoryBlock.java | 14 +++++++++++ .../core/memory/UnsafeMemoryAllocator.java | 1 + .../core/memory/UnsafeMemoryManager.java | 24 +++++++++++------- .../core/memory/UnsafeSortMemoryManager.java | 26 ++++++++++++-------- 5 files changed, 49 insertions(+), 19 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/carbondata/blob/28923260/core/src/main/java/org/apache/carbondata/core/memory/HeapMemoryAllocator.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/carbondata/core/memory/HeapMemoryAllocator.java b/core/src/main/java/org/apache/carbondata/core/memory/HeapMemoryAllocator.java index 2b63726..5862933 100644 --- a/core/src/main/java/org/apache/carbondata/core/memory/HeapMemoryAllocator.java +++ b/core/src/main/java/org/apache/carbondata/core/memory/HeapMemoryAllocator.java @@ -53,6 +53,8 @@ public class HeapMemoryAllocator implements MemoryAllocator { final MemoryBlock memory = blockReference.get(); if (memory != null) { assert (memory.size() == size); + // reuse this MemoryBlock + memory.setFreedStatus(false); return memory; } } @@ -76,5 +78,6 @@ public class HeapMemoryAllocator implements MemoryAllocator { pool.add(new WeakReference<>(memory)); } } + memory.setFreedStatus(true); } } http://git-wip-us.apache.org/repos/asf/carbondata/blob/28923260/core/src/main/java/org/apache/carbondata/core/memory/MemoryBlock.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/carbondata/core/memory/MemoryBlock.java b/core/src/main/java/org/apache/carbondata/core/memory/MemoryBlock.java index 74136c1..d6cb184 100644 --- a/core/src/main/java/org/apache/carbondata/core/memory/MemoryBlock.java +++ b/core/src/main/java/org/apache/carbondata/core/memory/MemoryBlock.java @@ -29,9 +29,15 @@ public class MemoryBlock extends MemoryLocation { private final long length; + /** + * whether freed or not + */ + private boolean isFreed = false; + public MemoryBlock(@Nullable Object obj, long offset, long length) { super(obj, offset); this.length = length; + this.isFreed = false; } /** @@ -41,6 +47,14 @@ public class MemoryBlock extends MemoryLocation { return length; } + public boolean isFreedStatus() { + return this.isFreed; + } + + public void setFreedStatus(boolean freedStatus) { + this.isFreed = freedStatus; + } + /** * Creates a memory block pointing to the memory used by the long array. */ http://git-wip-us.apache.org/repos/asf/carbondata/blob/28923260/core/src/main/java/org/apache/carbondata/core/memory/UnsafeMemoryAllocator.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/carbondata/core/memory/UnsafeMemoryAllocator.java b/core/src/main/java/org/apache/carbondata/core/memory/UnsafeMemoryAllocator.java index fcb0b88..67412ac 100644 --- a/core/src/main/java/org/apache/carbondata/core/memory/UnsafeMemoryAllocator.java +++ b/core/src/main/java/org/apache/carbondata/core/memory/UnsafeMemoryAllocator.java @@ -36,5 +36,6 @@ public class UnsafeMemoryAllocator implements MemoryAllocator { assert (memory.obj == null) : "baseObject not null; are you trying to use the off-heap allocator to free on-heap memory?"; CarbonUnsafe.getUnsafe().freeMemory(memory.offset); + memory.setFreedStatus(true); } } http://git-wip-us.apache.org/repos/asf/carbondata/blob/28923260/core/src/main/java/org/apache/carbondata/core/memory/UnsafeMemoryManager.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/carbondata/core/memory/UnsafeMemoryManager.java b/core/src/main/java/org/apache/carbondata/core/memory/UnsafeMemoryManager.java index 1190d56..06f907d 100644 --- a/core/src/main/java/org/apache/carbondata/core/memory/UnsafeMemoryManager.java +++ b/core/src/main/java/org/apache/carbondata/core/memory/UnsafeMemoryManager.java @@ -105,13 +105,17 @@ public class UnsafeMemoryManager { } public synchronized void freeMemory(long taskId, MemoryBlock memoryBlock) { - taskIdToMemoryBlockMap.get(taskId).remove(memoryBlock); - allocator.free(memoryBlock); - memoryUsed -= memoryBlock.size(); - memoryUsed = memoryUsed < 0 ? 0 : memoryUsed; - LOGGER.info( - "Freeing memory of size: " + memoryBlock.size() + "available memory: " + (totalMemory - - memoryUsed)); + if (taskIdToMemoryBlockMap.containsKey(taskId)) { + taskIdToMemoryBlockMap.get(taskId).remove(memoryBlock); + } + if (!memoryBlock.isFreedStatus()) { + allocator.free(memoryBlock); + memoryUsed -= memoryBlock.size(); + memoryUsed = memoryUsed < 0 ? 0 : memoryUsed; + LOGGER.info( + "Freeing memory of size: " + memoryBlock.size() + "available memory: " + (totalMemory + - memoryUsed)); + } } public synchronized void freeMemoryAll(long taskId) { @@ -123,8 +127,10 @@ public class UnsafeMemoryManager { MemoryBlock memoryBlock = null; while (iterator.hasNext()) { memoryBlock = iterator.next(); - occuppiedMemory += memoryBlock.size(); - allocator.free(memoryBlock); + if (!memoryBlock.isFreedStatus()) { + occuppiedMemory += memoryBlock.size(); + allocator.free(memoryBlock); + } } } memoryUsed -= occuppiedMemory; http://git-wip-us.apache.org/repos/asf/carbondata/blob/28923260/core/src/main/java/org/apache/carbondata/core/memory/UnsafeSortMemoryManager.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/carbondata/core/memory/UnsafeSortMemoryManager.java b/core/src/main/java/org/apache/carbondata/core/memory/UnsafeSortMemoryManager.java index b4a200a..c63b320 100644 --- a/core/src/main/java/org/apache/carbondata/core/memory/UnsafeSortMemoryManager.java +++ b/core/src/main/java/org/apache/carbondata/core/memory/UnsafeSortMemoryManager.java @@ -135,14 +135,18 @@ public class UnsafeSortMemoryManager { } public synchronized void freeMemory(long taskId, MemoryBlock memoryBlock) { - allocator.free(memoryBlock); - taskIdToMemoryBlockMap.get(taskId).remove(memoryBlock); - memoryUsed -= memoryBlock.size(); - memoryUsed = memoryUsed < 0 ? 0 : memoryUsed; - if (LOGGER.isDebugEnabled()) { - LOGGER.debug( - "Freeing memory of size: " + memoryBlock.size() + ": Current available memory is: " + ( - totalMemory - memoryUsed)); + if (taskIdToMemoryBlockMap.containsKey(taskId)) { + taskIdToMemoryBlockMap.get(taskId).remove(memoryBlock); + } + if (!memoryBlock.isFreedStatus()) { + allocator.free(memoryBlock); + memoryUsed -= memoryBlock.size(); + memoryUsed = memoryUsed < 0 ? 0 : memoryUsed; + if (LOGGER.isDebugEnabled()) { + LOGGER.debug( + "Freeing memory of size: " + memoryBlock.size() + ": Current available memory is: " + ( + totalMemory - memoryUsed)); + } } } @@ -161,8 +165,10 @@ public class UnsafeSortMemoryManager { MemoryBlock memoryBlock = null; while (iterator.hasNext()) { memoryBlock = iterator.next(); - occuppiedMemory += memoryBlock.size(); - allocator.free(memoryBlock); + if (!memoryBlock.isFreedStatus()) { + occuppiedMemory += memoryBlock.size(); + allocator.free(memoryBlock); + } } } memoryUsed -= occuppiedMemory;
