Mailing-List: contact commits-help@drill.apache.org; run by ezmlm
Precedence: bulk
Reply-To: commits@drill.apache.org
Content-Type: text/plain; charset="us-ascii"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
From: progers@apache.org
To: commits@drill.apache.org
Message-Id: <d777ce1827cc4a05a4d0b12b2c008888@git.apache.org>
Subject: drill git commit: DRILL-5808: Reduce memory allocator strictness for
 "managed" operators
Date: Wed, 11 Oct 2017 18:44:23 +0000 (UTC)
archived-at: Wed, 11 Oct 2017 18:44:25 -0000

Repository: drill
Updated Branches:
  refs/heads/master cf3a5eef2 -> fe8471316


DRILL-5808: Reduce memory allocator strictness for "managed" operators

closes #958


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

Branch: refs/heads/master
Commit: fe84713169d410c40da80d6faf658ba2ac884ccc
Parents: cf3a5ee
Author: Paul Rogers <progers@maprtech.com>
Authored: Sun Sep 24 12:51:43 2017 -0700
Committer: Paul Rogers <progers@maprtech.com>
Committed: Wed Oct 11 11:07:00 2017 -0700

----------------------------------------------------------------------
 .../physical/impl/aggregate/HashAggBatch.java   |   7 +
 .../physical/impl/xsort/managed/SortImpl.java   |  15 +-
 .../xsort/managed/TestLenientAllocation.java    | 194 +++++++++++++++++++
 .../apache/drill/exec/memory/Accountant.java    |  98 +++++++++-
 .../apache/drill/exec/memory/BaseAllocator.java |   9 +-
 .../drill/exec/memory/BufferAllocator.java      |  13 ++
 .../drill/exec/memory/ChildAllocator.java       |   4 +-
 7 files changed, 321 insertions(+), 19 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/drill/blob/fe847131/exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/aggregate/HashAggBatch.java
----------------------------------------------------------------------
diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/aggregate/HashAggBatch.java b/exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/aggregate/HashAggBatch.java
index 314cf6e..8006276 100644
--- a/exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/aggregate/HashAggBatch.java
+++ b/exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/aggregate/HashAggBatch.java
@@ -97,6 +97,13 @@ public class HashAggBatch extends AbstractRecordBatch<HashAggregate> {
       // nulls are equal in group by case
       comparators.add(Comparator.IS_NOT_DISTINCT_FROM);
     }
+
+    // This operator manages its memory use. Ask for leniency
+    // from the allocator to allow for slight errors due to the
+    // lumpiness of vector allocations beyond our control.
+
+    boolean allowed = oContext.getAllocator().setLenient();
+    logger.debug("Config: Is allocator lenient? {}", allowed);
   }
 
   @Override

