[GitHub] activemq-artemis issue #1918: ARTEMIS-1722 Reduce pooled Netty ByteBuf usage
Github user franz1981 commented on the issue: https://github.com/apache/activemq-artemis/pull/1918 I'm closing this one and helping @tabish121 to validate https://github.com/apache/activemq-artemis/pull/1928, that is 100% the best approach, because it avoid completly the copy in the common case ---
[GitHub] activemq-artemis issue #1918: ARTEMIS-1722 Reduce pooled Netty ByteBuf usage
Github user tabish121 commented on the issue: https://github.com/apache/activemq-artemis/pull/1918 @franz1981 @clebertsuconic I've sent an alternate PR #1928 which performs the send without any buffer copy unless necessary due to message redelivery or existence of original delivery annotations. Proton will be copying those bytes anyway so an intermediate isn't really needed on the common path. It could be argued that you could further change things beyond what I did and just keep a single ReadableBuffer instance around in the consumer and give it the outbound buffer to send as the proton send method will take that readable buffer and call get(array, offset, length) on it to get a copy which means netty should perform the most efficient write. ---
[GitHub] activemq-artemis issue #1918: ARTEMIS-1722 Reduce pooled Netty ByteBuf usage
Github user franz1981 commented on the issue: https://github.com/apache/activemq-artemis/pull/1918 @clebertsuconic > SoftReference sucks. It usually let you OME before itâs released. I know and i was too hurried to say that it could be used :P you're right :+1: > What about having a pool somewhere else? Or a smaller pool from Netty? It could be a solution: TBH if we found together a solution that doesn't require much (Netty) config from the user pov, would be even better! > Iâm out today and Monday. Can we talk about this when I get back on Tuesday? Sure buddy, this is not a fix so we can arrive on it taking the right time @tabish121 > It doesn't look like the buffer could be contended On Tuesday with @clebertsuconic will be confirmed, but thanks...the code was really too complex for just a simple behaviour ie reuse a buffer. > It isn't entirely uncommon for consumers to be subscribed to low volume queues or topics where they might sit idle for a longer period of time so having each consumer hold onto memory like that indefinitely could lead to quick resource exhaustion on the broker in the case of 1000s of connections (think IoT) That's important to know, yes, it makes sense! So a solution could be to use a scheduled sweeper that check the alive consumers, but idle for "too long" and deallocate their buffers or maybe making use of something that the AMQP protocol itself probably provide (a keepalive, tick or similar, don't know) to do the same. Just thinking loud; anyway, I will let this PR rest a bit and we could have a chat together next week if something here could be done :+1: ---
[GitHub] activemq-artemis issue #1918: ARTEMIS-1722 Reduce pooled Netty ByteBuf usage
Github user tabish121 commented on the issue: https://github.com/apache/activemq-artemis/pull/1918 @franz1981 It doesn't look like the buffer could be contended unless perhaps the consumer was able to be subscribed to more than one queue at a time via some sort of composite type subscription but I don't think that is possible ? otherwise the broker should be dispatching serially if I understand correctly but I'd defer to @clebertsuconic on that one. It isn't entirely uncommon for consumers to be subscribed to low volume queues or topics where they might sit idle for a longer period of time so having each consumer hold onto memory like that indefinitely could lead to quick resource exhaustion on the broker in the case of 1000s of connections (think IoT) ---
[GitHub] activemq-artemis issue #1918: ARTEMIS-1722 Reduce pooled Netty ByteBuf usage
Github user clebertsuconic commented on the issue: https://github.com/apache/activemq-artemis/pull/1918 SoftReference sucks. It usually let you OME before itâs released. What about having a pool somewhere else? Or a smaller pool from Netty? Iâm out today and Monday. Can we talk about this when I get back on Tuesday? ---
[GitHub] activemq-artemis issue #1918: ARTEMIS-1722 Reduce pooled Netty ByteBuf usage
Github user franz1981 commented on the issue: https://github.com/apache/activemq-artemis/pull/1918 @clebertsuconic Yep , don't worry I'm glad that you have taken a look on it! Will answer inline.. > i - There's a buffer sitting on the ProtonSenderContext now that may be unused for a while. What if you had 1000 connections at one time.. and then all of them go inactive? I don't understand how this will be Reducing footprint. I see quite the contrary here. I think that optimizing for the common case would be the bigger fish, but I don't know if what you have described is the common pattern...it is? If yes you're right, but consider that although Netty will release it to the pool, the memory footprint won't goes away. To solve it I will use `SoftReference` that will react on memory pressure demands, just to be safe. Re memory footprint, it helps due to how Netty heap pools work: just allocating 10 bytes on it from any thread will trigger all the netty heap arenas to be allocated: that means to have more than (using the default config) 16 MB * 16 = 250 MB of allocated heaps just sitting to perform buffer copies. And the sad fact is that it will add a constant memory footprint to Artemis when the broker is idle for real too. Let me show you it... That's on master (with the patch of Tim), after forcing a Full GC: 250 MB of Netty arena remaining allocated  That's is with this patch (and Tim commit too): all the allocated bytes disappear  For the graph is not visible the throughput improvement and we can just focus just on memory: it is pretty visbile that the memory used is lower than with pooled Netty ByteBuf too and that's because with too many threads (producers/consumers) the Netty pool isn't able to scale and will fallback to produce garbage. I hope to have explained my concerns. > ii - It is not clear to me when the buffer would be busy, since the ProtonSenderContext represents a single consumer. Maybe it would.. but it shouldn't. if that's happening you will be sending a large heap buffer to GC. That's why I needed you and @tabish121 :P : if it is uncontended the logic could be made much simpler. From my tests (with 64 pairs of producers/consumers) seems uncontended so I have supposed that the "busy" case is the uncommon one and just having one byte[] created that will die very soon isn't a big deal for any GC (if is uncommon). Re how much big, with G1 (our defualt GC) `-XX:G1HeapRegionSize` is 2 MB AFAIK and any byte[] allocation > 50% of it ie 1MB is "less optimized". Let me re-write it with `SoftReference` and without 'AtomicReference` (it isn't contended) and it could address all your point, thanks!! ---
[GitHub] activemq-artemis issue #1918: ARTEMIS-1722 Reduce pooled Netty ByteBuf usage
Github user clebertsuconic commented on the issue: https://github.com/apache/activemq-artemis/pull/1918 I won't be able to look into this before Tuesday.. Please don't merge this without me... :) ---
[GitHub] activemq-artemis issue #1918: ARTEMIS-1722 Reduce pooled Netty ByteBuf usage
Github user clebertsuconic commented on the issue: https://github.com/apache/activemq-artemis/pull/1918 I see two problems here.. i - There's a buffer sitting on the ProtonSenderContext now that may be unused for a while. What if you had 1000 connections at one time.. and then all of them go inactive? I don't understand how this will be Reducing footprint. I see quite the contrary here. ii - It is not clear to me when the buffer would be busy, since the ProtonSenderContext represents a single consumer. Maybe it would.. but it shouldn't. if that's happening you will be sending a large heap buffer to GC. ---
[GitHub] activemq-artemis issue #1918: ARTEMIS-1722 Reduce pooled Netty ByteBuf usage
Github user franz1981 commented on the issue: https://github.com/apache/activemq-artemis/pull/1918 This one, together with https://github.com/apache/activemq-artemis/pull/1916 aims to reduce the memoty footprint (improving scalability by different angles) of AMQP on the broker. I need a review from @clebertsuconic and @tabish121 for the mechanics that I've used to reuse the same send buffer: the mechanics is solid but if `ProtonServerSenderContext.deliverMessage` would be already uncontended/single-threaded it could be simplified avoiding the `AtomicReference` usage. ---
