drill-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From vrozov <...@git.apache.org>
Subject [GitHub] drill pull request #1070: DRILL-6004: Direct buffer bounds checking should b...
Date Thu, 21 Dec 2017 17:32:25 GMT
Github user vrozov commented on a diff in the pull request:

    https://github.com/apache/drill/pull/1070#discussion_r158334992
  
    --- Diff: exec/memory/base/src/main/java/org/apache/drill/exec/memory/BoundsChecking.java
---
    @@ -17,19 +17,92 @@
      */
     package org.apache.drill.exec.memory;
     
    +import java.lang.reflect.Field;
    +import java.util.Formatter;
    +
    +import com.google.common.base.Preconditions;
    +
    +import io.netty.buffer.AbstractByteBuf;
    +import io.netty.buffer.DrillBuf;
    +import io.netty.util.IllegalReferenceCountException;
    +
    +import static org.apache.drill.exec.util.SystemPropertyUtil.getBoolean;
    +
     public class BoundsChecking {
    -  static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(BoundsChecking.class);
    +  private static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(BoundsChecking.class);
     
    -  public static final boolean BOUNDS_CHECKING_ENABLED;
    +  public static final String ENABLE_UNSAFE_BOUNDS_CHECK_PROPERTY = "drill.exec.memory.enable_unsafe_bounds_check";
    +  // for backward compatibility check "drill.enable_unsafe_memory_access" property and
enable bounds checking when
    +  // unsafe memory access is explicitly disabled
    +  public static final String ENABLE_UNSAFE_MEMORY_ACCESS_PROPERTY = "drill.enable_unsafe_memory_access";
    +  public static final boolean BOUNDS_CHECKING_ENABLED =
    +      getBoolean(ENABLE_UNSAFE_BOUNDS_CHECK_PROPERTY, !getBoolean(ENABLE_UNSAFE_MEMORY_ACCESS_PROPERTY,
true));
    +  private static final boolean checkAccessible = getStaticBooleanField(AbstractByteBuf.class,
"checkAccessible", false);
     
       static {
    -    boolean isAssertEnabled = false;
    -    assert isAssertEnabled = true;
    -    BOUNDS_CHECKING_ENABLED = isAssertEnabled
    -        || !"true".equals(System.getProperty("drill.enable_unsafe_memory_access"));
    +    if (BOUNDS_CHECKING_ENABLED) {
    +      logger.warn("Drill is running with direct memory bounds checking enabled. If this
is a production system, disable it.");
    +    } else if (logger.isDebugEnabled()) {
    +      logger.debug("Direct memory bounds checking is disabled.");
    +    }
       }
     
       private BoundsChecking() {
       }
     
    +  private static boolean getStaticBooleanField(Class cls, String name, boolean def) {
    +    try {
    +      Field field = cls.getDeclaredField(name);
    +      field.setAccessible(true);
    +      return field.getBoolean(null);
    +    } catch (ReflectiveOperationException e) {
    +      return def;
    +    }
    +  }
    +
    +  private static void checkIndex(DrillBuf buf, int index, int fieldLength) {
    +    Preconditions.checkNotNull(buf);
    +    if (checkAccessible && buf.refCnt() == 0) {
    +      Formatter formatter = new Formatter().format("%s, refCnt: 0", buf);
    +      if (BaseAllocator.DEBUG) {
    +        formatter.format("%n%s", buf.toVerboseString());
    +      }
    +      throw new IllegalReferenceCountException(formatter.toString());
    +    }
    +    if (fieldLength < 0) {
    +      throw new IllegalArgumentException(String.format("length: %d (expected: >= 0)",
fieldLength));
    +    }
    +    if (index < 0 || index > buf.capacity() - fieldLength) {
    +      Formatter formatter = new Formatter().format("%s, index: %d, length: %d (expected:
range(0, %d))", buf, index, fieldLength, buf.capacity());
    +      if (BaseAllocator.DEBUG) {
    +        formatter.format("%n%s", buf.toVerboseString());
    +      }
    +      throw new IndexOutOfBoundsException(formatter.toString());
    +    }
    +  }
    +
    +  public static void lengthCheck(DrillBuf buf, int start, int length) {
    +    if (BOUNDS_CHECKING_ENABLED) {
    +      checkIndex(buf, start, length);
    +    }
    +  }
    +
    +  public static void rangeCheck(DrillBuf buf, int start, int end) {
    +    if (BOUNDS_CHECKING_ENABLED) {
    +      checkIndex(buf, start, end - start);
    +    }
    +  }
    +
    +  public static void rangeCheck(DrillBuf buf1, int start1, int end1, DrillBuf buf2, int
start2, int end2) {
    --- End diff --
    
    AFAIK, there will be no performance difference between a final static pointer and a final
static boolean. In both cases, hotspot will optimize it out and inline no-op versions. From
code readability point, I would prefer a single class that encapsulates bounds checking functionality.


---

Mime
View raw message