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

fetch: Avoid formatting client ip and port for each partition #17967

Merged
merged 3 commits into from
Apr 20, 2024

Conversation

StephanDollberg
Copy link
Member

We were formatting the ip and port of the client for each partition in
the fetch plan. This is unneeded as it obviously doesn't change by topic
or partition.

The formatting is fairly expensive and amounts for ~2-3% of total
reactor time in a high partition count scenario.

This could be further optimized by moving the formatted version to the
connection context to avoid having this copy alive many times but that's
a bit larger change for another PR.

Before:

image

After:

image

Backports Required

  • none - not a bug fix
  • none - this is a backport
  • none - issue does not exist in previous branches
  • none - papercut/not impactful enough to backport
  • v23.3.x
  • v23.2.x

Release Notes

  • none

@StephanDollberg
Copy link
Member Author

"Hide whitespace" or going commit-by-commit recommended. Not sure why clang-format suddenly thinks it needs to reformat everything.

Avoid a copy of the fetch_config which contains a string so this avoids
an extra allocation.
Putting this into a separate commit. For whatever reason clang-format
goes berzerk here and reformats that whole lambda.
We were formatting the ip and port of the client for each partition in
the fetch plan. This is unneeded as it obviously doesn't change by topic
or partition.

The formatting is fairly expensive and amounts for ~2-3% of total
reactor time in a high partition count scenario.

This could be further optimized by moving the formatted version to the
connection context to avoid having this copy alive many times but that's
a bit larger change for another PR.
@StephanDollberg StephanDollberg force-pushed the stephan/fetch-plan-dont-format-for-each-partition branch from 3fce339 to ab894d4 Compare April 19, 2024 17:31
Copy link
Member

@dotnwat dotnwat left a comment

Choose a reason for hiding this comment

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

nice find!

@dotnwat
Copy link
Member

dotnwat commented Apr 20, 2024

AssertionError: Non-exported include found: {PosixPath('cloud_roles/signature.h')}

This is merge skew and is fixed in upstream/dev so nothing to worry about.

@dotnwat dotnwat merged commit aec2b67 into dev Apr 20, 2024
16 of 17 checks passed
@dotnwat dotnwat deleted the stephan/fetch-plan-dont-format-for-each-partition branch April 20, 2024 18:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants