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

util: Pass limit to strutil.MergeSlices #7706

Merged
merged 1 commit into from
Sep 13, 2024

Conversation

harry671003
Copy link
Contributor

@harry671003 harry671003 commented Sep 6, 2024

  • I added CHANGELOG entry for this change.
  • Change is not relevant to the end user.

Changes

MergeSlices and MergeUnsortedSlices now accepts a limit param which will make the merge faster when a lmit is passed.

Benchmark

(base) ➜  thanos git:(merge_slices_with_limit) ✗ benchstat old.txt new.txt
goos: darwin
goarch: arm64
pkg: github.com/thanos-io/thanos/pkg/strutil
cpu: Apple M1 Pro
                         │      old.txt      │                  new.txt                  │
                         │      sec/op       │      sec/op        vs base                │
MergeSlices/benchmark-10   0.00275000n ± 13%   0.00006540n ± 16%  -97.62% (p=0.000 n=10)

                         │  old.txt   │            new.txt             │
                         │    B/op    │    B/op     vs base            │
MergeSlices/benchmark-10   0.000 ± 0%   0.000 ± 0%  ~ (p=1.000 n=10) ¹
¹ all samples are equal

                         │  old.txt   │            new.txt             │
                         │ allocs/op  │ allocs/op   vs base            │
MergeSlices/benchmark-10   0.000 ± 0%   0.000 ± 0%  ~ (p=1.000 n=10) ¹
¹ all samples are equal

Verification

  • Added unit tests
  • Added benchmarks

Copy link
Contributor

@yeya24 yeya24 left a comment

Choose a reason for hiding this comment

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

Looks great. Can you add a benchmark test case so that we can understand how much it improves?

@harry671003 harry671003 force-pushed the merge_slices_with_limit branch 4 times, most recently from c8e4efb to 01e0561 Compare September 9, 2024 14:49
Copy link
Contributor

@yeya24 yeya24 left a comment

Choose a reason for hiding this comment

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

Thanks!

Signed-off-by: 🌲 Harry 🌊 John 🏔 <johrry@amazon.com>
@yeya24 yeya24 merged commit f4af8aa into thanos-io:main Sep 13, 2024
22 checks passed
@harry671003 harry671003 deleted the merge_slices_with_limit branch September 13, 2024 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants