You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@activemq.apache.org by cshannon <gi...@git.apache.org> on 2018/04/12 11:15:40 UTC
[GitHub] activemq-artemis pull request #2012: ARTEMIS-1803 - Add sessionId to Message...
GitHub user cshannon opened a pull request:
https://github.com/apache/activemq-artemis/pull/2012
ARTEMIS-1803 - Add sessionId to MessageReference
Track the sessionId along with the consumerId in a MessageReference when
appropriate in order to figure out which consumer the reference belongs
to
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/cshannon/activemq-artemis ARTEMIS-1803
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/activemq-artemis/pull/2012.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #2012
----
commit 262cac9f0b8b581b56bda6219bd43b7922edf6c9
Author: Christopher L. Shannon (cshannon) <ch...@...>
Date: 2018-04-12T11:05:16Z
ARTEMIS-1803 - Add sessionId to MessageReference
Track the sessionId along with the consumerId in a MessageReference when
appropriate in order to figure out which consumer the reference belongs
to
----
---
[GitHub] activemq-artemis issue #2012: ARTEMIS-1803 - Add sessionId to MessageReferen...
Posted by cshannon <gi...@git.apache.org>.
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...
Posted by franz1981 <gi...@git.apache.org>.
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...
Posted by cshannon <gi...@git.apache.org>.
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...
Posted by franz1981 <gi...@git.apache.org>.
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:
---
[GitHub] activemq-artemis issue #2012: ARTEMIS-1803 - Add sessionId to MessageReferen...
Posted by cshannon <gi...@git.apache.org>.
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...
Posted by franz1981 <gi...@git.apache.org>.
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
0 12 (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.alreadyAcked N/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.protocolData N/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
0 12 (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 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.messageSize N/A
64 4 int PagedReferenceImpl.persistedCount N/A
68 4 int PagedReferenceImpl.messageEstimate N/A
72 4 int PagedReferenceImpl.deliveryCount N/A
76 1 boolean PagedReferenceImpl.hasConsumerID N/A
77 1 boolean PagedReferenceImpl.alreadyAcked N/A
78 1 byte PagedReferenceImpl.largeMessage N/A
79 1 (alignment/padding gap)
80 4 org.apache.activemq.artemis.core.paging.cursor.PagePosition PagedReferenceImpl.position N/A
84 4 java.lang.ref.WeakReference PagedReferenceImpl.message N/A
88 4 java.lang.String PagedReferenceImpl.sessionId N/A
92 4 org.apache.activemq.artemis.core.paging.cursor.PageSubscription PagedReferenceImpl.subscription N/A
96 4 java.lang.Object PagedReferenceImpl.protocolData N/A
100 4 (loss due to the next object alignment)
Instance size: 104 bytes
Space losses: 1 bytes internal + 4 bytes external = 5 bytes total
*****************************************************
org.apache.activemq.artemis.core.server.impl.LastValueQueue$HolderReference object internals:
OFFSET SIZE TYPE DESCRIPTION VALUE
0 12 (object header) N/A
12 1 boolean HolderReference.hasConsumerID N/A
13 3 (alignment/padding gap)
16 8 long HolderReference.consumerID N/A
24 4 org.apache.activemq.artemis.api.core.SimpleString HolderReference.prop N/A
28 4 org.apache.activemq.artemis.core.server.MessageReference HolderReference.ref N/A
32 4 java.lang.String HolderReference.sessionId N/A
36 4 org.apache.activemq.artemis.core.server.impl.LastValueQueue HolderReference.this$0 N/A
Instance size: 40 bytes
Space losses: 3 bytes internal + 0 bytes external = 3 bytes total
```
---
[GitHub] activemq-artemis issue #2012: ARTEMIS-1803 - Pass ServerConsumer to messageE...
Posted by cshannon <gi...@git.apache.org>.
Github user cshannon commented on the issue:
https://github.com/apache/activemq-artemis/pull/2012
@franz1981 and @michaelandrepearce - I updated the patch to pass a reference to the ServerConsumer (when it applies) for both the expired and acked callbacks and dropped the extra reference to the serverId in the MessageReference class....let me know what you think
---
[GitHub] activemq-artemis issue #2012: ARTEMIS-1803 - Add sessionId to MessageReferen...
Posted by franz1981 <gi...@git.apache.org>.
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...
Posted by cshannon <gi...@git.apache.org>.
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...
Posted by cshannon <gi...@git.apache.org>.
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...
Posted by cshannon <gi...@git.apache.org>.
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...
Posted by michaelandrepearce <gi...@git.apache.org>.
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 - Pass ServerConsumer to messageE...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/2012
@franz1981, any comment?
---
[GitHub] activemq-artemis issue #2012: ARTEMIS-1803 - Add sessionId to MessageReferen...
Posted by cshannon <gi...@git.apache.org>.
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...
Posted by cshannon <gi...@git.apache.org>.
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...
Posted by michaelandrepearce <gi...@git.apache.org>.
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 - Pass ServerConsumer to messageE...
Posted by cshannon <gi...@git.apache.org>.
Github user cshannon commented on the issue:
https://github.com/apache/activemq-artemis/pull/2012
@franz1981 and @michaelandrepearce - thanks for taking a look
---
[GitHub] activemq-artemis issue #2012: ARTEMIS-1803 - Add sessionId to MessageReferen...
Posted by michaelandrepearce <gi...@git.apache.org>.
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 pull request #2012: ARTEMIS-1803 - Pass ServerConsumer to m...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/activemq-artemis/pull/2012
---
[GitHub] activemq-artemis issue #2012: ARTEMIS-1803 - Add sessionId to MessageReferen...
Posted by michaelandrepearce <gi...@git.apache.org>.
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 - Pass ServerConsumer to messageE...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/2012
I personally prefer this.
---
[GitHub] activemq-artemis issue #2012: ARTEMIS-1803 - Add sessionId to MessageReferen...
Posted by franz1981 <gi...@git.apache.org>.
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...
Posted by cshannon <gi...@git.apache.org>.
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...
Posted by michaelandrepearce <gi...@git.apache.org>.
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...
Posted by franz1981 <gi...@git.apache.org>.
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 - Pass ServerConsumer to messageE...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on the issue:
https://github.com/apache/activemq-artemis/pull/2012
@michaelandrepearce @cshannon my sider-sense doesn't feel dangers anymore :
---