franz1981 edited a comment on issue #2844: ARTEMIS-1811 NIO Seq File should use 
RandomAccessFile with heap buffers
URL: https://github.com/apache/activemq-artemis/pull/2844#issuecomment-533606931
 
 
   @wy96f @clebertsuconic 
   Just for completeness that's what would change:
   
   - 
[Java_java_io_RandomAccessFile_writeBytes->writeBytes](https://github.com/frohoff/jdk8u-jdk/blob/da0da73ab82ed714dc5be94acd2f0d00fbdfe2e9/src/share/native/java/io/RandomAccessFile.c#L85)
   - 
[writeBytes->IO_Write](https://github.com/frohoff/jdk8u-jdk/blob/da0da73ab82ed714dc5be94acd2f0d00fbdfe2e9/src/share/native/java/io/io_util.c#L189)
 by using a stack buffer if write/read length is <= `BUF_SIZE` (ie 8192 bytes) 
or with a fresh new buffer allocated using malloc/free: in both cases 
GetByteArrayRegion/SetByteArrayRegion are used to perform a copy from/to the 
provided java `byte[]`
   - [IO_Write === 
handleWrite](https://github.com/frohoff/jdk8u-jdk/blob/da0da73ab82ed714dc5be94acd2f0d00fbdfe2e9/src/solaris/native/java/io/io_util_md.h#L71)
   - 
[handleWrite->write](https://github.com/frohoff/jdk8u-jdk/blob/da0da73ab82ed714dc5be94acd2f0d00fbdfe2e9/src/solaris/native/java/io/io_util_md.c#L164)
   
   I see that this PR has few advantages vs 
https://github.com/apache/activemq-artemis/pull/2832:
   
   - it is simpler/less impactfull on artemis code base
   - although copies always happen, no leaks can happen
   
   I see that for small writes/reads the perf hit is not very high because copy 
java byte[]<->stack allocated native byte[] copy is cheap if compared to the 
cost of the write/read syscall and won't impact scalability of the native 
allocator, because malloc/free isn't used.
   
   I'm worried, because this change would enable a non-transparent handling of 
large writes/reads on JNI that could silently kill performance due to 
`malloc/free` + the large copy: that's exactly our use case for OpenWire and 
AMQP (until we'll get streaming of large messages in) and sadly it would affect 
CORE too, given that CORE writes/reads in 100KB sized chunks, that's >  
`BUF_SIZE` (ie 8192 bytes).
   
   PLEASE DO NOT MERGE: I would like to discuss with you about it :)
   
   I've added 
https://github.com/apache/activemq-artemis/pull/2844/commits/453c9796a486e6b9289e8cc8aa61e1ebcf41fd9f
 trying to mitigate the effects of 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

Reply via email to