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

Default to UnpooledByteBufAllocator #157

Merged
merged 5 commits into from
Mar 25, 2019
Merged

Conversation

eolivelli
Copy link
Contributor

@eolivelli eolivelli commented Mar 22, 2019

Changes:

  • Default to UnpooledByteBufAllocator
  • Introduce a new CacheClient builder

This is the safest default
Netty by default will keep thread local Pools
f the application uses many different threads while accessing the cache (like a WebApplication)
but it does not perform frequent operations (for us those operations will lead to ByteBuffer allocations)
pooling ByteBuffers will lead to a large usage of direct memory
which won't be reclaimed, because by default Netty reclaims memory
per thread and every N allocations

Many thanks to @normanmaurer who helped understanding the troubles that may be caused by using PooledByteBufAllocator under certain conditions.

Introduce a new CacheClient builder.

This is the safest default
Netty by default will keep thread local Pools
if the application uses several threads in order to save values
on the case but it does not perform frequent operations (ByteBuffer allocations)
pooling ByteBuffers will lead to a large usage of direct memory
which won't be reclaimed, because by default Netty reclaims memory
per thread and every N allocations.
Enrico Olivelli - Diennea added 2 commits March 22, 2019 15:22
@normanmaurer
Copy link

@eolivelli another alternative (which I think may be better) would be to use:

https://github.com/netty/netty/blob/4.1/buffer/src/main/java/io/netty/buffer/PooledByteBufAllocator.java#L227

and set useCacheForAllThreads to false.

@eolivelli
Copy link
Contributor Author

Thanks @normanmaurer for your suggestion.

I had in mind only "io.netty.allocator.useCacheForAllThreads" system property and this cannot be controlled at this level.

I will change this patch in order to allow the user to pass a ByteBufAllocator and leave the default with UnpooledByteBufferAllocator, I can't let the user of CacheClient deal with every Netty configuration parameter and it won't be future proof.

Originally I didn't want to expose Netty classes on public API but I feel it is the best option nowadays.
I don't think we will leave Netty in the mid/long term :-)

Having the ability to use a shared ByteBufAllocator will help the integration with other Netty based libraries as well

@normanmaurer
Copy link

@eolivelli ok no worries... just wanted to show you some other option ;)

@eolivelli
Copy link
Contributor Author

@normanmaurer thanks.

I like more this new changeset with a shareable ByteBufAllocator.

* @return the builder itself
*/
public Builder allocator(ByteBufAllocator allocator) {
this.allocator = allocator;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: you may want to do a null check

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, sure

@eolivelli
Copy link
Contributor Author

@normanmaurer I have addressed your comment about the null check

@diegosalvi diegosalvi merged commit ff08b90 into master Mar 25, 2019
@diegosalvi diegosalvi deleted the fix/default-unpooled branch March 25, 2019 08:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants