You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by vrozov <gi...@git.apache.org> on 2017/12/13 05:21:20 UTC
[GitHub] drill pull request #1070: DRILL-6004: Direct buffer bounds checking should b...
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 <vr...@apache.org>
Date: 2017-12-12T21:37:38Z
DRILL-6004: Direct buffer bounds checking should be disabled by default
----
---
[GitHub] drill issue #1070: DRILL-6004: Direct buffer bounds checking should be disab...
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on the issue:
https://github.com/apache/drill/pull/1070
@paul-rogers Please review
---
[GitHub] drill pull request #1070: DRILL-6004: Direct buffer bounds checking should b...
Posted by asfgit <gi...@git.apache.org>.
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...
Posted by vrozov <gi...@git.apache.org>.
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...
Posted by vrozov <gi...@git.apache.org>.
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...
Posted by vrozov <gi...@git.apache.org>.
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...
Posted by paul-rogers <gi...@git.apache.org>.
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...
Posted by paul-rogers <gi...@git.apache.org>.
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...
Posted by paul-rogers <gi...@git.apache.org>.
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...
---