[GitHub] [activemq-artemis] franz1981 commented on a change in pull request #2844: ARTEMIS-1811 NIO Seq File should use RandomAccessFile with heap buffers
franz1981 commented on a change in pull request #2844: ARTEMIS-1811 NIO Seq File should use RandomAccessFile with heap buffers URL: https://github.com/apache/activemq-artemis/pull/2844#discussion_r327938663 ## File path: artemis-journal/src/main/java/org/apache/activemq/artemis/core/io/nio/NIOSequentialFile.java ## @@ -160,14 +173,63 @@ public int read(final ByteBuffer bytes) throws Exception { return read(bytes, null); } + //This value has been tuned just to reduce the memory footprint + //of read/write of the whole file size: given that this value + //is > 8192, RandomAccessFile JNI code will use malloc/free instead + //of using a copy on the stack, but it has been proven to NOT be + //a bottleneck + private static final int BUF_SIZE = 2 * 1024 * 1024; + + private static int readRaf(RandomAccessFile raf, byte[] b, int off, int len, int ioSize) throws IOException { + int remaining = len; + int offset = off; + while (remaining > 0) { + final int chunkSize = Math.min(ioSize, remaining); + final int read = raf.read(b, offset, chunkSize); + assert read != 0; + if (read == -1) { +if (len == remaining) { + return -1; +} +break; + } + offset += read; + remaining -= read; + } + return len - remaining; + } + + private static void writeRaf(RandomAccessFile raf, byte[] b, int off, int len, int ioSize) throws IOException { + int remaining = len; + int offset = off; + while (remaining > 0) { + final int chunkSize = Math.min(ioSize, remaining); + raf.write(b, offset, chunkSize); + offset += chunkSize; + remaining -= chunkSize; + } + } + @Override public synchronized int read(final ByteBuffer bytes, final IOCallback callback) throws IOException, ActiveMQIllegalStateException { try { if (channel == null) { throw new ActiveMQIllegalStateException("File " + this.getFileName() + " has a null channel"); } - int bytesRead = channel.read(bytes); + final int bytesRead; + if (bytes.hasArray()) { +if (bytes.remaining() > BUF_SIZE) { + bytesRead = readRaf(rfile, bytes.array(), bytes.arrayOffset() + bytes.position(), bytes.remaining(), BUF_SIZE); +} else { + bytesRead = rfile.read(bytes.array(), bytes.arrayOffset() + bytes.position(), bytes.remaining()); Review comment: to save inlining budget: for most of the cases where the first check will save the call on the happy path 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] franz1981 commented on a change in pull request #2844: ARTEMIS-1811 NIO Seq File should use RandomAccessFile with heap buffers
franz1981 commented on a change in pull request #2844: ARTEMIS-1811 NIO Seq File should use RandomAccessFile with heap buffers URL: https://github.com/apache/activemq-artemis/pull/2844#discussion_r327938663 ## File path: artemis-journal/src/main/java/org/apache/activemq/artemis/core/io/nio/NIOSequentialFile.java ## @@ -160,14 +173,63 @@ public int read(final ByteBuffer bytes) throws Exception { return read(bytes, null); } + //This value has been tuned just to reduce the memory footprint + //of read/write of the whole file size: given that this value + //is > 8192, RandomAccessFile JNI code will use malloc/free instead + //of using a copy on the stack, but it has been proven to NOT be + //a bottleneck + private static final int BUF_SIZE = 2 * 1024 * 1024; + + private static int readRaf(RandomAccessFile raf, byte[] b, int off, int len, int ioSize) throws IOException { + int remaining = len; + int offset = off; + while (remaining > 0) { + final int chunkSize = Math.min(ioSize, remaining); + final int read = raf.read(b, offset, chunkSize); + assert read != 0; + if (read == -1) { +if (len == remaining) { + return -1; +} +break; + } + offset += read; + remaining -= read; + } + return len - remaining; + } + + private static void writeRaf(RandomAccessFile raf, byte[] b, int off, int len, int ioSize) throws IOException { + int remaining = len; + int offset = off; + while (remaining > 0) { + final int chunkSize = Math.min(ioSize, remaining); + raf.write(b, offset, chunkSize); + offset += chunkSize; + remaining -= chunkSize; + } + } + @Override public synchronized int read(final ByteBuffer bytes, final IOCallback callback) throws IOException, ActiveMQIllegalStateException { try { if (channel == null) { throw new ActiveMQIllegalStateException("File " + this.getFileName() + " has a null channel"); } - int bytesRead = channel.read(bytes); + final int bytesRead; + if (bytes.hasArray()) { +if (bytes.remaining() > BUF_SIZE) { + bytesRead = readRaf(rfile, bytes.array(), bytes.arrayOffset() + bytes.position(), bytes.remaining(), BUF_SIZE); +} else { + bytesRead = rfile.read(bytes.array(), bytes.arrayOffset() + bytes.position(), bytes.remaining()); Review comment: to save inlining budget: for most of the cases the first check will save the call on the happy path 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] franz1981 commented on a change in pull request #2844: ARTEMIS-1811 NIO Seq File should use RandomAccessFile with heap buffers
franz1981 commented on a change in pull request #2844: ARTEMIS-1811 NIO Seq File should use RandomAccessFile with heap buffers URL: https://github.com/apache/activemq-artemis/pull/2844#discussion_r327938663 ## File path: artemis-journal/src/main/java/org/apache/activemq/artemis/core/io/nio/NIOSequentialFile.java ## @@ -160,14 +173,63 @@ public int read(final ByteBuffer bytes) throws Exception { return read(bytes, null); } + //This value has been tuned just to reduce the memory footprint + //of read/write of the whole file size: given that this value + //is > 8192, RandomAccessFile JNI code will use malloc/free instead + //of using a copy on the stack, but it has been proven to NOT be + //a bottleneck + private static final int BUF_SIZE = 2 * 1024 * 1024; + + private static int readRaf(RandomAccessFile raf, byte[] b, int off, int len, int ioSize) throws IOException { + int remaining = len; + int offset = off; + while (remaining > 0) { + final int chunkSize = Math.min(ioSize, remaining); + final int read = raf.read(b, offset, chunkSize); + assert read != 0; + if (read == -1) { +if (len == remaining) { + return -1; +} +break; + } + offset += read; + remaining -= read; + } + return len - remaining; + } + + private static void writeRaf(RandomAccessFile raf, byte[] b, int off, int len, int ioSize) throws IOException { + int remaining = len; + int offset = off; + while (remaining > 0) { + final int chunkSize = Math.min(ioSize, remaining); + raf.write(b, offset, chunkSize); + offset += chunkSize; + remaining -= chunkSize; + } + } + @Override public synchronized int read(final ByteBuffer bytes, final IOCallback callback) throws IOException, ActiveMQIllegalStateException { try { if (channel == null) { throw new ActiveMQIllegalStateException("File " + this.getFileName() + " has a null channel"); } - int bytesRead = channel.read(bytes); + final int bytesRead; + if (bytes.hasArray()) { +if (bytes.remaining() > BUF_SIZE) { + bytesRead = readRaf(rfile, bytes.array(), bytes.arrayOffset() + bytes.position(), bytes.remaining(), BUF_SIZE); +} else { + bytesRead = rfile.read(bytes.array(), bytes.arrayOffset() + bytes.position(), bytes.remaining()); Review comment: to save inlining budget: for most of the cases the first check will safe the call on the happy path 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] franz1981 commented on a change in pull request #2844: ARTEMIS-1811 NIO Seq File should use RandomAccessFile with heap buffers
franz1981 commented on a change in pull request #2844: ARTEMIS-1811 NIO Seq File should use RandomAccessFile with heap buffers URL: https://github.com/apache/activemq-artemis/pull/2844#discussion_r326859418 ## File path: artemis-journal/src/main/java/org/apache/activemq/artemis/core/io/nio/NIOSequentialFile.java ## @@ -173,6 +173,40 @@ public int read(final ByteBuffer bytes) throws Exception { return read(bytes, null); } + //On *nix platform is the size by which RandomAccessFile write/read are performed using a + //stack-allocated copy of the java byte[] Review comment: I've verified that is a the same behaviour for Windows: it is in the shared JNI code by any OSs 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] franz1981 commented on a change in pull request #2844: ARTEMIS-1811 NIO Seq File should use RandomAccessFile with heap buffers
franz1981 commented on a change in pull request #2844: ARTEMIS-1811 NIO Seq File should use RandomAccessFile with heap buffers URL: https://github.com/apache/activemq-artemis/pull/2844#discussion_r326849697 ## File path: artemis-journal/src/main/java/org/apache/activemq/artemis/core/io/nio/NIOSequentialFile.java ## @@ -173,6 +173,40 @@ public int read(final ByteBuffer bytes) throws Exception { return read(bytes, null); } + //On *nix platform is the size by which RandomAccessFile write/read are performed using a + //stack-allocated copy of the java byte[] Review comment: This was just a POC commit to mitigate the issue , but we can discuss on the comments about it :) Thanks to looking at it 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: us...@infra.apache.org With regards, Apache Git Services