ARTEMIS-2984 Compressed large messages can leak native resources #3334

franz1981 · 2020-11-12T16:01:57Z

https://issues.apache.org/jira/browse/ARTEMIS-2984

clebertsuconic · 2020-11-12T16:20:49Z

...lient/src/main/java/org/apache/activemq/artemis/core/client/impl/LargeMessageController.java

 import org.apache.activemq.artemis.api.core.ActiveMQException;

-public interface LargeMessageController extends ActiveMQBuffer {


this is an API change, isn't?

it's part of the Core Client API when dealing with large messages...

it's a client API.. I don't think. you can guarantee there are no users using this API.

answered on #3334 (comment)

clebertsuconic · 2020-11-12T16:21:07Z

artemis-core-client/src/test/java/org/apache/activemq/artemis/util/CompressionUtilTest.java

@@ -104,42 +103,6 @@ public void testDeflaterReader2() throws Exception {
      compareByteArray(allCompressed, output, compressedDataLength);
   }

-   @Test


what happened to this test?

InflaterReader was no longer needed, so could be removed (need to be sure of this one yet)!

.../org/apache/activemq/artemis/tests/unit/core/client/impl/ReadableLargeMessageController.java

franz1981 · 2020-11-13T07:00:43Z

@clebertsuconic I'm reworking on this to save users to break their expectations but I see some interesting things that could improve large message streaming, see:

   private byte getByte(final long index) {
      checkForPacket(index);

      if (fileCache != null && index < packetPosition) {
         return fileCache.getByteFromCache(index);
      } else {
         return currentPacket.getChunk()[(int) (index - packetPosition)];
      }
   }

This method is the only one using FileCache to read data as part of the ActiveMQBuffer API on large messages, that's internal.
Large message streaming is slowed down (and get some additional memory footprint increase too) by the FileCache presence during message streaming, because it needs to save each incoming chunk into it as well, but if users won't read it's just useless work...

clebertsuconic · 2020-11-17T19:22:54Z

@franz1981 I'm lost on what you mean on your last comment.. you mean we should remove usage, or you have already reworked the usage?

franz1981 · 2020-11-18T04:07:06Z

@clebertsuconic

you mean we should remove usage, or you have already reworked the usage?

I haven't done it because of the ActiveMQBuffer API on LargeMessageController we expose to users: if we drop it, we could save using FileCache while receiving chunks of the large message, both speeding it up and reducing its memory footprint.
Now we have a jmh module and I can work on a benchmark that can show the speedup I'm talking about

clebertsuconic · 2020-11-20T16:27:26Z

@franz1981 what about the previous propose? to make the usage separate? we break the API but allow an alternative?

you had a class that you had moved to tests.. all I asked you was to move that class to the API.. so if someone is broken by this they could just change their code to use this new class.

they are using an internal method.. they would be broken by it.. but at least they would have an alternative.

if that's not possible then we can talk about breaking it for sure.

michaelandrepearce · 2020-11-20T16:39:02Z

Do we really expose the controller to users? Yes its public but is it exposed in the client apis? I didnt think we do.

clebertsuconic · 2020-11-20T19:51:13Z

�we don't expose it...

Franz had moved the controller to a separate class on a test..

all I asked was to have that class as part of the main jar.

But if that's a big deal.. just take it out then.

franz1981 · 2020-11-21T07:25:11Z

@michaelandrepearce @clebertsuconic If we take the step to remove it I'm sure large message streaming can just drop any file cache and both code and perf will benefit there

what about the previous propose? to make the usage separate? we break the API but allow an alternative?

If the usage wasn't exposed to users, to remove the whole code paths that makes large msg streaming complex, saving both perf and leaky methods to exists as a whole. If removing isn't an option I can just prepare the code for a future removal, when we decide to do that, but we won't get any perf and stability benefits from removing it.
Adding a separate class to perform the read still won't allow me to remove the FileCache, so no perf benefits...

michaelandrepearce · 2020-11-21T08:52:33Z

@franz1981 i think we established end users dont see this class we can do with it as we want. Removal, refactor, give it cups of tea.

franz1981 · 2020-11-23T15:28:10Z

Please don't merge it, I'm still trying to be 100% sure that file cache isn't used..but maybe it's used somehow...

clebertsuconic · 2020-11-23T15:45:42Z

If it's easy. Keep the class available like before.

If not. Just take it out.

Keep it simple.

franz1981 · 2020-11-23T15:48:16Z

As it is now is just packed in a different class to make it more readeable :)
Hence this version on this PR should be fine, just need to run the CI on it

clebertsuconic · 2021-02-04T20:34:45Z

