[GitHub] activemq-artemis issue #2012: ARTEMIS-1803 - Add sessionId to MessageReferen...

2018-04-12 Thread michaelandrepearce 
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...

2018-04-12 Thread franz1981 
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...

2018-04-12 Thread michaelandrepearce 
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...

2018-04-12 Thread cshannon 
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...

2018-04-12 Thread michaelandrepearce 
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...

2018-04-12 Thread cshannon 
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...

2018-04-12 Thread michaelandrepearce 
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...

2018-04-12 Thread cshannon 
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...

2018-04-12 Thread cshannon 
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...

2018-04-12 Thread cshannon 
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...

2018-04-12 Thread cshannon 
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...

2018-04-12 Thread franz1981 
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

[GitHub] activemq-artemis issue #2012: ARTEMIS-1803 - Add sessionId to MessageReferen...

2018-04-12 Thread michaelandrepearce 
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...

2018-04-12 Thread franz1981 
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...

2018-04-12 Thread cshannon 
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...

2018-04-12 Thread franz1981 
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...

2018-04-12 Thread cshannon 
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...

2018-04-12 Thread franz1981 
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...

2018-04-12 Thread cshannon 
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...

2018-04-12 Thread franz1981 
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: 


---