[GitHub] [activemq-artemis] franz1981 commented on issue #2832: ARTEMIS-2482 Large messages could leak native ByteBuffers

2019-09-16 Thread GitBox
franz1981 commented on issue #2832: ARTEMIS-2482 Large messages could leak 
native ByteBuffers
URL: https://github.com/apache/activemq-artemis/pull/2832#issuecomment-531690229
 
 
   @wy96f fair enough :)
   Still waiting some results from the integration tests on the CI and will be 
ready to be merged, although I prefer to improve code quality of this PR, there 
are some changes due to JDBC that makes is uglier than I wished


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 issue #2832: ARTEMIS-2482 Large messages could leak native ByteBuffers

2019-09-16 Thread GitBox
franz1981 commented on issue #2832: ARTEMIS-2482 Large messages could leak 
native ByteBuffers
URL: https://github.com/apache/activemq-artemis/pull/2832#issuecomment-531656312
 
 
   @wy96f 
   > So the cache will be freed and used by other threads when thread dies?
   
   Yes, it is, but my concern is that it needs a finalization run before 
getting there ie not that deterministic as I wish :)
   


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 issue #2832: ARTEMIS-2482 Large messages could leak native ByteBuffers

2019-09-11 Thread GitBox
franz1981 commented on issue #2832: ARTEMIS-2482 Large messages could leak 
native ByteBuffers
URL: https://github.com/apache/activemq-artemis/pull/2832#issuecomment-530319911
 
 
   @wy96f I've taken a second look to the way Netty handle thread local pools 
and I can confirm that `-Dio.netty.allocator.useCacheForAllThreads=false` 
should save non-netty threads from creating such thread local regions for 
tiny/small/medium sized buffers.
   While related to 
   > but they just contain wrappers to direct memory that cannot be released 
(is part of the pool and they have no cleaner). The impact should be way less 
then IOUtil that pool native ByteBuffers holding exclusively native memory that 
won't be reused anymore...
   
   I have to add more detail and I was wrong: the tiny/small/medium thread 
local caches are actually leaking memory regions (with default 
`io.netty.allocator.useCacheForAllThread`) that need the holding thread to die 
in order to make the available to other threads, but their size is quite small 
by default and the size I've chosen as `LARGE_MESSAGE_CHUNK_SIZE` (ie 100 * 
1024) is `medium` but doesn't fall in any of those caches, so no real leak will 
happen until we allocate something smaller (could happen but is rare).
   IMO we should consider (for a separate issue) 2 separate improvements:
   - allow threads to stay alive forever, but just idle (and maybe using the FJ 
thread pool executor instead of the AMQ thread pool executor in that case): in 
that case we can consider such caches to be ok to be used by *all* threads 
(including non-netty ones), because the leak is just to cope with future load
   - for the AMQ thread pool executor as it is, make the presence of such 
caches configurable and off by default
   
   wdyt?
   Thanks for the comment, good catch!


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 issue #2832: ARTEMIS-2482 Large messages could leak native ByteBuffers

2019-09-11 Thread GitBox
franz1981 commented on issue #2832: ARTEMIS-2482 Large messages could leak 
native ByteBuffers
URL: https://github.com/apache/activemq-artemis/pull/2832#issuecomment-530284944
 
 
   @wy96f Yes and no :)
   The would leak (at first look, but let me take a better look), but they 
would contain just wrappers to direct memory that cannot be released (is part 
of the pool) and no cleaner (so they won't collect anything, when contains 
ByteBuffers). The impact should be way less then `IOUtil` that pool native 
`ByteBuffer`s holding exclusively native memory that won't be reused anymore...


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 issue #2832: ARTEMIS-2482 Large messages could leak native ByteBuffers

2019-09-11 Thread GitBox
franz1981 commented on issue #2832: ARTEMIS-2482 Large messages could leak 
native ByteBuffers
URL: https://github.com/apache/activemq-artemis/pull/2832#issuecomment-530273041
 
 
   @wy96f 
   
   > PooledByteBufAllocator::directBuffer uses java's ThreadLocal. Wouldn't 
this result in leak in the case if thread terminates due to idle timeout as in 
#2199 ?
   
   Do you mean the thread local arena? Or the thread local NIO ByteBuffer?
   Anyway, at the end of its usage we always release the `ByteBuf` and Netty 
will take care to release any referenced resources: it's a pool so it leaks "by 
definition" :+1: 
   
   
   
   


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 issue #2832: ARTEMIS-2482 Large messages could leak native ByteBuffers

2019-09-10 Thread GitBox
franz1981 commented on issue #2832: ARTEMIS-2482 Large messages could leak 
native ByteBuffers
URL: https://github.com/apache/activemq-artemis/pull/2832#issuecomment-530104594
 
 
   @clebertsuconic seems that I cannot add the label...please check the code, 
because I've found another part on replication where it could happen...


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 issue #2832: ARTEMIS-2482 Large messages could leak native ByteBuffers

2019-09-10 Thread GitBox
franz1981 commented on issue #2832: ARTEMIS-2482 Large messages could leak 
native ByteBuffers
URL: https://github.com/apache/activemq-artemis/pull/2832#issuecomment-530076449
 
 
   @clebertsuconic I still need to address few bits about it...will put a 
proper label...
   


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 issue #2832: ARTEMIS-2482 Large messages could leak native ByteBuffers

2019-09-10 Thread GitBox
franz1981 commented on issue #2832: ARTEMIS-2482 Large messages could leak 
native ByteBuffers
URL: https://github.com/apache/activemq-artemis/pull/2832#issuecomment-530064493
 
 
   On paging we cache manually so is a different "issue" and probably for many 
user is fine.
   Re the property I'm not sure: imo is a good idea but it will impact existing 
users where it is happening without some; they will start to perceive slower 
performance in a stealthy way..
   Anyway I don't have a strong opinion on that so will do it if you prefer 
(just need to verify if is present in jdk 11 as well)


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