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

fix: reduce allocations in the Time Window Block Selector #4255

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

javiermolinar
Copy link
Contributor

What this PR does:

I noticed a huge amount of objects allocated during compactions from the Sprintf method

Screenshot 2024-10-31 at 10 09 33

This PR reuses a string buffer to reduce these allocations

❯ benchstat old.txt new.txt
goos: darwin
goarch: arm64
pkg: github.com/grafana/tempo/tempodb
cpu: Apple M3 Pro
              │   old.txt   │               new.txt               │
              │   sec/op    │   sec/op     vs base                │
BlockSelector   60.07m ± 1%   25.37m ± 2%  -57.77% (p=0.000 n=10)

              │   old.txt    │               new.txt                │
              │     B/op     │     B/op      vs base                │
BlockSelector   43.74Mi ± 0%   28.23Mi ± 0%  -35.45% (p=0.000 n=10)

              │   old.txt   │               new.txt               │
              │  allocs/op  │  allocs/op   vs base                │
BlockSelector   800.0k ± 0%   700.0k ± 0%  -12.50% (p=0.000 n=10)

Checklist

  • Tests updated

  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@javiermolinar javiermolinar changed the title reduce allocations in the Time Window Block Selector fix: reduce allocations in the Time Window Block Selector Oct 31, 2024
if activeWindow <= w {
// Grow size: 2+20+1+16+1+20
builder.Grow(60)
Copy link
Member

@joe-elliott joe-elliott Oct 31, 2024

Choose a reason for hiding this comment

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

this is a very tiny %age of all compactor allocations and the previous implementation had a lot of clarity on what the group/order/hash strings looked like. the current implementation loses this a bit

i'm fine with moving forward with this, but we need the new implementation to read as cleanly as the old given that the value gained is so small

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