use heap buffers in the default payload decoder

[SerCeMan]

Hey, folks!

This PR proposes changing DefaultPayloadDecoder to use on-heap buffers instead of off-heap buffers.

The reason for this is that off-heap buffers release the underlying buffer using cleaner when the ByteBuffer's reference is gone. Due to direct buffers, it's much harder to predict what the actual memory consumption of the app is as there are no guarantees on when the cleaner is run and when the references are actually released. Additionally, jdk.internal.ref.Cleaner increases the GC pressure. In our case, replacing allocateDirect with allocate resulted in a significant drop of the length of GC pauses as on the graphs below.

In my understanding, the change should also positively affect performance in the case when the buffers are small. For instance, in the case where the server handles a large number of mostly idle connections and mostly processes small payload and keepalive frames as the buffers be allocated directly in TLAB and not as a chunk of global memory.

I believe that it would be preferable for the default generic purpose mode to allow for predictability, so I'm proposing this change.

[OlegDokuka]

LGTM. There is nothing better than production tested improvement. Thank you @SerCeMan for sharing your prod observations!

Also, can you specify what JDK version did you use, have you checked the same on the other JDK versions?

Can that be tunned using JVM props?

Just wanna make sure that other JDK versions/setups will be affected in a positive way as well.

[OlegDokuka]

Also, just curious, why don't you use the ZERO_COPY setup? Do you have any challenges using it?

[SerCeMan]

Also, can you specify what JDK version did you use, have you checked the same on the other JDK versions?

openjdk version "13.0.4" 2020-07-14
OpenJDK Runtime Environment Zulu13.33+25-CA (build 13.0.4+8-MTS)
OpenJDK 64-Bit Server VM Zulu13.33+25-CA (build 13.0.4+8-MTS, mixed mode, sharing)

I believe that the behavior would manifest on any jdk, however, the effect is likely to be different and depends on GC as different GCs handle weak references differently. In our case, we use ShenandoahGC on this service, and the direct buffers here were a cause of much longer pauses than it would be otherwise. If I understand correctly, the instances can also run out of memory when using direct buffers on any JVM (there is a more detailed description of the effect in "2. OutOfMemoryError: Direct Buffer Memory").

I was also planning to publish a small blog post on this as the debugging story was quite interesting and the behavior can be reproduced relatively easily.

Also, just curious, why don't you use the ZERO_COPY setup? Do you have any challenges using it?

We use it in a different setup, however, in this case, we haven't switched yet as it requires more careful ref management.

[OlegDokuka]

@SerCeMan Does it makes sense to use similar buffer creation to what is done in netty? E.g. having a property which specifies how to create ByteBuffer. For example, we can safely rely on Netties PlatformDependent class for that purpose (https://github.com/netty/netty/blob/162e59848ad1801ab26e501c3c93ee08e83f5065/common/src/main/java/io/netty/util/internal/PlatformDependent.java#L201)

[OlegDokuka]

We use it in a different setup, however, in this case, we haven't switched yet as it requires more careful ref management.

Also, if you use it in 1.0.x, I suggest setting 1.0.x as a base branch for this PR. Master aims 1.1.x now

[SerCeMan]

Does it makes sense to use similar buffer creation to what is done in netty? E.g. having a property which specifies how to create ByteBuffer.

If I understand correctly, it would still require releasing the buffers using PlatformDependent.freeByteBuffer to release the underlying native memory proactively while the goal of DefaultPayloadDecoder is to provide a setup in which ref management isn't needed.

Also, if you use it in 1.0.x, I suggest setting 1.0.x as a base branch for this PR. Master aims 1.1.x now

Thanks! We're using 1.0.2 but since we've already supplied a "custom" PayloadDecoder which uses allocate, I'm happy to wait until 1.1.x is released. However, happy to change the base branch if 1.0.x is preferable 👍

[OlegDokuka]

I have been talking about the PlatformDependent#preferDirectBuffer() property to create a if(preferDirectBuffer) allocateDirect() else allocate() statement in DefaultPayloadDecoder, so this becomes a configurable thing, and if one prefer DirectByffer to ByteBuffer, then such can be configured

[rstoyanchev]

It is even a surprise to me that it's using direct memory. It also wasn't always the case. It looks like it changed some time before 1.0. To me the point of DefaultPayloadDecoder is to provide an alternative to using direct memory at the cost of avoiding zero copy.

[SerCeMan]

I have been talking about the PlatformDependent#preferDirectBuffer() property to create a if(preferDirectBuffer) allocateDirect() else allocate() statement in DefaultPayloadDecoder, so this becomes a configurable thing, and if one prefer DirectByffer to ByteBuffer, then such can be configured

Ah, I see. However, I'm not 100% sure about the use case for preferDirectBuffer here. The directBufferPreferred property in the case of DefaultPayloadDecoder is likely to be true (even on Java 6 the buffer can be released using reflection, CleanerJava6) which feels like a suboptimal default given that the users will face similar issues and they will have to release the buffers using #freeByteBuffer to avoid relying on weak references.

[OlegDokuka]

Ok, let's merge it as it is now. @SerCeMan please rebase this one onto 1.0.x so I will merge it and then port to master. The flexibility of the Payloads creation will be revised in the PR for #793

[SerCeMan]

Ok, let's merge it as it is now. @SerCeMan please rebase this one onto 1.0.x so I will merge it and then port to master.

Thanks! Done.

[OlegDokuka]

@SerCeMan merged and ported to master! Thank you for your contribution!