Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ARTEMIS-1622 Reduce memory footprint of QueueImpl #1791

Closed
wants to merge 1 commit into from

Conversation

franz1981
Copy link
Contributor

LinkedListImpl is turned into an optionally intrusive linked list by allowing message references to extend Node

@franz1981
Copy link
Contributor Author

I've already run CI on it and it seems to be safe.
Just few numbers to justify it...
On master:
image

While with this PR:
image
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 👍

@michaelandrepearce
Copy link
Contributor

@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

and like wise in MessageReferenceImpl

@franz1981
Copy link
Contributor Author

@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>?

@franz1981
Copy link
Contributor Author

franz1981 commented Jan 19, 2018

@michaelandrepearce The fact is that what Node contains is "enforced" by who use it and that is already generics (ie LinkedListImpl). Adding generics 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 :)

@michaelandrepearce
Copy link
Contributor

Makes sense and agreed anyone using the list wouldn’t be exposed to the node

@franz1981
Copy link
Contributor Author

@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

LinkedListImpl is turned into an optionally intrusive linked list by allowing message references to extend Node.
@michaelandrepearce
Copy link
Contributor

@franz1981 looks good to me.

@asfgit asfgit closed this in 72a267c Jan 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants