drill-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From adene...@apache.org
Subject drill git commit: DRILL-3811: AtomicRemainder incorrectly accounts for transferred allocations
Date Tue, 22 Sep 2015 19:29:11 GMT
Repository: drill
Updated Branches:
  refs/heads/master d37462e59 -> 942d3525b


DRILL-3811: AtomicRemainder incorrectly accounts for transferred allocations

this closes #163


Project: http://git-wip-us.apache.org/repos/asf/drill/repo
Commit: http://git-wip-us.apache.org/repos/asf/drill/commit/942d3525
Tree: http://git-wip-us.apache.org/repos/asf/drill/tree/942d3525
Diff: http://git-wip-us.apache.org/repos/asf/drill/diff/942d3525

Branch: refs/heads/master
Commit: 942d3525bb12885d97bc996c0395ff51179495e7
Parents: d37462e
Author: adeneche <adeneche@gmail.com>
Authored: Sun Sep 20 12:11:55 2015 -0700
Committer: adeneche <adeneche@gmail.com>
Committed: Tue Sep 22 10:57:05 2015 -0700

----------------------------------------------------------------------
 .../drill/exec/memory/AtomicRemainder.java      | 31 ++++++++++----------
 .../drill/exec/memory/TestAllocators.java       | 23 +++++++++++++++
 2 files changed, 38 insertions(+), 16 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/drill/blob/942d3525/exec/java-exec/src/main/java/org/apache/drill/exec/memory/AtomicRemainder.java
----------------------------------------------------------------------
diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/memory/AtomicRemainder.java
b/exec/java-exec/src/main/java/org/apache/drill/exec/memory/AtomicRemainder.java
index 057cfa6..0f6b8b0 100644
--- a/exec/java-exec/src/main/java/org/apache/drill/exec/memory/AtomicRemainder.java
+++ b/exec/java-exec/src/main/java/org/apache/drill/exec/memory/AtomicRemainder.java
@@ -25,9 +25,7 @@ import java.util.concurrent.atomic.AtomicLong;
  * TODO: Fix this so that preallocation can never be released back to general pool until
allocator is closed.
  */
 public class AtomicRemainder {
-  static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(AtomicRemainder.class);
-
-  private static final boolean DEBUG = true;
+  private static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(AtomicRemainder.class);
 
   private final AtomicRemainder parent;
   private final AtomicLong availableShared;
@@ -66,7 +64,7 @@ public class AtomicRemainder {
   /**
    * Allow an allocator to constrain the remainder to a particular limit that is lower than
the initTotal.
    * If limit is larger than initTotal, then the function will do nothing and the hasLimit
flag will not be set.
-   * @param limit
+   * @param limit new remainder limit
    */
   public void setLimit(long limit) {
     if(limit<initTotal){
@@ -76,16 +74,17 @@ public class AtomicRemainder {
 
   }
   /**
-   * Automatically allocate memory. This is used when an actual allocation happened to be
larger than requested. This
-   * memory has already been used up so it must be accurately accounted for in future allocations.
+   * Automatically allocate memory. This is used when an actual allocation happened to be
larger than requested, or when
+   * a buffer has it's ownership passed to another allocator.<br>
+   * This memory has already been used up so it must be accurately accounted for in future
allocations.
    *
-   * @param size
+   * @param size extra allocated memory that needs to be accounted for
    */
   public boolean forceGet(long size) {
     if (get(size, this.applyFragmentLimit)) {
       return true;
     } else {
-      availableShared.addAndGet(size);
+      availableShared.addAndGet(-size);
       if (parent != null) {
         parent.forceGet(size);
       }
@@ -108,13 +107,13 @@ public class AtomicRemainder {
           parent.returnAllocation(size);
         }
         StackTraceElement[] ste = (new Throwable()).getStackTrace();
-        StringBuffer sb = new StringBuffer();
+        StringBuilder sb = new StringBuilder();
         for (StackTraceElement s : ste) {
           sb.append(s.toString());
           sb.append("\n");
         }
-        logger.warn("No more memory. Fragment limit ("+this.limit +
-          " bytes) reached. Trying to allocate "+size+ " bytes. "+getUsed()+" bytes already
allocated.\n"+sb.toString());
+        logger.warn("No more memory. Fragment limit ({} bytes) reached. Trying to allocate
{} bytes. {} bytes " +
+          "already allocated.\n{}", limit, size, getUsed(), sb.toString());
         return false;
       }
 
@@ -122,8 +121,7 @@ public class AtomicRemainder {
       long outcome = availableShared.addAndGet(-size);
 //      assert outcome <= initShared;
       if (outcome < 0) {
-        long newAvailableShared = availableShared.addAndGet(size);
-        // assert newAvailableShared <= initShared;
+        availableShared.addAndGet(size);
         if (parent != null) {
           parent.returnAllocation(size);
         }
@@ -163,7 +161,9 @@ public class AtomicRemainder {
           // we failed to get space from available shared. Return allocations to initial
state.
           availablePrivate.addAndGet(size);
           availableShared.addAndGet(additionalSpaceNeeded);
-          parent.returnAllocation(additionalSpaceNeeded);
+          if (parent != null) {
+            parent.returnAllocation(additionalSpaceNeeded);
+          }
           return false;
         }
       }
@@ -175,7 +175,7 @@ public class AtomicRemainder {
   /**
    * Return the memory accounting to the allocation pool. Make sure to first maintain hold
of the preallocated memory.
    *
-   * @param size
+   * @param size amount of memory returned
    */
   public void returnAllocation(long size) {
     long privateSize = availablePrivate.get();
@@ -188,7 +188,6 @@ public class AtomicRemainder {
     if (parent != null) {
       parent.returnAllocation(sharedChange);
     }
-    assert getUsed() <= initTotal;
   }
 
   public void close() {

http://git-wip-us.apache.org/repos/asf/drill/blob/942d3525/exec/java-exec/src/test/java/org/apache/drill/exec/memory/TestAllocators.java
----------------------------------------------------------------------
diff --git a/exec/java-exec/src/test/java/org/apache/drill/exec/memory/TestAllocators.java
b/exec/java-exec/src/test/java/org/apache/drill/exec/memory/TestAllocators.java
index 0b8314c..f362257 100644
--- a/exec/java-exec/src/test/java/org/apache/drill/exec/memory/TestAllocators.java
+++ b/exec/java-exec/src/test/java/org/apache/drill/exec/memory/TestAllocators.java
@@ -63,6 +63,29 @@ public class TestAllocators {
   private final static String planFile="/physical_allocator_test.json";
 
   @Test
+  public void testTransfer() throws Exception {
+    final Properties props = new Properties() {
+      {
+        put(ExecConstants.TOP_LEVEL_MAX_ALLOC, "1000000");
+        put(ExecConstants.ERROR_ON_MEMORY_LEAK, "true");
+      }
+    };
+    final DrillConfig config = DrillConfig.create(props);
+    BufferAllocator a = RootAllocatorFactory.newRoot(config);
+    BufferAllocator b = RootAllocatorFactory.newRoot(config);
+
+    DrillBuf buf1 = a.buffer(1_000_000);
+    DrillBuf buf2 = b.buffer(1_000);
+    b.takeOwnership(buf1);
+
+    buf1.release();
+    buf2.release();
+
+    a.close();
+    b.close();
+  }
+
+  @Test
   public void testAllocators() throws Exception {
     // Setup a drillbit (initializes a root allocator)
     final DrillConfig config = DrillConfig.create(TEST_CONFIGURATIONS);


Mime
View raw message