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

Serialize outbound messages on netty buffers #80111

Merged
merged 23 commits into from
Nov 4, 2021

Conversation

Tim-Brooks
Copy link
Contributor

Currently we use BigArrays for serializing outbound messages. Since
these messages are only handled at the network layer, it is better to
use the Netty allocator when using the netty transport. This commit
implements this serialization method when netty is enabled.

@Tim-Brooks Tim-Brooks added >enhancement :Distributed/Network Http and internode communication implementations v8.1.0 labels Oct 29, 2021
@elasticmachine elasticmachine added the Team:Distributed Meta label for distributed team label Oct 29, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed (Team:Distributed)

Copy link
Contributor

@jpountz jpountz left a comment

Choose a reason for hiding this comment

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

I only had a high-level look but it looks good. Being closer to the byte[] where the data is written and using the VarHandles will hopefully make writing data faster.

NetworkStreamOutput looks to me as a better BytesStreamOutput in many cases. E.g. I believe it would also be a better fit than BytesStreamOutput in GeometryDocValuesWriter? If so maybe we should give it a more generic name that doesn't sound like it's only a good fit for network messages?

@Tim-Brooks
Copy link
Contributor Author

@jpountz - I made your changes. As I mentioned offline, the stream I created RecylerBytesStreamOutput could replace BytesStreamOutput. The main scenario where BytesStreamOutput works better currently is for expected values much smaller than the page size.

Currently, RecylerBytesStreamOutput use with the non_recycling_instance will allocate a minimum of 16KB for streams. So probably would over allocate bytes in the case of DimensionalShapeType. If you want me to investigate making RecylerBytesStreamOutput replace all instances of BytesStreamOutput I can improve its performance on streams that are very small.

Or I could look into that in a follow-up.

@jpountz
Copy link
Contributor

jpountz commented Nov 4, 2021

Thanks for explaining. Looking into replacing BytesStreamOutput as a follow-up sounds good to me as it will make it easier to evaluate the potential downsides for networking.

Copy link
Member

@original-brownbear original-brownbear left a comment

Choose a reason for hiding this comment

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

LGTM on the code changes this looks really nice and IMO is 100% the right thing to do.
The one thing I'm a little worried about is the change in behavior for very large transport messages.
Previously, the BigArrays would eventually stop pooling bytes and move to un-pooled allocation after going over the configured heap threshold, this isn't the case any longer with this change and we'd potentially pool a lot more memory than we used to after a sporadic spike in required buffers (thinking about things like servicing a CS request for stats with a large cluster state or so). What is Netty's behavior here in terms of downsizing the buffer pool again after dealing with a message of hundreds of MB on a 2G master for example?

@Tim-Brooks Tim-Brooks merged commit 34d9cbe into elastic:master Nov 4, 2021
@Tim-Brooks
Copy link
Contributor Author

What is Netty's behavior here in terms of downsizing the buffer pool again after dealing with a message of hundreds of MB on a 2G master for example?

Netty immediately releases PoolChunks when they are no longer utilized. PoolChunks are larger that 16KB. However, in the case of large messages you are discussing, there will be minimal waste.

With that said, we have obviously implemented our own recycler here that is used for inbound decompression and outbound messages. If we are concerned about overuse, we can add a counter and fallback on JVM allocations once it crosses a certain threshold. We can discuss.

DaveCTurner added a commit to DaveCTurner/elasticsearch that referenced this pull request Nov 11, 2021
Today we allocate memory for the cluster state to be sent in a
publication using `BigArrays`, but really this memory usage is
network-related so (as in elastic#80111) we should allocate its memory via the
recycler used in the transport layers (i.e. Netty's allocator) instead.
This commit does that.
DaveCTurner added a commit that referenced this pull request Nov 18, 2021
Today we allocate memory for the cluster state to be sent in a
publication using `BigArrays`, but really this memory usage is
network-related so (as in #80111) we should allocate its memory via the
recycler used in the transport layers (i.e. Netty's allocator) instead.
This commit does that.
original-brownbear added a commit to original-brownbear/elasticsearch that referenced this pull request Dec 27, 2021
original-brownbear added a commit that referenced this pull request May 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Distributed/Network Http and internode communication implementations >enhancement Team:Distributed Meta label for distributed team v8.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants