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

Reduce fmt.Sprintf allocations in query encoding #2919

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

kgeckhart
Copy link

This PR eliminates some calls to fmt.Sprintf in favor of string concatenation when the values being concatenated are strings. This should reduce the memory required to construct queries with complex objects / larger arrays. Arrays which are not flat should see a further improvement as the prefix is now computed once ahead of time instead of doing it with an fmt.Sprintf call on every element.

Included are some benchmarks which I used to decide that the maps + array use cases with fmt.Sprintf seem okay. Dropping fmt.Sprintf is faster but won't lead to any reduction in allocations and in the case of arrays a size between 100 and 499 is actually more allocations.

@kgeckhart kgeckhart requested a review from a team as a code owner December 4, 2024 17:20
@Madrigal
Copy link
Contributor

Thanks for your contribution. Just letting you know this is still on our radar, but we've been a bit busy.

For context, was there a particular scenario where this was causing a performance issue for you?

@kgeckhart
Copy link
Author

Thanks for the response. I work on an OSS CloudWatch exporter https://github.com/prometheus-community/yet-another-cloudwatch-exporter/. When running high volumes we see some rather spiky memory and I was trying to reduce it a bit. This shows the top allocators from pprof after running for some time,
image
fmt.Sprintf was a lot higher than I expected it be on the list.

Almost all of the fmt.Sprintf allocations appear to stem from these two functions
image
when serializing the large GetMetricDataInput payload.

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