[GitHub] drill pull request #1070: DRILL-6004: Direct buffer bounds checking should b...

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/drill/pull/1070


---

[GitHub] drill pull request #1070: DRILL-6004: Direct buffer bounds checking should b...

[vrozov]

Github user vrozov commented on a diff in the pull request:

https://github.com/apache/drill/pull/1070#discussion_r158335955
  
--- 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 --

It is a common case for `equal()` and `compare()`.


---

[GitHub] drill pull request #1070: DRILL-6004: Direct buffer bounds checking should b...

[vrozov]

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.


---

[GitHub] drill pull request #1070: DRILL-6004: Direct buffer bounds checking should b...

[vrozov]

Github user vrozov commented on a diff in the pull request:

https://github.com/apache/drill/pull/1070#discussion_r158331061
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/impl/XXHash.java ---
@@ -166,9 +164,7 @@ public static long hash64(double val, long seed){
   }
 
   public static long hash64(long start, long end, DrillBuf buffer, long 
seed){
-if (BoundsChecking.BOUNDS_CHECKING_ENABLED) {
-  buffer.checkBytes((int)start, (int)end);
-}
+rangeCheck(buffer, (int)start, (int)end);
--- End diff --

I don't see `XXHash` class being used anymore and it possibly can be 
removed as a follow up for DRILL-4237 and DRILL-4478. I'd prefer to limit this 
PR to bounds checking functionality.


---

[GitHub] drill pull request #1070: DRILL-6004: Direct buffer bounds checking should b...

[paul-rogers]

Github user paul-rogers commented on a diff in the pull request:

https://github.com/apache/drill/pull/1070#discussion_r158164933
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/impl/XXHash.java ---
@@ -166,9 +164,7 @@ public static long hash64(double val, long seed){
   }
 
   public static long hash64(long start, long end, DrillBuf buffer, long 
seed){
-if (BoundsChecking.BOUNDS_CHECKING_ENABLED) {
-  buffer.checkBytes((int)start, (int)end);
-}
+rangeCheck(buffer, (int)start, (int)end);
--- End diff --

Why are the arguments to `hash64` longs? The hash may be 64 bits, but Drill 
does not support vectors above 2 GB (31 bits) in length, so the `start` and 
`end` need only be `int`.


---

[GitHub] drill pull request #1070: DRILL-6004: Direct buffer bounds checking should b...

[paul-rogers]

Github user paul-rogers commented on a diff in the pull request:

https://github.com/apache/drill/pull/1070#discussion_r158165417
  
--- 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 --

This may be easier/faster if we do:

* Create a static pointer to a bounds checker.
* Two implementations: on and off.
* The off implementation simply returns.
* The on implementation does the check.
* Startup code sets the on or off instance as desired.

However, it may be that the compiler generates better code with static 
functions and final variables...


---

[GitHub] drill pull request #1070: DRILL-6004: Direct buffer bounds checking should b...

[paul-rogers]

Github user paul-rogers commented on a diff in the pull request:

https://github.com/apache/drill/pull/1070#discussion_r158165539
  
--- 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 --

Why do we need a two-buffer version? Why not two calls to the one-buffer 
version for simplicity?


---

[GitHub] drill pull request #1070: DRILL-6004: Direct buffer bounds checking should b...

[vrozov]

GitHub user vrozov opened a pull request:

https://github.com/apache/drill/pull/1070

DRILL-6004: Direct buffer bounds checking should be disabled by default



You can merge this pull request into a Git repository by running:

$ git pull https://github.com/vrozov/drill DRILL-6004

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/drill/pull/1070.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #1070


commit b863f2a0c9c595dacb924747dea6c449c9ee9917
Author: Vlad Rozov 
Date:   2017-12-12T21:37:38Z

DRILL-6004: Direct buffer bounds checking should be disabled by default




---