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

region: Simplify buffer funcs #258

Merged
merged 1 commit into from
Jun 14, 2024
Merged

region: Simplify buffer funcs #258

merged 1 commit into from
Jun 14, 2024

Conversation

aaronbee
Copy link
Collaborator

@aaronbee aaronbee commented Jun 13, 2024

Replace growBuffer with append and resizeBufferCap with slices.Grow. And remove redundant if checks on capacity or size that append will do as well. This is equivalent behavior after undoing the unintentional change from commit 1fee39f. Benchmarks show the performance is equivalent to the code before commit 1fee39f.

Also replace some deprecated calls to rand funcs.

Replace `growBuffer` with 'append' and `resizeBufferCap` with
`slices.Grow`. And remove redundant 'if' checks on capacity or size
that `append` will do as well. This is equivalent behavior after
undoing the unintentional change from commit 1fee39f. Benchmarks
show the performance is equivalent to the code before commit
1fee39f.

Also replace some deprecated calls to rand funcs.
@aaronbee aaronbee changed the title region: Simplify buffer funcs in compressor.go region: Simplify buffer funcs Jun 13, 2024
Copy link
Collaborator

@dethi dethi left a comment

Choose a reason for hiding this comment

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

LGTM, but QQ: how do you know the change to resizeBufferCap was unintentional?

@aaronbee
Copy link
Collaborator Author

LGTM, but QQ: how do you know the change to resizeBufferCap was unintentional?

I spoke to @tsuna about it. He has no recollection of making that change and didn't mean to submit it with the other unrelated changes in #255.

It causeed a slow down because an append was replaced with a make to create a slice. make doesn't leave any slack at the end of the slice so it's more likely to need to be reallocated:

goos: darwin
goarch: amd64
pkg: github.com/tsuna/gohbase/region
cpu: Intel(R) Core(TM) i9-9980HK CPU @ 2.40GHz
                                        │ existing.txt │             before.txt              │
                                        │    sec/op    │   sec/op     vs base                │
DecompressCellblocks1/BlockLen10-16        71.08n ± 0%   60.99n ± 0%  -14.20% (p=0.000 n=10)
DecompressCellblocks1/BlockLen100-16       254.3n ± 0%   243.9n ± 1%   -4.07% (p=0.000 n=10)
DecompressCellblocks1/BlockLen1000-16      1.986µ ± 0%   1.961µ ± 0%   -1.21% (p=0.000 n=10)
DecompressCellblocks1/BlockLen10000-16     18.86µ ± 0%   18.81µ ± 0%   -0.24% (p=0.002 n=10)
DecompressCellblocks10/BlockLen10-16       468.3n ± 0%   740.8n ± 0%  +58.18% (p=0.000 n=10)
DecompressCellblocks10/BlockLen100-16      2.811µ ± 1%   3.603µ ± 0%  +28.20% (p=0.000 n=10)
DecompressCellblocks10/BlockLen1000-16     26.17µ ± 1%   29.50µ ± 0%  +12.75% (p=0.000 n=10)
DecompressCellblocks10/BlockLen10000-16    249.3µ ± 1%   268.3µ ± 0%   +7.62% (p=0.000 n=10)
geomean                                    2.955µ        3.221µ        +9.02%

                                        │ existing.txt │               before.txt                │
                                        │     B/op     │     B/op      vs base                   │
DecompressCellblocks1/BlockLen10-16         16.00 ± 0%     16.00 ± 0%         ~ (p=1.000 n=10) ¹
DecompressCellblocks1/BlockLen100-16        112.0 ± 0%     112.0 ± 0%         ~ (p=1.000 n=10) ¹
DecompressCellblocks1/BlockLen1000-16     1.000Ki ± 0%   1.000Ki ± 0%         ~ (p=1.000 n=10) ¹
DecompressCellblocks1/BlockLen10000-16    10.00Ki ± 0%   10.00Ki ± 0%         ~ (p=1.000 n=10) ¹
DecompressCellblocks10/BlockLen10-16        240.0 ± 0%     616.0 ± 0%  +156.67% (p=0.000 n=10)
DecompressCellblocks10/BlockLen100-16     2.641Ki ± 0%   5.719Ki ± 0%  +116.57% (p=0.000 n=10)
DecompressCellblocks10/BlockLen1000-16    40.25Ki ± 0%   56.50Ki ± 0%   +40.37% (p=0.000 n=10)
DecompressCellblocks10/BlockLen10000-16   423.3Ki ± 0%   566.0Ki ± 0%   +33.73% (p=0.000 n=10)
geomean                                   1.914Ki        2.566Ki        +34.06%
¹ all samples are equal

                                        │ existing.txt │               before.txt               │
                                        │  allocs/op   │  allocs/op   vs base                   │
DecompressCellblocks1/BlockLen10-16         1.000 ± 0%    1.000 ± 0%         ~ (p=1.000 n=10) ¹
DecompressCellblocks1/BlockLen100-16        1.000 ± 0%    1.000 ± 0%         ~ (p=1.000 n=10) ¹
DecompressCellblocks1/BlockLen1000-16       1.000 ± 0%    1.000 ± 0%         ~ (p=1.000 n=10) ¹
DecompressCellblocks1/BlockLen10000-16      1.000 ± 0%    1.000 ± 0%         ~ (p=1.000 n=10) ¹
DecompressCellblocks10/BlockLen10-16        4.000 ± 0%   10.000 ± 0%  +150.00% (p=0.000 n=10)
DecompressCellblocks10/BlockLen100-16       5.000 ± 0%   10.000 ± 0%  +100.00% (p=0.000 n=10)
DecompressCellblocks10/BlockLen1000-16      7.000 ± 0%   10.000 ± 0%   +42.86% (p=0.000 n=10)
DecompressCellblocks10/BlockLen10000-16     7.000 ± 0%   10.000 ± 0%   +42.86% (p=0.000 n=10)
geomean                                     2.365         3.162        +33.69%
¹ all samples are equal

This PR restores the previous performance.

@dethi dethi merged commit c100991 into master Jun 14, 2024
2 checks passed
@dethi dethi deleted the bufferfuncs branch June 14, 2024 08:31
@tsuna
Copy link
Owner

tsuna commented Jun 14, 2024

Thanks for fixing my mistake, I'm not sure how I managed to get that change in, when all I meant to do was to add a metric.

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.

None yet

3 participants