...re-client/src/main/java/org/apache/activemq/artemis/core/client/impl/ClientProducerImpl.java

+         bytesRead += read;
+      }
+      while (bytesRead < chunkLimit);
+      assert bytesRead == chunkLimit;


is this assert correct? what if you are reading a large message that Is not a multiple of chunkLimit?

clebertsuconic · 2021-02-04T20:35:34Z

@franz1981 what's the status on this PR? is this ready to merge?

I saw briefly one assert (that I added a comment) that I wasn't sure at a first look.

michaelandrepearce · 2021-09-06T10:25:14Z

@clebertsuconic / @franz1981 whats occuring with this, wanting to start clearing down old / stagnant PR's a bit of spring cleaning so to say - so we focus on stuff thats actively working on / relevant .

franz1981 · 2021-09-07T07:47:21Z

Closing this because large messages changed a bit recently: need to be reworked

franz1981 added 2 commits November 12, 2020 17:09

ARTEMIS-2984 Deflater native memory must be released on error

2661803

ARTEMIS-2984 Release Inflater resources

be308b8

franz1981 force-pushed the no_mem_leaks branch from 6990319 to a8b2090 Compare November 12, 2020 16:09

clebertsuconic reviewed Nov 12, 2020

View reviewed changes

.../org/apache/activemq/artemis/tests/unit/core/client/impl/ReadableLargeMessageController.java Outdated Show resolved Hide resolved

franz1981 force-pushed the no_mem_leaks branch 2 times, most recently from 480a877 to a8a3adc Compare November 13, 2020 07:39

ARTEMIS-2984 Simplify large message controllers (re)moving dead code

fd02ad4

franz1981 force-pushed the no_mem_leaks branch from a8a3adc to fd02ad4 Compare November 13, 2020 07:55

clebertsuconic reviewed Feb 4, 2021

View reviewed changes

franz1981 closed this Sep 7, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ARTEMIS-2984 Compressed large messages can leak native resources #3334

ARTEMIS-2984 Compressed large messages can leak native resources #3334

franz1981 commented Nov 12, 2020

clebertsuconic Nov 12, 2020

franz1981 Nov 12, 2020

clebertsuconic Nov 12, 2020

franz1981 Nov 12, 2020

franz1981 commented Nov 13, 2020 •

edited

clebertsuconic commented Nov 17, 2020

franz1981 commented Nov 18, 2020 •

edited

clebertsuconic commented Nov 20, 2020

michaelandrepearce commented Nov 20, 2020 •

edited

clebertsuconic commented Nov 20, 2020

franz1981 commented Nov 21, 2020 •

edited

michaelandrepearce commented Nov 21, 2020

franz1981 commented Nov 23, 2020

clebertsuconic commented Nov 23, 2020

franz1981 commented Nov 23, 2020

clebertsuconic Feb 4, 2021

clebertsuconic commented Feb 4, 2021

michaelandrepearce commented Sep 6, 2021

franz1981 commented Sep 7, 2021

		import org.apache.activemq.artemis.api.core.ActiveMQException;

		public interface LargeMessageController extends ActiveMQBuffer {

ARTEMIS-2984 Compressed large messages can leak native resources #3334

ARTEMIS-2984 Compressed large messages can leak native resources #3334

Conversation

franz1981 commented Nov 12, 2020

clebertsuconic Nov 12, 2020

Choose a reason for hiding this comment

franz1981 Nov 12, 2020

Choose a reason for hiding this comment

clebertsuconic Nov 12, 2020

Choose a reason for hiding this comment

franz1981 Nov 12, 2020

Choose a reason for hiding this comment

franz1981 commented Nov 13, 2020 • edited

clebertsuconic commented Nov 17, 2020

franz1981 commented Nov 18, 2020 • edited

clebertsuconic commented Nov 20, 2020

michaelandrepearce commented Nov 20, 2020 • edited

clebertsuconic commented Nov 20, 2020

franz1981 commented Nov 21, 2020 • edited

michaelandrepearce commented Nov 21, 2020

franz1981 commented Nov 23, 2020

clebertsuconic commented Nov 23, 2020

franz1981 commented Nov 23, 2020

clebertsuconic Feb 4, 2021

Choose a reason for hiding this comment

clebertsuconic commented Feb 4, 2021

michaelandrepearce commented Sep 6, 2021

franz1981 commented Sep 7, 2021

franz1981 commented Nov 13, 2020 •

edited

franz1981 commented Nov 18, 2020 •

edited

michaelandrepearce commented Nov 20, 2020 •

edited

franz1981 commented Nov 21, 2020 •

edited