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

optimize channel closing to avoid allocations when flushing pending responses #3553

Merged
merged 1 commit into from
Mar 4, 2024

Conversation

fwbrasil
Copy link
Contributor

Problem

Scala's foreach is quite difficult for the JIT to optimize. The use of shared abstract implementations in Scala's collections makes call sides become highly polymorphic and simple cases like this one end up requiring allocations and interface dispatches.

Solution

Use a while since pendingResponses is a mutable queue and can efficiently dequeue elements.

Notes

  • This is part of Reduce allocation rate #3552
  • This issue isn't present in my benchmark implementation anymore because I've fixed the nginx config to use keepalive but I thought it was still worth sending the patch.

@adamw adamw requested a review from kciesielski February 29, 2024 20:04
@@ -85,7 +85,9 @@ class NettyServerHandler[F[_]](
pendingResponses.length
)
}
pendingResponses.foreach(_.apply())
while(pendingResponses.nonEmpty) {
pendingResponses.dequeue()()
Copy link
Member

Choose a reason for hiding this comment

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

I know it's a nit-pick, but maybe pendingResponses.dequeue().apply() would be more readable, as the double ()() look a bit like a typo?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point. I've just updated it

@adamw adamw merged commit 3d7913a into softwaremill:master Mar 4, 2024
23 checks passed
@fwbrasil fwbrasil deleted the opt-pending branch March 4, 2024 18:04
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.

2 participants