[GitHub] activemq-artemis issue #2012: ARTEMIS-1803 - Add sessionId to MessageReferen...
Github user michaelandrepearce commented on the issue: https://github.com/apache/activemq-artemis/pull/2012 My point is whilst it will require not quite a quick win, itâs not impossible to avoid this, and refactor to get this is probably a good thing. I realise itâs not a quick win as this. But nor is all the scaling and perf Work we have been doing. ---
[GitHub] activemq-artemis issue #2012: ARTEMIS-1803 - Add sessionId to MessageReferen...
Github user franz1981 commented on the issue: https://github.com/apache/activemq-artemis/pull/2012 @cshannon Summarized: ``` org.apache.activemq.artemis.core.server.impl.MessageReferenceImpl: Instance size: 72 bytes Space losses: 2 bytes internal + 4 bytes external = 6 bytes total org.apache.activemq.artemis.core.paging.cursor.PagedReferenceImpl: Instance size: 104 bytes Space losses: 1 bytes internal + 4 bytes external = 5 bytes total org.apache.activemq.artemis.core.server.impl.LastValueQueue$HolderReference: Instance size: 40 bytes Space losses: 3 bytes internal + 0 bytes external = 3 bytes total ``` `Instance size` is the value you're searching for: I have attached the `Space losses` as well, because it is related to what @michaelandrepearce is saying: just adding that mere 4 bytes reference (with heap < 32 Gb the JVM uses compressed oops by default) has added 4 bytes (external) of padding ie a total of 8 bytes of more space used. TBH that's a difficult choice: we have done recently many changes to make every protocol much GC "gentle" to allow scaling more easily, but @cshannon is right that there are monitoring/telemetry reasons very important to be achieved as well. You both have very good points :O ---
[GitHub] activemq-artemis issue #2012: ARTEMIS-1803 - Add sessionId to MessageReferen...
Github user michaelandrepearce commented on the issue: https://github.com/apache/activemq-artemis/pull/2012 I disagree itâs a 4% overhead at best and 10% extra over head for worst. Thatâs quite a kick ---
[GitHub] activemq-artemis issue #2012: ARTEMIS-1803 - Add sessionId to MessageReferen...
Github user cshannon commented on the issue: https://github.com/apache/activemq-artemis/pull/2012 I understand wanting to keep memory low but there's a point where you take things too far. At some point usability and correctness is more important. Having just a consumerId is just not correct, end of story. It is not a unique value and will be duplicated. The client library is responsible for creating that consumerId per session so there's no way to go back and make the consumerId unique (can't change old client libraries). I wish there were, that would solve this problem. I do not agree that refactoring is the best approach. At the end of the day if we can't add a reference to a String I think we are taking trying to limit memory usage too far. ---
[GitHub] activemq-artemis issue #2012: ARTEMIS-1803 - Add sessionId to MessageReferen...
Github user michaelandrepearce commented on the issue: https://github.com/apache/activemq-artemis/pull/2012 As noted my biggest concern is that message refs need to be as light as humanly possible as theyâre all in memory and affects greatly the scaling. I would personally prefer the refactor if needed, than take this hit. Especially as this is only needed by someone wanting to use this in a plugin. Which means everyone else has to suffer ---
[GitHub] activemq-artemis issue #2012: ARTEMIS-1803 - Add sessionId to MessageReferen...
Github user cshannon commented on the issue: https://github.com/apache/activemq-artemis/pull/2012 @michaelandrepearce - because 1) there can be more than just that use case for having sessionId part of the reference in the future and 2) the acknowledge code is not part of the consumer and is handled later by the QueueImpl...see the acknowledge method inside QueueImpl..it just gets the reference and does the acking and doesn't know the consumer by that point because the ServerConsumer calls ack on the ref but doesn't pass itself to it...there would have to be a good amount of refactoring to change this along with changes to public interfaces ---
[GitHub] activemq-artemis issue #2012: ARTEMIS-1803 - Add sessionId to MessageReferen...
Github user michaelandrepearce commented on the issue: https://github.com/apache/activemq-artemis/pull/2012 Should that then not be logged by the consumer code referencing the message if needed? As the consumer anyhow will be causing the ack. ---
[GitHub] activemq-artemis issue #2012: ARTEMIS-1803 - Add sessionId to MessageReferen...
Github user cshannon commented on the issue: https://github.com/apache/activemq-artemis/pull/2012 For example, it's a common use case to fire notifications or logging when a message is acknowledged. I have a requirement to do this for my organization and I need to quickly track the specific consumer that acked the message from inside the plugin. Having the sessionId as part of the reference is the only way to do this. ---
[GitHub] activemq-artemis issue #2012: ARTEMIS-1803 - Add sessionId to MessageReferen...
Github user cshannon commented on the issue: https://github.com/apache/activemq-artemis/pull/2012 @franz1981 - So what size would you recommend? Seems like maybe 8 is too much to add to the estimation based on the output. ---
[GitHub] activemq-artemis issue #2012: ARTEMIS-1803 - Add sessionId to MessageReferen...
Github user cshannon commented on the issue: https://github.com/apache/activemq-artemis/pull/2012 Furthermore there is more to a broker than just processing. Monitoring and metrics are very important to business flows and trying to save a few bytes of memory is not worth it if you sacrifice basic things such as being able to do proper logging and troubleshooting. ---
[GitHub] activemq-artemis issue #2012: ARTEMIS-1803 - Add sessionId to MessageReferen...
Github user cshannon commented on the issue: https://github.com/apache/activemq-artemis/pull/2012 It's not attached to the message. The reference has the information for which consumer is attached to it. The consumer id is not unique so you need to have the session Id as well otherwise there is no way to find out which consumer the reference belongs to so this has to be in memory. I really don't think it's going to be a big deal to add a reference id. ---
[GitHub] activemq-artemis issue #2012: ARTEMIS-1803 - Add sessionId to MessageReferen...
Github user franz1981 commented on the issue: https://github.com/apache/activemq-artemis/pull/2012 @michaelandrepearce Michael have good points on that, especially for such important classes. @cshannon I have anyway the results: ``` # Running 64-bit HotSpot VM. # Using compressed oop with 3-bit shift. # Using compressed klass with 3-bit shift. # Objects are 8 bytes aligned. # Field sizes by type: 4, 1, 1, 2, 2, 4, 4, 8, 8 [bytes] # Array element sizes: 4, 1, 1, 2, 2, 4, 4, 8, 8 [bytes] org.apache.activemq.artemis.core.server.impl.MessageReferenceImpl object internals: OFFSET SIZE TYPE DESCRIPTION VALUE 012 (object header) N/A 12 4 int Node.iterCount N/A 16 4 org.apache.activemq.artemis.utils.collections.LinkedListImpl.Node Node.next N/A 20 4 org.apache.activemq.artemis.utils.collections.LinkedListImpl.Node Node.prev N/A 24 8 long MessageReferenceImpl.scheduledDeliveryTime N/A 32 8 long MessageReferenceImpl.consumerID N/A 40 4 int MessageReferenceImpl.deliveryCount N/A 44 4 int MessageReferenceImpl.persistedCount N/A 48 1 boolean MessageReferenceImpl.hasConsumerID N/A 49 1 boolean MessageReferenceImpl.alreadyAckedN/A 50 2 (alignment/padding gap) 52 4 org.apache.activemq.artemis.api.core.Message MessageReferenceImpl.message N/A 56 4 org.apache.activemq.artemis.core.server.Queue MessageReferenceImpl.queue N/A 60 4 java.lang.String MessageReferenceImpl.sessionId N/A 64 4 java.lang.Object MessageReferenceImpl.protocolDataN/A 68 4 (loss due to the next object alignment) Instance size: 72 bytes Space losses: 2 bytes internal + 4 bytes external = 6 bytes total * org.apache.activemq.artemis.core.paging.cursor.PagedReferenceImpl object internals: OFFSET SIZE TYPE DESCRIPTION VALUE 012 (object header) N/A 12 4 int Node.iterCountN/A 16 4 org.apache.activemq.artemis.utils.collections.LinkedListImpl.Node Node.next N/A 20 4 org.apache.activemq.artemis.utils.collections.LinkedListImpl.Node Node.prev N/A 24 8 long PagedReferenceImpl.deliveryTime N/A 32 8 long PagedReferenceImpl.consumerID N/A 40 8 long PagedReferenceImpl.transactionID N/A 48 8 long PagedReferenceImpl.messageID N/A 56 8 long PagedReferenceImpl.messageSizeN/A 64 4 int PagedReferenceImpl.persistedCount N/A 68 4 int PagedReferenceImpl.messageEstimateN/A 72 4 int PagedReferenceImpl.deliveryCount N/A 76 1 bo
[GitHub] activemq-artemis issue #2012: ARTEMIS-1803 - Add sessionId to MessageReferen...
Github user michaelandrepearce commented on the issue: https://github.com/apache/activemq-artemis/pull/2012 Sorry to be a numpty but why this needs to be held in memory? Is it going to be used for processing the flow in broker. From an admin point even if the message is paged, if a browser or admin needs it simply get it from the message (reading out of paging) Less stuff having to hold in memory better perf. Iâm a little against this if thereâs no processing need for this. ---
[GitHub] activemq-artemis issue #2012: ARTEMIS-1803 - Add sessionId to MessageReferen...
Github user franz1981 commented on the issue: https://github.com/apache/activemq-artemis/pull/2012 @cshannon Sure, np! Probably I have some minutes to do it now ;) Just a matter to download your branch and I will do it ---
[GitHub] activemq-artemis issue #2012: ARTEMIS-1803 - Add sessionId to MessageReferen...
Github user cshannon commented on the issue: https://github.com/apache/activemq-artemis/pull/2012 @franz1981 - If you don't mind double checking with the tool again I would appreciate it since you have already validated the memory before but if you don't have time I can try and figure it out ---
[GitHub] activemq-artemis issue #2012: ARTEMIS-1803 - Add sessionId to MessageReferen...
Github user franz1981 commented on the issue: https://github.com/apache/activemq-artemis/pull/2012 @cshannon I've validated the original value using this tool: http://openjdk.java.net/projects/code-tools/jol/ If possible do the same or just wait that I take a look using it as well using the default configuration used for the broker. ---
[GitHub] activemq-artemis issue #2012: ARTEMIS-1803 - Add sessionId to MessageReferen...
Github user cshannon commented on the issue: https://github.com/apache/activemq-artemis/pull/2012 I updated the PR and added 8 to the estimated size in MessageRefereceImpl. ---
[GitHub] activemq-artemis issue #2012: ARTEMIS-1803 - Add sessionId to MessageReferen...
Github user franz1981 commented on the issue: https://github.com/apache/activemq-artemis/pull/2012 @cshannon ehehe I wish to have said "would" :) The problem with the references is that the footprint of the reference depends on the JVM configuration (`-XX:+UseCompressedOops` or not) and on the alignment with the rest of the fields: the JVM tends to add padding in order to have each field size-aligned. ---
[GitHub] activemq-artemis issue #2012: ARTEMIS-1803 - Add sessionId to MessageReferen...
Github user cshannon commented on the issue: https://github.com/apache/activemq-artemis/pull/2012 @franz1981 - Did you mean probably "would" be enough? The string is a UUID (which is normally 16 bytes I think) but it is owned by the ServerSessionImpl so it would just be a reference so however much memory that takes up (maybe 8 bytes to be safe?) ---
[GitHub] activemq-artemis issue #2012: ARTEMIS-1803 - Add sessionId to MessageReferen...
Github user franz1981 commented on the issue: https://github.com/apache/activemq-artemis/pull/2012 @cshannon It would be needed to update the memory footprint estimation of each message as well: rising it up it by 4 or 8 bytes wouldn't be enough probably :+1: ---