http://git-wip-us.apache.org/repos/asf/drill/blob/fe847131/exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/xsort/managed/SortImpl.java
----------------------------------------------------------------------
diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/xsort/managed/SortImpl.java b/exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/xsort/managed/SortImpl.java
index 96861ab..d2b589c 100644
--- a/exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/xsort/managed/SortImpl.java
+++ b/exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/xsort/managed/SortImpl.java
@@ -203,15 +203,16 @@ public class SortImpl {
     metrics = new SortMetrics(opContext.getStats());
     bufferedBatches = new BufferedBatches(opContext);
 
-    // Reset the allocator to allow a 10% safety margin. This is done because
-    // the memory manager will enforce the original limit. Changing the hard
-    // limit will reduce the probability that random chance causes the allocator
+    // Request leniency from the allocator. Leniency
+    // will reduce the probability that random chance causes the allocator
     // to kill the query because of a small, spurious over-allocation.
 
-    long maxMem = memManager.getMemoryLimit();
-    long newMax = (long)(maxMem * 1.10);
-    allocator.setLimit(newMax);
-    logger.debug("Config: Resetting allocator to 10% safety margin: {}", newMax);
+//    long maxMem = memManager.getMemoryLimit();
+//    long newMax = (long)(maxMem * 1.10);
+//    allocator.setLimit(newMax);
+//    logger.debug("Config: Resetting allocator to 10% safety margin: {}", newMax);
+    boolean allowed = allocator.setLenient();
+    logger.debug("Config: Is allocator lenient? {}", allowed);
   }
 
   public void setSchema(BatchSchema schema) {

http://git-wip-us.apache.org/repos/asf/drill/blob/fe847131/exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/xsort/managed/TestLenientAllocation.java
----------------------------------------------------------------------
diff --git a/exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/xsort/managed/TestLenientAllocation.java b/exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/xsort/managed/TestLenientAllocation.java
new file mode 100644
index 0000000..71a51f5
--- /dev/null
+++ b/exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/xsort/managed/TestLenientAllocation.java
@@ -0,0 +1,194 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.drill.exec.physical.impl.xsort.managed;
+
+import static org.junit.Assert.*;
+
+import org.apache.drill.exec.exception.OutOfMemoryException;
+import org.apache.drill.exec.memory.Accountant;
+import org.apache.drill.exec.memory.BufferAllocator;
+import org.apache.drill.exec.util.AssertionUtil;
+import org.apache.drill.test.LogFixture;
+import org.apache.drill.test.LogFixture.LogFixtureBuilder;
+import org.apache.drill.test.SubOperatorTest;
+import org.junit.Test;
+
+import ch.qos.logback.classic.Level;
+import io.netty.buffer.DrillBuf;
+
+/**
+ * Test of temporary allocator feature to allow a grace margin
+ * for error in allocations from operators that make a good-faith
+ * effort to stay within their budgets, but are sometimes undone
+ * by unexpected power-of-two buffer sizes and vector doubling.
+ */
+
+public class TestLenientAllocation extends SubOperatorTest {
+
+  /**
+   * Use a test-time hack to force the allocator to be lenient,
+   * regardless of whether we are in debug mode or not.
+   */
+
+  @Test
+  public void testLenient() {
+    LogFixtureBuilder logBuilder = LogFixture.builder()
+        .logger(Accountant.class, Level.WARN);
+
+    try (LogFixture logFixture = logBuilder.build()) {
+
+      // Test can't run without assertions
+
+      assertTrue(AssertionUtil.isAssertionsEnabled());
+
+      // Create a child allocator
+
+      BufferAllocator allocator = fixture.allocator().newChildAllocator("test", 10 * 1024, 128 * 1024);
+      ((Accountant) allocator).forceLenient();
+
+      // Allocate most of the available memory
+
+      DrillBuf buf1 = allocator.buffer(64 * 1024);
+
+      // Oops, we did our math wrong; allocate too large a buffer.
+
+      DrillBuf buf2 = allocator.buffer(128 * 1024);
+
+      assertEquals(192 * 1024, allocator.getAllocatedMemory());
+
+      // We keep making mistakes.
+
+      DrillBuf buf3 = allocator.buffer(32 * 1024);
+
+      // Right up to the hard limit
+
+      DrillBuf buf4 = allocator.buffer(32 * 1024);
+      assertEquals(256 * 1024, allocator.getAllocatedMemory());
+
+      // Enough of this; we're abusing the system. Next
+      // allocation fails.
+
+      try {
+        allocator.buffer(8);
+        fail();
+      } catch (OutOfMemoryException e) {
+        // Expected
+      }
+
+      // Recover from our excesses
+
+      buf2.close();
+      buf3.close();
+      buf4.close();
+      assertEquals(64 * 1024, allocator.getAllocatedMemory());
+
+      // We're back in the good graces of the allocator,
+      // can allocate more.
+
+      DrillBuf buf5 = allocator.buffer(8);
+
+      // Clean up
+
+      buf1.close();
+      buf5.close();
+      allocator.close();
+    }
+  }
+
+  /**
+   * Test that the allocator is normally strict in debug mode.
+   */
+
+  @Test
+  public void testStrict() {
+    LogFixtureBuilder logBuilder = LogFixture.builder()
+        .logger(Accountant.class, Level.WARN);
+
+    try (LogFixture logFixture = logBuilder.build()) {
+
+      // Test can't run without assertions
+
+      assertTrue(AssertionUtil.isAssertionsEnabled());
+
+      // Create a child allocator
+
+      BufferAllocator allocator = fixture.allocator().newChildAllocator("test", 10 * 1024, 128 * 1024);
+
+      // Allocate most of the available memory
+
+      DrillBuf buf1 = allocator.buffer(64 * 1024);
+
+      // Oops, we did our math wrong; allocate too large a buffer.
+
+      try {
+        allocator.buffer(128 * 1024);
+        fail();
+      } catch (OutOfMemoryException e) {
+        // Expected
+      }
+
+      // Clean up
+
+      buf1.close();
+      allocator.close();
+    }
+  }
+
+  public static final int ONE_MEG = 1024 * 1024;
+
+  @Test
+  public void testLenientLimit() {
+    LogFixtureBuilder logBuilder = LogFixture.builder()
+        .logger(Accountant.class, Level.WARN);
+
+    try (LogFixture logFixture = logBuilder.build()) {
+
+      // Test can't run without assertions
+
+      assertTrue(AssertionUtil.isAssertionsEnabled());
+
+      // Create a child allocator
+
+      BufferAllocator allocator = fixture.allocator().newChildAllocator("test", 10 * ONE_MEG, 128 * ONE_MEG);
+      ((Accountant) allocator).forceLenient();
+
+      // Allocate most of the available memory
+
+      DrillBuf buf1 = allocator.buffer(64 * ONE_MEG);
+
+      // Oops, we did our math wrong; allocate too large a buffer.
+
+      DrillBuf buf2 = allocator.buffer(128 * ONE_MEG);
+
+      // Can't go the full 2x over limit, errors capped at 100 MB.
+
+      try {
+        allocator.buffer(64 * ONE_MEG);
+        fail();
+      } catch (OutOfMemoryException e) {
+        // Expected
+      }
+
+      // Clean up
+
+      buf1.close();
+      buf2.close();
+      allocator.close();
+    }
+  }
+}

http://git-wip-us.apache.org/repos/asf/drill/blob/fe847131/exec/memory/base/src/main/java/org/apache/drill/exec/memory/Accountant.java
----------------------------------------------------------------------
diff --git a/exec/memory/base/src/main/java/org/apache/drill/exec/memory/Accountant.java b/exec/memory/base/src/main/java/org/apache/drill/exec/memory/Accountant.java
index 8bcf6a0..b94a6f0 100644
--- a/exec/memory/base/src/main/java/org/apache/drill/exec/memory/Accountant.java
+++ b/exec/memory/base/src/main/java/org/apache/drill/exec/memory/Accountant.java
@@ -1,4 +1,4 @@
-/**
+/*
  * Licensed to the Apache Software Foundation (ASF) under one
  * or more contributor license agreements.  See the NOTICE file
  * distributed with this work for additional information
@@ -22,7 +22,9 @@ import java.util.concurrent.atomic.AtomicLong;
 import javax.annotation.concurrent.ThreadSafe;
 
 import org.apache.drill.exec.exception.OutOfMemoryException;
+import org.apache.drill.exec.util.AssertionUtil;
 
+import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Preconditions;
 
 /**
@@ -30,8 +32,57 @@ import com.google.common.base.Preconditions;
  * operations are threadsafe (except for close).
  */
 @ThreadSafe
-class Accountant implements AutoCloseable {
-  // private static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(Accountant.class);
+@VisibleForTesting
+public class Accountant implements AutoCloseable {
+  private static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(Accountant.class);
+
+  // See DRILL-5808
+  // Allow a "grace margin" above the allocator limit for those operators
+  // that are trying to stay within the limit, but are hindered by the
+  // current power-of-two allocations and random vector doublings. Instead
+  // of failing the query, the accountant allows an excess allocation within
+  // the grace, but issues a warning to the log.
+  //
+  // Normally, the "grace margin" (lenient) allocation is allowed only for
+  // operators that request leniency (because they attempt to manage memory)
+  // and only in production mode. Leniency is usually denied in a debug
+  // run (assertions enabled.) Use the following system option to force
+  // allowing lenient allocation in a debug run (or, to disable lenient
+  // allocation in a production run.)
+  //
+  // Force allowing lenient allocation:
+  //
+  // -Ddrill.memory.lenient.allocator=false
+  //
+  // Force strict allocation;
+  //
+  // -Ddrill.memory.lenient.allocator=false
+
+  public static final String ALLOW_LENIENT_ALLOCATION = "drill.memory.lenient.allocator";
+
+  // Allow clients to allocate beyond the limit by this factor.
+  // If the limit is 10K, then with a margin of 1, the system
+  // will allow a grace of another 10K for a hard limit of 20K.
+  // Warnings are issued for allocations that use the grace.
+
+  public static final int GRACE_MARGIN = 1;
+
+  // In large-memory allocators, don't allow more than 100 MB
+  // grace.
+
+  public static final int MAX_GRACE = 100 * 1024 * 1024;
+
+  // Use the strictness set in a system property. If not set, then
+  // default to strict in debug mode, lenient in production mode.
+
+  public static final boolean ALLOW_LENIENCY =
+      System.getProperty(ALLOW_LENIENT_ALLOCATION) == null
+      ? ! AssertionUtil.isAssertionsEnabled()
+      : Boolean.parseBoolean(System.getProperty(ALLOW_LENIENT_ALLOCATION));
+
+  // Whether leniency has been requested, and granted for this allocator.
+
+  private boolean lenient = false;
 
   /**
    * The parent allocator
@@ -80,6 +131,31 @@ class Accountant implements AutoCloseable {
   }
 
   /**
+   * Request lenient allocations: allows exceeding the allocation limit
+   * by the configured grace amount. The request is granted only if strict
+   * limits are not required.
+   *
+   * @return true if the leniency was granted, false if the current
+   * execution mode, or system property, disallows leniency
+   */
+
+  public boolean setLenient() {
+    lenient = ALLOW_LENIENCY;
+    return lenient;
+  }
+
+  /**
+   * Force lenient allocation. Used for testing to avoid the need to muck
+   * with global settings (assertions or system properties.) <b>Do not</b>
+   * use in production code!
+   */
+
+  @VisibleForTesting
+  public void forceLenient() {
+    lenient = true;
+  }
+
+  /**
    * Attempt to allocate the requested amount of memory. Either completely succeeds or completely fails. Constructs a a
    * log of delta
    *
@@ -148,7 +224,20 @@ class Accountant implements AutoCloseable {
   private AllocationOutcome allocate(final long size, final boolean incomingUpdatePeak, final boolean forceAllocation) {
     final long newLocal = locallyHeldMemory.addAndGet(size);
     final long beyondReservation = newLocal - reservation;
-    final boolean beyondLimit = newLocal > allocationLimit.get();
+    boolean beyondLimit = newLocal > allocationLimit.get();
+
+    // Non-strict allocation
+
+    if (beyondLimit && lenient) {
+      long grace = Math.min(MAX_GRACE, allocationLimit.get() * GRACE_MARGIN);
+      long graceLimit = allocationLimit.get() + grace;
+      beyondLimit = newLocal > graceLimit;
+      if (! beyondLimit) {
+        logger.warn("Excess allocation of {} bytes beyond limit of {}, " +
+                    "within grace of {}, new total allocation: {}",
+            size, allocationLimit.get(), grace, newLocal);
+      }
+    }
     final boolean updatePeak = forceAllocation || (incomingUpdatePeak && !beyondLimit);
 
     AllocationOutcome parentOutcome = AllocationOutcome.SUCCESS;
@@ -201,6 +290,7 @@ class Accountant implements AutoCloseable {
   /**
    * Close this Accountant. This will release any reservation bytes back to a parent Accountant.
    */
+  @Override
   public void close() {
     // return memory reservation to parent allocator.
     if (parent != null) {

http://git-wip-us.apache.org/repos/asf/drill/blob/fe847131/exec/memory/base/src/main/java/org/apache/drill/exec/memory/BaseAllocator.java
----------------------------------------------------------------------
diff --git a/exec/memory/base/src/main/java/org/apache/drill/exec/memory/BaseAllocator.java b/exec/memory/base/src/main/java/org/apache/drill/exec/memory/BaseAllocator.java
index e97493e..6662073 100644
--- a/exec/memory/base/src/main/java/org/apache/drill/exec/memory/BaseAllocator.java
+++ b/exec/memory/base/src/main/java/org/apache/drill/exec/memory/BaseAllocator.java
@@ -1,4 +1,4 @@
-/**
+/*
  * Licensed to the Apache Software Foundation (ASF) under one
  * or more contributor license agreements.  See the NOTICE file
  * distributed with this work for additional information
@@ -133,9 +133,7 @@ public abstract class BaseAllocator extends Accountant implements BufferAllocato
   }
 
   @Override
-  public String getName() {
-    return name;
-  }
+  public String getName() { return name; }
 
   @Override
   public DrillBuf getEmpty() {
@@ -233,7 +231,7 @@ public abstract class BaseAllocator extends Accountant implements BufferAllocato
     final int actualRequestSize = initialRequestSize < CHUNK_SIZE ?
         nextPowerOfTwo(initialRequestSize)
         : initialRequestSize;
-    AllocationOutcome outcome = this.allocateBytes(actualRequestSize);
+    AllocationOutcome outcome = allocateBytes(actualRequestSize);
     if (!outcome.isOk()) {
       throw new OutOfMemoryException(createErrorMsg(this, actualRequestSize, initialRequestSize));
     }
@@ -828,6 +826,7 @@ public abstract class BaseAllocator extends Accountant implements BufferAllocato
     }
   }
 
+  @Override
   public DrillBuf read(int length, InputStream in) throws IOException {
     DrillBuf buf = buffer(length);
     try {

http://git-wip-us.apache.org/repos/asf/drill/blob/fe847131/exec/memory/base/src/main/java/org/apache/drill/exec/memory/BufferAllocator.java
----------------------------------------------------------------------
diff --git a/exec/memory/base/src/main/java/org/apache/drill/exec/memory/BufferAllocator.java b/exec/memory/base/src/main/java/org/apache/drill/exec/memory/BufferAllocator.java
index bdf3073..d45c62f 100644
--- a/exec/memory/base/src/main/java/org/apache/drill/exec/memory/BufferAllocator.java
+++ b/exec/memory/base/src/main/java/org/apache/drill/exec/memory/BufferAllocator.java
@@ -102,6 +102,19 @@ public interface BufferAllocator extends AutoCloseable {
   public void setLimit(long newLimit);
 
   /**
+   * Request lenient enforcement of the allocation limits. Use for
+   * memory-managed operators to prevent minor math errors from killing
+   * queries. This is temporary until Drill manages memory better.
+   * Leniency is allowed only in production code (no assertions),
+   * not in debug mode (assertions enabled).
+   *
+   * @return true if leniency was granted, false if the allocator will
+   * enforce strict limits despite the request
+   */
+
+  public boolean setLenient();
+
+  /**
    * Return the current maximum limit this allocator imposes.
    *
    * @return Limit in number of bytes.

http://git-wip-us.apache.org/repos/asf/drill/blob/fe847131/exec/memory/base/src/main/java/org/apache/drill/exec/memory/ChildAllocator.java
----------------------------------------------------------------------
diff --git a/exec/memory/base/src/main/java/org/apache/drill/exec/memory/ChildAllocator.java b/exec/memory/base/src/main/java/org/apache/drill/exec/memory/ChildAllocator.java
index 8fcabb1..1e4e030 100644
--- a/exec/memory/base/src/main/java/org/apache/drill/exec/memory/ChildAllocator.java
+++ b/exec/memory/base/src/main/java/org/apache/drill/exec/memory/ChildAllocator.java
@@ -1,4 +1,4 @@
-/**
+/*
  * Licensed to the Apache Software Foundation (ASF) under one
  * or more contributor license agreements.  See the NOTICE file
  * distributed with this work for additional information
@@ -48,6 +48,4 @@ class ChildAllocator extends BaseAllocator {
       long maxAllocation) {
     super(parentAllocator, name, initReservation, maxAllocation);
   }
-
-
 }