You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2020/06/04 09:49:00 UTC
[GitHub] [arrow] rymurr opened a new pull request #7347: ARROW-8230: [Java] Remove netty dependency from arrow-memory
rymurr opened a new pull request #7347:
URL: https://github.com/apache/arrow/pull/7347
This commit moves all Netty specific calls into a few classes.
This is the precursor to splitting the netty and unsafe allocatorsout to their own modules
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] liyafan82 merged pull request #7347: ARROW-8230: [Java] Remove netty dependency from arrow-memory
Posted by GitBox <gi...@apache.org>.
liyafan82 merged pull request #7347:
URL: https://github.com/apache/arrow/pull/7347
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rymurr commented on a change in pull request #7347: ARROW-8230: [Java] Remove netty dependency from arrow-memory
Posted by GitBox <gi...@apache.org>.
rymurr commented on a change in pull request #7347:
URL: https://github.com/apache/arrow/pull/7347#discussion_r445373746
##########
File path: java/memory/src/main/java/org/apache/arrow/memory/rounding/DefaultRoundingPolicy.java
##########
@@ -31,19 +28,18 @@
public final long chunkSize;
+ /**
+ * default chunk size from Netty.
+ */
+ private static final long DEFAULT_CHUNK_SIZE = 16777216L;
Review comment:
I ended up taking the implementation from Netty rather than the static value here. I thought it was a bit safer.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rymurr commented on a change in pull request #7347: ARROW-8230: [Java] Remove netty dependency from arrow-memory
Posted by GitBox <gi...@apache.org>.
rymurr commented on a change in pull request #7347:
URL: https://github.com/apache/arrow/pull/7347#discussion_r446921213
##########
File path: java/memory/src/main/java/org/apache/arrow/memory/rounding/DefaultRoundingPolicy.java
##########
@@ -17,33 +17,107 @@
package org.apache.arrow.memory.rounding;
-import java.lang.reflect.Field;
-
-import org.apache.arrow.memory.NettyAllocationManager;
import org.apache.arrow.memory.util.CommonUtil;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import io.netty.util.internal.SystemPropertyUtil;
/**
* The default rounding policy. That is, if the requested size is within the chunk size,
* the rounded size will be the next power of two. Otherwise, the rounded size
* will be identical to the requested size.
*/
public class DefaultRoundingPolicy implements RoundingPolicy {
-
+ private static final Logger logger = LoggerFactory.getLogger(DefaultRoundingPolicy.class);
public final long chunkSize;
/**
- * The singleton instance.
+ * The variables here and the static block calculates teh DEFAULT_CHUNK_SIZE.
+ *
+ * <p>
+ * It was copied from {@link io.netty.buffer.PooledByteBufAllocator}.
+ * </p>
*/
- public static final DefaultRoundingPolicy INSTANCE = new DefaultRoundingPolicy();
+ private static final int MIN_PAGE_SIZE = 4096;
+ private static final int MAX_CHUNK_SIZE = (int) (((long) Integer.MAX_VALUE + 1) / 2);
+ private static final long DEFAULT_CHUNK_SIZE;
+ private static final int DEFAULT_PAGE_SIZE;
+ private static final int DEFAULT_MAX_ORDER;
- private DefaultRoundingPolicy() {
+
+ static {
+ int defaultPageSize = SystemPropertyUtil.getInt("org.apache.memory.allocator.pageSize", 8192);
+ Throwable pageSizeFallbackCause = null;
try {
- Field field = NettyAllocationManager.class.getDeclaredField("CHUNK_SIZE");
- field.setAccessible(true);
- chunkSize = (Long) field.get(null);
- } catch (Exception e) {
- throw new RuntimeException("Failed to get chunk size from allocation manager");
+ validateAndCalculatePageShifts(defaultPageSize);
+ } catch (Throwable t) {
+ pageSizeFallbackCause = t;
+ defaultPageSize = 8192;
+ }
+ DEFAULT_PAGE_SIZE = defaultPageSize;
+
+ int defaultMaxOrder = SystemPropertyUtil.getInt("org.apache.memory.allocator.maxOrder", 11);
+ Throwable maxOrderFallbackCause = null;
+ try {
+ validateAndCalculateChunkSize(DEFAULT_PAGE_SIZE, defaultMaxOrder);
+ } catch (Throwable t) {
+ maxOrderFallbackCause = t;
+ defaultMaxOrder = 11;
+ }
+ DEFAULT_MAX_ORDER = defaultMaxOrder;
+ DEFAULT_CHUNK_SIZE = validateAndCalculateChunkSize(DEFAULT_PAGE_SIZE, DEFAULT_MAX_ORDER);
+ if (logger.isDebugEnabled()) {
+ if (pageSizeFallbackCause == null) {
+ logger.debug("-Dorg.apache.memory.allocator.pageSize: {}", DEFAULT_PAGE_SIZE);
+ } else {
+ logger.debug("-Dorg.apache.memory.allocator.pageSize: {}", DEFAULT_PAGE_SIZE, pageSizeFallbackCause);
+ }
+ if (maxOrderFallbackCause == null) {
+ logger.debug("-Dorg.apache.memory.allocator.maxOrder: {}", DEFAULT_MAX_ORDER);
+ } else {
+ logger.debug("-Dorg.apache.memory.allocator.maxOrder: {}", DEFAULT_MAX_ORDER, maxOrderFallbackCause);
+ }
Review comment:
done
##########
File path: java/memory/src/main/java/org/apache/arrow/memory/rounding/DefaultRoundingPolicy.java
##########
@@ -17,33 +17,107 @@
package org.apache.arrow.memory.rounding;
-import java.lang.reflect.Field;
-
-import org.apache.arrow.memory.NettyAllocationManager;
import org.apache.arrow.memory.util.CommonUtil;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import io.netty.util.internal.SystemPropertyUtil;
/**
* The default rounding policy. That is, if the requested size is within the chunk size,
* the rounded size will be the next power of two. Otherwise, the rounded size
* will be identical to the requested size.
*/
public class DefaultRoundingPolicy implements RoundingPolicy {
-
+ private static final Logger logger = LoggerFactory.getLogger(DefaultRoundingPolicy.class);
public final long chunkSize;
/**
- * The singleton instance.
+ * The variables here and the static block calculates teh DEFAULT_CHUNK_SIZE.
+ *
+ * <p>
+ * It was copied from {@link io.netty.buffer.PooledByteBufAllocator}.
+ * </p>
*/
- public static final DefaultRoundingPolicy INSTANCE = new DefaultRoundingPolicy();
+ private static final int MIN_PAGE_SIZE = 4096;
+ private static final int MAX_CHUNK_SIZE = (int) (((long) Integer.MAX_VALUE + 1) / 2);
+ private static final long DEFAULT_CHUNK_SIZE;
+ private static final int DEFAULT_PAGE_SIZE;
+ private static final int DEFAULT_MAX_ORDER;
- private DefaultRoundingPolicy() {
+
+ static {
+ int defaultPageSize = SystemPropertyUtil.getInt("org.apache.memory.allocator.pageSize", 8192);
+ Throwable pageSizeFallbackCause = null;
try {
- Field field = NettyAllocationManager.class.getDeclaredField("CHUNK_SIZE");
- field.setAccessible(true);
- chunkSize = (Long) field.get(null);
- } catch (Exception e) {
- throw new RuntimeException("Failed to get chunk size from allocation manager");
+ validateAndCalculatePageShifts(defaultPageSize);
+ } catch (Throwable t) {
+ pageSizeFallbackCause = t;
+ defaultPageSize = 8192;
+ }
+ DEFAULT_PAGE_SIZE = defaultPageSize;
+
+ int defaultMaxOrder = SystemPropertyUtil.getInt("org.apache.memory.allocator.maxOrder", 11);
+ Throwable maxOrderFallbackCause = null;
+ try {
+ validateAndCalculateChunkSize(DEFAULT_PAGE_SIZE, defaultMaxOrder);
+ } catch (Throwable t) {
+ maxOrderFallbackCause = t;
+ defaultMaxOrder = 11;
+ }
+ DEFAULT_MAX_ORDER = defaultMaxOrder;
+ DEFAULT_CHUNK_SIZE = validateAndCalculateChunkSize(DEFAULT_PAGE_SIZE, DEFAULT_MAX_ORDER);
+ if (logger.isDebugEnabled()) {
+ if (pageSizeFallbackCause == null) {
+ logger.debug("-Dorg.apache.memory.allocator.pageSize: {}", DEFAULT_PAGE_SIZE);
+ } else {
+ logger.debug("-Dorg.apache.memory.allocator.pageSize: {}", DEFAULT_PAGE_SIZE, pageSizeFallbackCause);
Review comment:
done
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rymurr commented on a change in pull request #7347: ARROW-8230: [Java] Remove netty dependency from arrow-memory
Posted by GitBox <gi...@apache.org>.
rymurr commented on a change in pull request #7347:
URL: https://github.com/apache/arrow/pull/7347#discussion_r445373657
##########
File path: java/memory/src/main/java/org/apache/arrow/memory/util/MemoryUtil.java
##########
@@ -78,6 +77,55 @@ public Object run() {
Field addressField = java.nio.Buffer.class.getDeclaredField("address");
addressField.setAccessible(true);
BYTE_BUFFER_ADDRESS_OFFSET = UNSAFE.objectFieldOffset(addressField);
+
+ Constructor<?> directBufferConstructor;
+ long address = -1;
+ final ByteBuffer direct = ByteBuffer.allocateDirect(1);
+ try {
+
+ final Object maybeDirectBufferConstructor =
+ AccessController.doPrivileged(new PrivilegedAction<Object>() {
+ @Override
+ public Object run() {
+ try {
+ final Constructor<?> constructor =
+ direct.getClass().getDeclaredConstructor(long.class, int.class);
+ constructor.setAccessible(true);
+ return constructor;
+ } catch (NoSuchMethodException e) {
+ return e;
+ } catch (SecurityException e) {
+ return e;
+ }
+ }
+ });
+
+ if (maybeDirectBufferConstructor instanceof Constructor<?>) {
+ address = UNSAFE.allocateMemory(1);
+ // try to use the constructor now
+ try {
+ ((Constructor<?>) maybeDirectBufferConstructor).newInstance(address, 1);
+ directBufferConstructor = (Constructor<?>) maybeDirectBufferConstructor;
+ logger.debug("direct buffer constructor: available");
+ } catch (InstantiationException e) {
+ directBufferConstructor = null;
Review comment:
done
##########
File path: java/memory/src/main/java/org/apache/arrow/memory/rounding/DefaultRoundingPolicy.java
##########
@@ -31,19 +28,18 @@
public final long chunkSize;
+ /**
+ * default chunk size from Netty.
+ */
+ private static final long DEFAULT_CHUNK_SIZE = 16777216L;
Review comment:
done
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rymurr commented on pull request #7347: ARROW-8230: [Java] Remove netty dependency from arrow-memory
Posted by GitBox <gi...@apache.org>.
rymurr commented on pull request #7347:
URL: https://github.com/apache/arrow/pull/7347#issuecomment-651078278
Thanks @liyafan82 I have updated based on your comments and the integration tests have passed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] liyafan82 commented on a change in pull request #7347: ARROW-8230: [Java] Remove netty dependency from arrow-memory
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #7347:
URL: https://github.com/apache/arrow/pull/7347#discussion_r437116925
##########
File path: java/memory/src/main/java/io/netty/buffer/NettyArrowBuf.java
##########
@@ -404,7 +407,7 @@ protected int _getUnsignedMediumLE(int index) {
this.chk(index, 3);
long addr = this.addr(index);
return PlatformDependent.getByte(addr) & 255 |
- (Short.reverseBytes(PlatformDependent.getShort(addr + 1L)) & '\uffff') << 8;
+ (Short.reverseBytes(PlatformDependent.getShort(addr + 1L)) & '\uffff') << 8;
Review comment:
(Maybe it is irrelavant to this PR). Here a single call to PlatformDependent.getShort should be sufficient?
It seems reverseBytes and then shift left amounts to directly getting the lower bits?
##########
File path: java/memory/src/main/java/org/apache/arrow/memory/util/MemoryUtil.java
##########
@@ -78,6 +77,55 @@ public Object run() {
Field addressField = java.nio.Buffer.class.getDeclaredField("address");
addressField.setAccessible(true);
BYTE_BUFFER_ADDRESS_OFFSET = UNSAFE.objectFieldOffset(addressField);
+
+ Constructor<?> directBufferConstructor;
+ long address = -1;
+ final ByteBuffer direct = ByteBuffer.allocateDirect(1);
+ try {
+
+ final Object maybeDirectBufferConstructor =
+ AccessController.doPrivileged(new PrivilegedAction<Object>() {
+ @Override
+ public Object run() {
+ try {
+ final Constructor<?> constructor =
+ direct.getClass().getDeclaredConstructor(long.class, int.class);
+ constructor.setAccessible(true);
+ return constructor;
+ } catch (NoSuchMethodException e) {
+ return e;
+ } catch (SecurityException e) {
+ return e;
+ }
+ }
+ });
+
+ if (maybeDirectBufferConstructor instanceof Constructor<?>) {
+ address = UNSAFE.allocateMemory(1);
+ // try to use the constructor now
+ try {
+ ((Constructor<?>) maybeDirectBufferConstructor).newInstance(address, 1);
+ directBufferConstructor = (Constructor<?>) maybeDirectBufferConstructor;
+ logger.debug("direct buffer constructor: available");
+ } catch (InstantiationException e) {
+ directBufferConstructor = null;
Review comment:
we should print some logs here?
##########
File path: java/memory/src/main/java/org/apache/arrow/memory/util/MemoryUtil.java
##########
@@ -78,6 +77,55 @@ public Object run() {
Field addressField = java.nio.Buffer.class.getDeclaredField("address");
addressField.setAccessible(true);
BYTE_BUFFER_ADDRESS_OFFSET = UNSAFE.objectFieldOffset(addressField);
+
+ Constructor<?> directBufferConstructor;
+ long address = -1;
+ final ByteBuffer direct = ByteBuffer.allocateDirect(1);
+ try {
+
+ final Object maybeDirectBufferConstructor =
+ AccessController.doPrivileged(new PrivilegedAction<Object>() {
+ @Override
+ public Object run() {
+ try {
+ final Constructor<?> constructor =
+ direct.getClass().getDeclaredConstructor(long.class, int.class);
+ constructor.setAccessible(true);
+ return constructor;
+ } catch (NoSuchMethodException e) {
+ return e;
Review comment:
It would be helpful for problem diagnostic, if we could print some logs here?
##########
File path: java/memory/src/main/java/org/apache/arrow/memory/rounding/DefaultRoundingPolicy.java
##########
@@ -31,19 +28,18 @@
public final long chunkSize;
+ /**
+ * default chunk size from Netty.
+ */
+ private static final long DEFAULT_CHUNK_SIZE = 16777216L;
Review comment:
Is it possible that this value changes from Netty code base?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] liyafan82 commented on a change in pull request #7347: ARROW-8230: [Java] Remove netty dependency from arrow-memory
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #7347:
URL: https://github.com/apache/arrow/pull/7347#discussion_r446594755
##########
File path: java/memory/src/main/java/org/apache/arrow/memory/ArrowBuf.java
##########
@@ -227,13 +207,28 @@ public ArrowBuf slice(long index, long length) {
return newBuf;
}
+ /**
+ * Make an nio byte buffery from this arrowbuf.
Review comment:
nit: an -> a
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] liyafan82 commented on pull request #7347: ARROW-8230: [Java] Remove netty dependency from arrow-memory
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on pull request #7347:
URL: https://github.com/apache/arrow/pull/7347#issuecomment-650685726
@rymurr Thanks for your effort. I will make another pass today.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #7347: ARROW-8230: [Java] Remove netty dependency from arrow-memory
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #7347:
URL: https://github.com/apache/arrow/pull/7347#issuecomment-638752095
https://issues.apache.org/jira/browse/ARROW-8230
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] liyafan82 commented on pull request #7347: ARROW-8230: [Java] Remove netty dependency from arrow-memory
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on pull request #7347:
URL: https://github.com/apache/arrow/pull/7347#issuecomment-651474250
@rymurr Thanks for your work. A few typos.
I think it would be ready for merge.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] liyafan82 commented on a change in pull request #7347: ARROW-8230: [Java] Remove netty dependency from arrow-memory
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #7347:
URL: https://github.com/apache/arrow/pull/7347#discussion_r447363481
##########
File path: java/memory/src/main/java/org/apache/arrow/memory/rounding/DefaultRoundingPolicy.java
##########
@@ -17,33 +17,107 @@
package org.apache.arrow.memory.rounding;
-import java.lang.reflect.Field;
-
-import org.apache.arrow.memory.NettyAllocationManager;
import org.apache.arrow.memory.util.CommonUtil;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import io.netty.util.internal.SystemPropertyUtil;
/**
* The default rounding policy. That is, if the requested size is within the chunk size,
* the rounded size will be the next power of two. Otherwise, the rounded size
* will be identical to the requested size.
*/
public class DefaultRoundingPolicy implements RoundingPolicy {
-
+ private static final Logger logger = LoggerFactory.getLogger(DefaultRoundingPolicy.class);
public final long chunkSize;
/**
- * The singleton instance.
+ * The variables here and the static block calculates teh DEFAULT_CHUNK_SIZE.
+ *
+ * <p>
+ * It was copied from {@link io.netty.buffer.PooledByteBufAllocator}.
+ * </p>
*/
- public static final DefaultRoundingPolicy INSTANCE = new DefaultRoundingPolicy();
+ private static final int MIN_PAGE_SIZE = 4096;
+ private static final int MAX_CHUNK_SIZE = (int) (((long) Integer.MAX_VALUE + 1) / 2);
+ private static final long DEFAULT_CHUNK_SIZE;
+ private static final int DEFAULT_PAGE_SIZE;
+ private static final int DEFAULT_MAX_ORDER;
- private DefaultRoundingPolicy() {
+
+ static {
+ int defaultPageSize = SystemPropertyUtil.getInt("org.apache.memory.allocator.pageSize", 8192);
+ Throwable pageSizeFallbackCause = null;
try {
- Field field = NettyAllocationManager.class.getDeclaredField("CHUNK_SIZE");
- field.setAccessible(true);
- chunkSize = (Long) field.get(null);
- } catch (Exception e) {
- throw new RuntimeException("Failed to get chunk size from allocation manager");
+ validateAndCalculatePageShifts(defaultPageSize);
+ } catch (Throwable t) {
+ pageSizeFallbackCause = t;
+ defaultPageSize = 8192;
+ }
+ DEFAULT_PAGE_SIZE = defaultPageSize;
+
+ int defaultMaxOrder = SystemPropertyUtil.getInt("org.apache.memory.allocator.maxOrder", 11);
+ Throwable maxOrderFallbackCause = null;
+ try {
+ validateAndCalculateChunkSize(DEFAULT_PAGE_SIZE, defaultMaxOrder);
+ } catch (Throwable t) {
+ maxOrderFallbackCause = t;
+ defaultMaxOrder = 11;
+ }
+ DEFAULT_MAX_ORDER = defaultMaxOrder;
+ DEFAULT_CHUNK_SIZE = validateAndCalculateChunkSize(DEFAULT_PAGE_SIZE, DEFAULT_MAX_ORDER);
+ if (logger.isDebugEnabled()) {
+ if (pageSizeFallbackCause == null) {
+ logger.debug("-Dorg.apache.memory.allocator.pageSize: {}", DEFAULT_PAGE_SIZE);
+ } else {
+ logger.debug("-Dorg.apache.memory.allocator.pageSize: {}", DEFAULT_PAGE_SIZE, pageSizeFallbackCause);
+ }
+ if (maxOrderFallbackCause == null) {
+ logger.debug("-Dorg.apache.memory.allocator.maxOrder: {}", DEFAULT_MAX_ORDER);
+ } else {
+ logger.debug("-Dorg.apache.memory.allocator.maxOrder: {}", DEFAULT_MAX_ORDER, maxOrderFallbackCause);
+ }
Review comment:
Also here we need only one parameter in the "else" statement.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] liyafan82 commented on a change in pull request #7347: ARROW-8230: [Java] Remove netty dependency from arrow-memory
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #7347:
URL: https://github.com/apache/arrow/pull/7347#discussion_r446617744
##########
File path: java/memory/src/main/java/org/apache/arrow/memory/rounding/DefaultRoundingPolicy.java
##########
@@ -17,33 +17,107 @@
package org.apache.arrow.memory.rounding;
-import java.lang.reflect.Field;
-
-import org.apache.arrow.memory.NettyAllocationManager;
import org.apache.arrow.memory.util.CommonUtil;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import io.netty.util.internal.SystemPropertyUtil;
/**
* The default rounding policy. That is, if the requested size is within the chunk size,
* the rounded size will be the next power of two. Otherwise, the rounded size
* will be identical to the requested size.
*/
public class DefaultRoundingPolicy implements RoundingPolicy {
-
+ private static final Logger logger = LoggerFactory.getLogger(DefaultRoundingPolicy.class);
public final long chunkSize;
/**
- * The singleton instance.
+ * The variables here and the static block calculates teh DEFAULT_CHUNK_SIZE.
+ *
+ * <p>
+ * It was copied from {@link io.netty.buffer.PooledByteBufAllocator}.
+ * </p>
*/
- public static final DefaultRoundingPolicy INSTANCE = new DefaultRoundingPolicy();
+ private static final int MIN_PAGE_SIZE = 4096;
+ private static final int MAX_CHUNK_SIZE = (int) (((long) Integer.MAX_VALUE + 1) / 2);
+ private static final long DEFAULT_CHUNK_SIZE;
+ private static final int DEFAULT_PAGE_SIZE;
+ private static final int DEFAULT_MAX_ORDER;
- private DefaultRoundingPolicy() {
+
+ static {
+ int defaultPageSize = SystemPropertyUtil.getInt("org.apache.memory.allocator.pageSize", 8192);
+ Throwable pageSizeFallbackCause = null;
try {
- Field field = NettyAllocationManager.class.getDeclaredField("CHUNK_SIZE");
- field.setAccessible(true);
- chunkSize = (Long) field.get(null);
- } catch (Exception e) {
- throw new RuntimeException("Failed to get chunk size from allocation manager");
+ validateAndCalculatePageShifts(defaultPageSize);
+ } catch (Throwable t) {
+ pageSizeFallbackCause = t;
+ defaultPageSize = 8192;
+ }
+ DEFAULT_PAGE_SIZE = defaultPageSize;
+
+ int defaultMaxOrder = SystemPropertyUtil.getInt("org.apache.memory.allocator.maxOrder", 11);
+ Throwable maxOrderFallbackCause = null;
+ try {
+ validateAndCalculateChunkSize(DEFAULT_PAGE_SIZE, defaultMaxOrder);
+ } catch (Throwable t) {
+ maxOrderFallbackCause = t;
+ defaultMaxOrder = 11;
+ }
+ DEFAULT_MAX_ORDER = defaultMaxOrder;
+ DEFAULT_CHUNK_SIZE = validateAndCalculateChunkSize(DEFAULT_PAGE_SIZE, DEFAULT_MAX_ORDER);
+ if (logger.isDebugEnabled()) {
+ if (pageSizeFallbackCause == null) {
+ logger.debug("-Dorg.apache.memory.allocator.pageSize: {}", DEFAULT_PAGE_SIZE);
+ } else {
+ logger.debug("-Dorg.apache.memory.allocator.pageSize: {}", DEFAULT_PAGE_SIZE, pageSizeFallbackCause);
+ }
+ if (maxOrderFallbackCause == null) {
+ logger.debug("-Dorg.apache.memory.allocator.maxOrder: {}", DEFAULT_MAX_ORDER);
+ } else {
+ logger.debug("-Dorg.apache.memory.allocator.maxOrder: {}", DEFAULT_MAX_ORDER, maxOrderFallbackCause);
+ }
Review comment:
Please remove "DEFAULT_MAX_ORDER" here
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rymurr commented on a change in pull request #7347: ARROW-8230: [Java] Remove netty dependency from arrow-memory
Posted by GitBox <gi...@apache.org>.
rymurr commented on a change in pull request #7347:
URL: https://github.com/apache/arrow/pull/7347#discussion_r446920091
##########
File path: java/memory/src/main/java/org/apache/arrow/memory/util/MemoryUtil.java
##########
@@ -95,4 +143,26 @@ public static long getByteBufferAddress(ByteBuffer buf) {
private MemoryUtil() {
}
+
+ /**
+ * Create nio byte buffer.
+ */
+ public static ByteBuffer directBuffer(long address, int capacity) {
+ if (DIRECT_BUFFER_CONSTRUCTOR != null) {
+ if (capacity < 0) {
+ throw new IllegalArgumentException("Capacity is negative, has to be positive or 0");
+ }
+ try {
+ return (ByteBuffer) DIRECT_BUFFER_CONSTRUCTOR.newInstance(address, capacity);
+ } catch (Throwable cause) {
+ // Not expected to ever throw!
+ if (cause instanceof Error) {
+ throw (Error) cause;
Review comment:
None, only that it was how the netty developers wrote it. I thought I would copy their lead. However, I have simplified it now,
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] liyafan82 commented on pull request #7347: ARROW-8230: [Java] Remove netty dependency from arrow-memory
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on pull request #7347:
URL: https://github.com/apache/arrow/pull/7347#issuecomment-650739545
Mostly looks good to me. There are a few minor issues.
Since it involves some fundamental classes, could you please make sure our integration tests pass? @rymurr
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] liyafan82 commented on a change in pull request #7347: ARROW-8230: [Java] Remove netty dependency from arrow-memory
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #7347:
URL: https://github.com/apache/arrow/pull/7347#discussion_r446595068
##########
File path: java/memory/src/main/java/org/apache/arrow/memory/rounding/DefaultRoundingPolicy.java
##########
@@ -17,33 +17,107 @@
package org.apache.arrow.memory.rounding;
-import java.lang.reflect.Field;
-
-import org.apache.arrow.memory.NettyAllocationManager;
import org.apache.arrow.memory.util.CommonUtil;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import io.netty.util.internal.SystemPropertyUtil;
/**
* The default rounding policy. That is, if the requested size is within the chunk size,
* the rounded size will be the next power of two. Otherwise, the rounded size
* will be identical to the requested size.
*/
public class DefaultRoundingPolicy implements RoundingPolicy {
-
+ private static final Logger logger = LoggerFactory.getLogger(DefaultRoundingPolicy.class);
public final long chunkSize;
/**
- * The singleton instance.
+ * The variables here and the static block calculates teh DEFAULT_CHUNK_SIZE.
Review comment:
teh -> the
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] liyafan82 commented on a change in pull request #7347: ARROW-8230: [Java] Remove netty dependency from arrow-memory
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #7347:
URL: https://github.com/apache/arrow/pull/7347#discussion_r441929412
##########
File path: java/memory/src/main/java/io/netty/buffer/NettyArrowBuf.java
##########
@@ -404,7 +407,7 @@ protected int _getUnsignedMediumLE(int index) {
this.chk(index, 3);
long addr = this.addr(index);
return PlatformDependent.getByte(addr) & 255 |
- (Short.reverseBytes(PlatformDependent.getShort(addr + 1L)) & '\uffff') << 8;
+ (Short.reverseBytes(PlatformDependent.getShort(addr + 1L)) & '\uffff') << 8;
Review comment:
Thanks for your effort.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] liyafan82 commented on a change in pull request #7347: ARROW-8230: [Java] Remove netty dependency from arrow-memory
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #7347:
URL: https://github.com/apache/arrow/pull/7347#discussion_r441931433
##########
File path: java/memory/src/main/java/org/apache/arrow/memory/rounding/DefaultRoundingPolicy.java
##########
@@ -31,19 +28,18 @@
public final long chunkSize;
+ /**
+ * default chunk size from Netty.
+ */
+ private static final long DEFAULT_CHUNK_SIZE = 16777216L;
Review comment:
Maybe it is beneficial to add above discussion to the JavaDoc
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rymurr commented on pull request #7347: ARROW-8230: [Java] Remove netty dependency from arrow-memory
Posted by GitBox <gi...@apache.org>.
rymurr commented on pull request #7347:
URL: https://github.com/apache/arrow/pull/7347#issuecomment-652291140
Thanks a lot @liyafan82 I have addressed your suggestions and rebased
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] liyafan82 commented on a change in pull request #7347: ARROW-8230: [Java] Remove netty dependency from arrow-memory
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #7347:
URL: https://github.com/apache/arrow/pull/7347#discussion_r447363293
##########
File path: java/memory/src/main/java/org/apache/arrow/memory/rounding/DefaultRoundingPolicy.java
##########
@@ -17,33 +17,103 @@
package org.apache.arrow.memory.rounding;
-import java.lang.reflect.Field;
-
-import org.apache.arrow.memory.NettyAllocationManager;
import org.apache.arrow.memory.util.CommonUtil;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import io.netty.util.internal.SystemPropertyUtil;
/**
* The default rounding policy. That is, if the requested size is within the chunk size,
* the rounded size will be the next power of two. Otherwise, the rounded size
* will be identical to the requested size.
*/
public class DefaultRoundingPolicy implements RoundingPolicy {
-
+ private static final Logger logger = LoggerFactory.getLogger(DefaultRoundingPolicy.class);
public final long chunkSize;
/**
- * The singleton instance.
+ * The variables here and the static block calculates the DEFAULT_CHUNK_SIZE.
+ *
+ * <p>
+ * It was copied from {@link io.netty.buffer.PooledByteBufAllocator}.
+ * </p>
*/
- public static final DefaultRoundingPolicy INSTANCE = new DefaultRoundingPolicy();
+ private static final int MIN_PAGE_SIZE = 4096;
+ private static final int MAX_CHUNK_SIZE = (int) (((long) Integer.MAX_VALUE + 1) / 2);
+ private static final long DEFAULT_CHUNK_SIZE;
- private DefaultRoundingPolicy() {
+
+ static {
+ int defaultPageSize = SystemPropertyUtil.getInt("org.apache.memory.allocator.pageSize", 8192);
+ Throwable pageSizeFallbackCause = null;
try {
- Field field = NettyAllocationManager.class.getDeclaredField("CHUNK_SIZE");
- field.setAccessible(true);
- chunkSize = (Long) field.get(null);
- } catch (Exception e) {
- throw new RuntimeException("Failed to get chunk size from allocation manager");
+ validateAndCalculatePageShifts(defaultPageSize);
+ } catch (Throwable t) {
+ pageSizeFallbackCause = t;
+ defaultPageSize = 8192;
+ }
+
+ int defaultMaxOrder = SystemPropertyUtil.getInt("org.apache.memory.allocator.maxOrder", 11);
+ Throwable maxOrderFallbackCause = null;
+ try {
+ validateAndCalculateChunkSize(defaultPageSize, defaultMaxOrder);
+ } catch (Throwable t) {
+ maxOrderFallbackCause = t;
+ defaultMaxOrder = 11;
+ }
+ DEFAULT_CHUNK_SIZE = validateAndCalculateChunkSize(defaultPageSize, defaultMaxOrder);
+ if (logger.isDebugEnabled()) {
+ if (pageSizeFallbackCause == null) {
+ logger.debug("-Dorg.apache.memory.allocator.pageSize: {}", defaultPageSize);
+ } else {
+ logger.debug("-Dorg.apache.memory.allocator.pageSize: {}", defaultPageSize, pageSizeFallbackCause);
+ }
Review comment:
I think here we only need one parameter here? So it should be
logger.debug("-Dorg.apache.memory.allocator.pageSize: {}", pageSizeFallbackCause);
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rymurr commented on pull request #7347: ARROW-8230: [Java] Remove netty dependency from arrow-memory
Posted by GitBox <gi...@apache.org>.
rymurr commented on pull request #7347:
URL: https://github.com/apache/arrow/pull/7347#issuecomment-651081660
looks like an ongoing github incident is causing build failures. Will rebase and rebuild once Github is back to normal
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rymurr commented on a change in pull request #7347: ARROW-8230: [Java] Remove netty dependency from arrow-memory
Posted by GitBox <gi...@apache.org>.
rymurr commented on a change in pull request #7347:
URL: https://github.com/apache/arrow/pull/7347#discussion_r440899130
##########
File path: java/memory/src/main/java/org/apache/arrow/memory/util/MemoryUtil.java
##########
@@ -78,6 +77,55 @@ public Object run() {
Field addressField = java.nio.Buffer.class.getDeclaredField("address");
addressField.setAccessible(true);
BYTE_BUFFER_ADDRESS_OFFSET = UNSAFE.objectFieldOffset(addressField);
+
+ Constructor<?> directBufferConstructor;
+ long address = -1;
+ final ByteBuffer direct = ByteBuffer.allocateDirect(1);
+ try {
+
+ final Object maybeDirectBufferConstructor =
+ AccessController.doPrivileged(new PrivilegedAction<Object>() {
+ @Override
+ public Object run() {
+ try {
+ final Constructor<?> constructor =
+ direct.getClass().getDeclaredConstructor(long.class, int.class);
+ constructor.setAccessible(true);
+ return constructor;
+ } catch (NoSuchMethodException e) {
+ return e;
Review comment:
done! thanks!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] liyafan82 commented on a change in pull request #7347: ARROW-8230: [Java] Remove netty dependency from arrow-memory
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #7347:
URL: https://github.com/apache/arrow/pull/7347#discussion_r441930746
##########
File path: java/memory/src/main/java/org/apache/arrow/memory/util/MemoryUtil.java
##########
@@ -78,6 +77,55 @@ public Object run() {
Field addressField = java.nio.Buffer.class.getDeclaredField("address");
addressField.setAccessible(true);
BYTE_BUFFER_ADDRESS_OFFSET = UNSAFE.objectFieldOffset(addressField);
+
+ Constructor<?> directBufferConstructor;
+ long address = -1;
+ final ByteBuffer direct = ByteBuffer.allocateDirect(1);
+ try {
+
+ final Object maybeDirectBufferConstructor =
+ AccessController.doPrivileged(new PrivilegedAction<Object>() {
+ @Override
+ public Object run() {
+ try {
+ final Constructor<?> constructor =
+ direct.getClass().getDeclaredConstructor(long.class, int.class);
+ constructor.setAccessible(true);
+ return constructor;
+ } catch (NoSuchMethodException e) {
+ return e;
+ } catch (SecurityException e) {
+ return e;
+ }
+ }
+ });
+
+ if (maybeDirectBufferConstructor instanceof Constructor<?>) {
+ address = UNSAFE.allocateMemory(1);
+ // try to use the constructor now
+ try {
+ ((Constructor<?>) maybeDirectBufferConstructor).newInstance(address, 1);
+ directBufferConstructor = (Constructor<?>) maybeDirectBufferConstructor;
+ logger.debug("direct buffer constructor: available");
+ } catch (InstantiationException e) {
+ directBufferConstructor = null;
Review comment:
looks good. thanks.
since the exception handling logic is identical, does it make the code conciser to use the following semantics?
```
catch (InstantiationException | IllegalAccessException | InvocationTargetException e) {
...
}
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rymurr commented on a change in pull request #7347: ARROW-8230: [Java] Remove netty dependency from arrow-memory
Posted by GitBox <gi...@apache.org>.
rymurr commented on a change in pull request #7347:
URL: https://github.com/apache/arrow/pull/7347#discussion_r440896771
##########
File path: java/memory/src/main/java/org/apache/arrow/memory/util/MemoryUtil.java
##########
@@ -78,6 +77,55 @@ public Object run() {
Field addressField = java.nio.Buffer.class.getDeclaredField("address");
addressField.setAccessible(true);
BYTE_BUFFER_ADDRESS_OFFSET = UNSAFE.objectFieldOffset(addressField);
+
+ Constructor<?> directBufferConstructor;
+ long address = -1;
+ final ByteBuffer direct = ByteBuffer.allocateDirect(1);
+ try {
+
+ final Object maybeDirectBufferConstructor =
+ AccessController.doPrivileged(new PrivilegedAction<Object>() {
+ @Override
+ public Object run() {
+ try {
+ final Constructor<?> constructor =
+ direct.getClass().getDeclaredConstructor(long.class, int.class);
+ constructor.setAccessible(true);
+ return constructor;
+ } catch (NoSuchMethodException e) {
+ return e;
+ } catch (SecurityException e) {
+ return e;
+ }
+ }
+ });
+
+ if (maybeDirectBufferConstructor instanceof Constructor<?>) {
+ address = UNSAFE.allocateMemory(1);
+ // try to use the constructor now
+ try {
+ ((Constructor<?>) maybeDirectBufferConstructor).newInstance(address, 1);
+ directBufferConstructor = (Constructor<?>) maybeDirectBufferConstructor;
+ logger.debug("direct buffer constructor: available");
+ } catch (InstantiationException e) {
+ directBufferConstructor = null;
Review comment:
sure, any suggestions where? Note: This is taken from PlatformDependent0 in netty (logs and all).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rymurr commented on a change in pull request #7347: ARROW-8230: [Java] Remove netty dependency from arrow-memory
Posted by GitBox <gi...@apache.org>.
rymurr commented on a change in pull request #7347:
URL: https://github.com/apache/arrow/pull/7347#discussion_r446920388
##########
File path: java/memory/src/main/java/org/apache/arrow/memory/util/MemoryUtil.java
##########
@@ -78,6 +77,55 @@ public Object run() {
Field addressField = java.nio.Buffer.class.getDeclaredField("address");
addressField.setAccessible(true);
BYTE_BUFFER_ADDRESS_OFFSET = UNSAFE.objectFieldOffset(addressField);
+
+ Constructor<?> directBufferConstructor;
+ long address = -1;
+ final ByteBuffer direct = ByteBuffer.allocateDirect(1);
+ try {
+
+ final Object maybeDirectBufferConstructor =
+ AccessController.doPrivileged(new PrivilegedAction<Object>() {
+ @Override
+ public Object run() {
+ try {
+ final Constructor<?> constructor =
+ direct.getClass().getDeclaredConstructor(long.class, int.class);
+ constructor.setAccessible(true);
+ logger.debug("Constructor for direct buffer found and made accessible");
+ return constructor;
+ } catch (NoSuchMethodException e) {
+ logger.debug("Cannot get constructor for direct buffer allocation", e);
+ return e;
+ } catch (SecurityException e) {
+ logger.debug("Cannot get constructor for direct buffer allocation", e);
+ return e;
+ }
+ }
+ });
+
+ if (maybeDirectBufferConstructor instanceof Constructor<?>) {
+ address = UNSAFE.allocateMemory(1);
+ // try to use the constructor now
+ try {
+ ((Constructor<?>) maybeDirectBufferConstructor).newInstance(address, 1);
+ directBufferConstructor = (Constructor<?>) maybeDirectBufferConstructor;
+ logger.debug("direct buffer constructor: available");
+ } catch (InstantiationException | IllegalAccessException | InvocationTargetException e) {
+ logger.debug("unable to instantiate a direct buffer via constructor", e);
Review comment:
fixed
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] liyafan82 commented on a change in pull request #7347: ARROW-8230: [Java] Remove netty dependency from arrow-memory
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #7347:
URL: https://github.com/apache/arrow/pull/7347#discussion_r446626385
##########
File path: java/memory/src/main/java/org/apache/arrow/memory/util/MemoryUtil.java
##########
@@ -78,6 +77,55 @@ public Object run() {
Field addressField = java.nio.Buffer.class.getDeclaredField("address");
addressField.setAccessible(true);
BYTE_BUFFER_ADDRESS_OFFSET = UNSAFE.objectFieldOffset(addressField);
+
+ Constructor<?> directBufferConstructor;
+ long address = -1;
+ final ByteBuffer direct = ByteBuffer.allocateDirect(1);
+ try {
+
+ final Object maybeDirectBufferConstructor =
+ AccessController.doPrivileged(new PrivilegedAction<Object>() {
+ @Override
+ public Object run() {
+ try {
+ final Constructor<?> constructor =
+ direct.getClass().getDeclaredConstructor(long.class, int.class);
+ constructor.setAccessible(true);
+ logger.debug("Constructor for direct buffer found and made accessible");
+ return constructor;
+ } catch (NoSuchMethodException e) {
+ logger.debug("Cannot get constructor for direct buffer allocation", e);
+ return e;
+ } catch (SecurityException e) {
+ logger.debug("Cannot get constructor for direct buffer allocation", e);
+ return e;
+ }
+ }
+ });
+
+ if (maybeDirectBufferConstructor instanceof Constructor<?>) {
+ address = UNSAFE.allocateMemory(1);
+ // try to use the constructor now
+ try {
+ ((Constructor<?>) maybeDirectBufferConstructor).newInstance(address, 1);
+ directBufferConstructor = (Constructor<?>) maybeDirectBufferConstructor;
+ logger.debug("direct buffer constructor: available");
+ } catch (InstantiationException | IllegalAccessException | InvocationTargetException e) {
+ logger.debug("unable to instantiate a direct buffer via constructor", e);
Review comment:
here the log level should be warning or error?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] liyafan82 commented on pull request #7347: ARROW-8230: [Java] Remove netty dependency from arrow-memory
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on pull request #7347:
URL: https://github.com/apache/arrow/pull/7347#issuecomment-652739238
> @liyafan82 if you aren't already please make sure you use the merge script under dev to merge PRs
@emkornfield Thanks a lot for your kind reminder. I will use the script next time.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rymurr commented on a change in pull request #7347: ARROW-8230: [Java] Remove netty dependency from arrow-memory
Posted by GitBox <gi...@apache.org>.
rymurr commented on a change in pull request #7347:
URL: https://github.com/apache/arrow/pull/7347#discussion_r440906089
##########
File path: java/memory/src/main/java/org/apache/arrow/memory/rounding/DefaultRoundingPolicy.java
##########
@@ -31,19 +28,18 @@
public final long chunkSize;
+ /**
+ * default chunk size from Netty.
+ */
+ private static final long DEFAULT_CHUNK_SIZE = 16777216L;
Review comment:
It is possible that it changes. It was a bit involved to copy the Netty code to get chunk size into the arrow codebase so I left it at a likely value for x86-64 machines. I am happy to replace this with the Netty value or have chunk size taken from the actual allocator directly if you thikn that is better than a single static value?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] liyafan82 commented on a change in pull request #7347: ARROW-8230: [Java] Remove netty dependency from arrow-memory
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #7347:
URL: https://github.com/apache/arrow/pull/7347#discussion_r446617676
##########
File path: java/memory/src/main/java/org/apache/arrow/memory/rounding/DefaultRoundingPolicy.java
##########
@@ -17,33 +17,107 @@
package org.apache.arrow.memory.rounding;
-import java.lang.reflect.Field;
-
-import org.apache.arrow.memory.NettyAllocationManager;
import org.apache.arrow.memory.util.CommonUtil;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import io.netty.util.internal.SystemPropertyUtil;
/**
* The default rounding policy. That is, if the requested size is within the chunk size,
* the rounded size will be the next power of two. Otherwise, the rounded size
* will be identical to the requested size.
*/
public class DefaultRoundingPolicy implements RoundingPolicy {
-
+ private static final Logger logger = LoggerFactory.getLogger(DefaultRoundingPolicy.class);
public final long chunkSize;
/**
- * The singleton instance.
+ * The variables here and the static block calculates teh DEFAULT_CHUNK_SIZE.
+ *
+ * <p>
+ * It was copied from {@link io.netty.buffer.PooledByteBufAllocator}.
+ * </p>
*/
- public static final DefaultRoundingPolicy INSTANCE = new DefaultRoundingPolicy();
+ private static final int MIN_PAGE_SIZE = 4096;
+ private static final int MAX_CHUNK_SIZE = (int) (((long) Integer.MAX_VALUE + 1) / 2);
+ private static final long DEFAULT_CHUNK_SIZE;
+ private static final int DEFAULT_PAGE_SIZE;
+ private static final int DEFAULT_MAX_ORDER;
- private DefaultRoundingPolicy() {
+
+ static {
+ int defaultPageSize = SystemPropertyUtil.getInt("org.apache.memory.allocator.pageSize", 8192);
+ Throwable pageSizeFallbackCause = null;
try {
- Field field = NettyAllocationManager.class.getDeclaredField("CHUNK_SIZE");
- field.setAccessible(true);
- chunkSize = (Long) field.get(null);
- } catch (Exception e) {
- throw new RuntimeException("Failed to get chunk size from allocation manager");
+ validateAndCalculatePageShifts(defaultPageSize);
+ } catch (Throwable t) {
+ pageSizeFallbackCause = t;
+ defaultPageSize = 8192;
+ }
+ DEFAULT_PAGE_SIZE = defaultPageSize;
+
+ int defaultMaxOrder = SystemPropertyUtil.getInt("org.apache.memory.allocator.maxOrder", 11);
+ Throwable maxOrderFallbackCause = null;
+ try {
+ validateAndCalculateChunkSize(DEFAULT_PAGE_SIZE, defaultMaxOrder);
+ } catch (Throwable t) {
+ maxOrderFallbackCause = t;
+ defaultMaxOrder = 11;
+ }
+ DEFAULT_MAX_ORDER = defaultMaxOrder;
+ DEFAULT_CHUNK_SIZE = validateAndCalculateChunkSize(DEFAULT_PAGE_SIZE, DEFAULT_MAX_ORDER);
+ if (logger.isDebugEnabled()) {
+ if (pageSizeFallbackCause == null) {
+ logger.debug("-Dorg.apache.memory.allocator.pageSize: {}", DEFAULT_PAGE_SIZE);
+ } else {
+ logger.debug("-Dorg.apache.memory.allocator.pageSize: {}", DEFAULT_PAGE_SIZE, pageSizeFallbackCause);
Review comment:
please remove "DEFAULT_PAGE_SIZE" here
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rymurr commented on a change in pull request #7347: ARROW-8230: [Java] Remove netty dependency from arrow-memory
Posted by GitBox <gi...@apache.org>.
rymurr commented on a change in pull request #7347:
URL: https://github.com/apache/arrow/pull/7347#discussion_r440907579
##########
File path: java/memory/src/main/java/io/netty/buffer/NettyArrowBuf.java
##########
@@ -404,7 +407,7 @@ protected int _getUnsignedMediumLE(int index) {
this.chk(index, 3);
long addr = this.addr(index);
return PlatformDependent.getByte(addr) & 255 |
- (Short.reverseBytes(PlatformDependent.getShort(addr + 1L)) & '\uffff') << 8;
+ (Short.reverseBytes(PlatformDependent.getShort(addr + 1L)) & '\uffff') << 8;
Review comment:
I have added https://issues.apache.org/jira/browse/ARROW-9148 to address this
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] liyafan82 commented on a change in pull request #7347: ARROW-8230: [Java] Remove netty dependency from arrow-memory
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #7347:
URL: https://github.com/apache/arrow/pull/7347#discussion_r446639368
##########
File path: java/memory/src/main/java/org/apache/arrow/memory/util/MemoryUtil.java
##########
@@ -95,4 +143,26 @@ public static long getByteBufferAddress(ByteBuffer buf) {
private MemoryUtil() {
}
+
+ /**
+ * Create nio byte buffer.
+ */
+ public static ByteBuffer directBuffer(long address, int capacity) {
+ if (DIRECT_BUFFER_CONSTRUCTOR != null) {
+ if (capacity < 0) {
+ throw new IllegalArgumentException("Capacity is negative, has to be positive or 0");
+ }
+ try {
+ return (ByteBuffer) DIRECT_BUFFER_CONSTRUCTOR.newInstance(address, capacity);
+ } catch (Throwable cause) {
+ // Not expected to ever throw!
+ if (cause instanceof Error) {
+ throw (Error) cause;
Review comment:
Is there a special reason for the cast here?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] liyafan82 commented on a change in pull request #7347: ARROW-8230: [Java] Remove netty dependency from arrow-memory
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #7347:
URL: https://github.com/apache/arrow/pull/7347#discussion_r447363600
##########
File path: java/memory/src/main/java/org/apache/arrow/memory/ArrowBuf.java
##########
@@ -227,13 +207,25 @@ public ArrowBuf slice(long index, long length) {
return newBuf;
}
+ /**
+ * Make a nio byte buffer from this arrowbuf.
+ */
public ByteBuffer nioBuffer() {
- return asNettyBuffer().nioBuffer();
+ return nioBuffer(readerIndex, checkedCastToInt(readableBytes()));
}
+
+ /**
+ * Make an nio byte buffer from this ArrowBuf.
Review comment:
nit: an -> a
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rymurr commented on pull request #7347: ARROW-8230: [Java] Remove netty dependency from arrow-memory
Posted by GitBox <gi...@apache.org>.
rymurr commented on pull request #7347:
URL: https://github.com/apache/arrow/pull/7347#issuecomment-649372476
Thanks for the reviews @liyafan82 I have updated based on your comments. Please let me know what you think!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] liyafan82 commented on pull request #7347: ARROW-8230: [Java] Remove netty dependency from arrow-memory
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on pull request #7347:
URL: https://github.com/apache/arrow/pull/7347#issuecomment-652319908
> Thanks a lot @liyafan82 I have addressed your suggestions and rebased
@rymurr Thanks for your work. Will merge when it turns green.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] emkornfield commented on pull request #7347: ARROW-8230: [Java] Remove netty dependency from arrow-memory
Posted by GitBox <gi...@apache.org>.
emkornfield commented on pull request #7347:
URL: https://github.com/apache/arrow/pull/7347#issuecomment-652738045
@liyafan82 if you aren't already please make sure you use the merge script under dev to merge PRs
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org