Skip to content

Use ChannelFutureListener in Netty code to reduce capturing lambdas#112967

Merged
original-brownbear merged 3 commits intoelastic:mainfrom
original-brownbear:capture-less-netty-listeners
Sep 18, 2024
Merged

Use ChannelFutureListener in Netty code to reduce capturing lambdas#112967
original-brownbear merged 3 commits intoelastic:mainfrom
original-brownbear:capture-less-netty-listeners

Conversation

@original-brownbear
Copy link
Copy Markdown
Contributor

Mainly motivated by simplifying the reference chains for Netty buffers and have easier to analyze heap dumps in some spots but also a small performance win in and of itself.

Mainly motivated by simplifying the reference chains for Netty buffers
and have easier to analyze heap dumps in some spots but also a small
performance win in and of itself.
@original-brownbear original-brownbear added >non-issue :Distributed/Network Http and internode communication implementations labels Sep 16, 2024
@elasticsearchmachine elasticsearchmachine added the Team:Distributed Meta label for distributed team. label Sep 16, 2024
@elasticsearchmachine
Copy link
Copy Markdown
Collaborator

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

Copy link
Copy Markdown
Member

@DaveCTurner DaveCTurner left a comment

Choose a reason for hiding this comment

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

LGTM but I fear we'll keep making this mistake. Could we forbid the overly-generic io.netty.channel.ChannelFuture#addListener and add something to Netty4Utils to force the type to be ChannelFutureListener?

@original-brownbear
Copy link
Copy Markdown
Contributor Author

@DaveCTurner

Thanks for taking a look!

io.netty.channel.ChannelFuture#addListener and add something to Netty4Utils to force the type to be ChannelFutureListener?

You mean completely forbidding addListener via forbidden APIs and then having a Netty4Utils.addListener(ChannelPromise, ChannelFutureListener) or so and only use that? I could do that, but that makes the code one step harder to read and we have a couple of spots where the added listener doesn't need to know about the channel. To me it's not worth the extra indirection step, but it's "your code" now :) Happy to add that, don't feel strongly about it :)

@DaveCTurner
Copy link
Copy Markdown
Member

Yeah that's what I mean, I'd prefer that to those over-subtle casts.

@original-brownbear original-brownbear requested a review from a team as a code owner September 18, 2024 15:15
@original-brownbear
Copy link
Copy Markdown
Contributor Author

@DaveCTurner alrighty done with the forbidden APIs :)

Copy link
Copy Markdown
Member

@DaveCTurner DaveCTurner left a comment

Choose a reason for hiding this comment

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

LGTM

@original-brownbear
Copy link
Copy Markdown
Contributor Author

Thanks David!

@original-brownbear original-brownbear merged commit 90e343c into elastic:main Sep 18, 2024
@original-brownbear original-brownbear deleted the capture-less-netty-listeners branch September 18, 2024 16:32
Tim-Brooks pushed a commit that referenced this pull request Sep 19, 2024
…112967)

Mainly motivated by simplifying the reference chains for Netty buffers
and have easier to analyze heap dumps in some spots but also a small
performance win in and of itself.
Tim-Brooks pushed a commit to Tim-Brooks/elasticsearch that referenced this pull request Sep 19, 2024
…lastic#112967)

Mainly motivated by simplifying the reference chains for Netty buffers
and have easier to analyze heap dumps in some spots but also a small
performance win in and of itself.
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 >non-issue Team:Distributed Meta label for distributed team. v8.16.0 v9.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants