You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@activemq.apache.org by franz1981 <gi...@git.apache.org> on 2018/01/19 08:28:46 UTC
[GitHub] activemq-artemis pull request #1791: ARTEMIS-1622 Reduce memory footprint of...
GitHub user franz1981 opened a pull request:
https://github.com/apache/activemq-artemis/pull/1791
ARTEMIS-1622 Reduce memory footprint of QueueImpl
LinkedListImpl is turned into an optionally intrusive linked list by allowing message references to extend Node
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/franz1981/activemq-artemis node_gc
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/activemq-artemis/pull/1791.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 #1791
----
commit 77956f00c95a41ac22cdfbad6ffaf6606bf2488d
Author: Francesco Nigro <ni...@...>
Date: 2018-01-18T14:45:20Z
ARTEMIS-1622 Reduce memory footprint of QueueImpl
LinkedListImpl is turned into an optionally intrusive linked list by allowing message references to extend Node
----
---
[GitHub] activemq-artemis issue #1791: ARTEMIS-1622 Reduce memory footprint of QueueI...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on the issue:
https://github.com/apache/activemq-artemis/pull/1791
@michaelandrepearce That's the best I could think to reduce the number of changes but honestly `MessageReferenceImpl` isn't *really* forced to use himself as a parameter
---
[GitHub] activemq-artemis issue #1791: ARTEMIS-1622 Reduce memory footprint of QueueI...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on the issue:
https://github.com/apache/activemq-artemis/pull/1791
I've already run CI on it and it seems to be safe.
Just few numbers to justify it...
On master:
While with this PR:
800 MB vs 640 MB = 160 MB saved bytes
Given the default broker settings for the JVM the math is:
- each `Node` instance is 32 bytes = 12 bytes obj header + 16 bytes of fields + 4 bytes of padding to have 8 byte alignment
- the original `MessageReferenceImpl` was 48 bytes, including a 12 bytes header
- this PR `MessageReferenceImpl` is 64 bytes = 48 bytes (of the original) + 16 bytes (of `Node`) + 0 bytes of padding (64 is already 8 bytes aligned)
We're saving 4 bytes of padding and 12 bytes of Object header from `Node` = 16 bytes for each message entry.
If we have 10_000_000 messages we're saving 160 Megabytes.
IMO there are few other checks to be performed and need anyone that knows how the original LinkedListImpl works, but the change is easy to be reverted: I've already checked that there aren't evident memory leaks, but any feedback is appreciated :+1:
---
[GitHub] activemq-artemis issue #1791: ARTEMIS-1622 Reduce memory footprint of QueueI...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on the issue:
https://github.com/apache/activemq-artemis/pull/1791
@michaelandrepearce I will try to maintain the generics too if possible (not simple to fight with generics!) and about the cast it will be put already by the JVM with a thing called "uncommon trap" because generics live only at compile time and the JVM always prefers to be safer than sorry.
> this could have been: PagedReferenceImpl extends LinkedListImpl.Node
do you mean `PagedReferenceImpl extends LinkedListImpl.Node<PagedReferenceImpl>`?
---
[GitHub] activemq-artemis issue #1791: ARTEMIS-1622 Reduce memory footprint of QueueI...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on the issue:
https://github.com/apache/activemq-artemis/pull/1791
@michaelandrepearce The fact is that what Node contains is "enforced" by who use it and that is already generics (ie `LinkedListImpl`) and adding generic to `Node' won't add any value to who declare it (ie `PagedReferenceImpl`) indeed all the `Node` fields are hidden on purpose (for perf reasons mainly).
I hope it will explain why I've chosen to "drop" this feature, but I agree that could be cool to have it :)
---
[GitHub] activemq-artemis pull request #1791: ARTEMIS-1622 Reduce memory footprint of...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/activemq-artemis/pull/1791
---
[GitHub] activemq-artemis issue #1791: ARTEMIS-1622 Reduce memory footprint of QueueI...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/1791
@franz1981 looks good to me.
---
[GitHub] activemq-artemis issue #1791: ARTEMIS-1622 Reduce memory footprint of QueueI...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/1791
Makes sense and agreed anyone using the list wouldn’t be exposed to the node
---
[GitHub] activemq-artemis issue #1791: ARTEMIS-1622 Reduce memory footprint of QueueI...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/1791
@franz1981 why did we need to loose the generics on Node? it now means theres a cast on get, and alot of un-needed change IMO.
if it was due to the extension now made here:
PagedReferenceImpl extends LinkedListImpl.Node
this could have been: PagedReferenceImpl extends LinkedListImpl.Node<PagedReferenceImpl>
and like wise in MessageReferenceImpl
---