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 allocations #39

Merged
merged 3 commits into from
Sep 27, 2024
Merged

Conversation

inkel
Copy link
Contributor

@inkel inkel commented Sep 4, 2024

This PR reduces the allocations in getPattern and it also improves the overall speed of the function.

First note that I had to fix the existing benchmark, as the reader was being read only once in the first iteration, and afterwards it was always empty, thus parseCodeowners was parsing an empty file.

These were the metrics before the change:

$ go test -benchmem -bench=.
goos: darwin
goarch: arm64
pkg: github.com/hairyhenderson/go-codeowners
BenchmarkParseCodeowners-10      2677503               465.3 ns/op          4096 B/op          1 allocs/op
PASS
ok      github.com/hairyhenderson/go-codeowners 2.255s

After the change it's not showing more realistic data:

$ go test -benchmem -bench=.
goos: darwin
goarch: arm64
pkg: github.com/hairyhenderson/go-codeowners
BenchmarkParseCodeowners-10        31054             38910 ns/op           61424 B/op        641 allocs/op
PASS
ok      github.com/hairyhenderson/go-codeowners 3.529s

Then we move out of getPattern all regexp compilations, reducing the allocations ~62% and the performance in ~55%. Finally uses strings.ReplaceAll instead of the regexp versions when possible. These are the end results:

$ benchstat main.log stringsreplace.log
goos: darwin
goarch: arm64
pkg: github.com/hairyhenderson/go-codeowners
                   │  main.log   │         stringsreplace.log          │
                   │   sec/op    │   sec/op     vs base                │
ParseCodeowners-10   38.72µ ± 2%   15.21µ ± 7%  -60.73% (p=0.000 n=10)

                   │   main.log   │          stringsreplace.log          │
                   │     B/op     │     B/op      vs base                │
ParseCodeowners-10   59.95Ki ± 0%   25.98Ki ± 0%  -56.66% (p=0.000 n=10)

                   │  main.log  │         stringsreplace.log         │
                   │ allocs/op  │ allocs/op   vs base                │
ParseCodeowners-10   641.0 ± 0%   194.0 ± 0%  -69.73% (p=0.000 n=10)

@inkel
Copy link
Contributor Author

inkel commented Sep 4, 2024

I honestly don't know what the CI error is about 🤔

@hairyhenderson
Copy link
Owner

@inkel you'll need to amend the commit message(s) to match conventional commit patterns - maybe something like "fix(perf): Reduce allocations"

@inkel inkel force-pushed the reduce-allocations branch 2 times, most recently from c23d5d6 to 483c183 Compare September 25, 2024 18:40
@inkel
Copy link
Contributor Author

inkel commented Sep 25, 2024

@hairyhenderson done!

The reader was being read only once in the first iteration, and
afterwards it was always empty, thus `parseCodeowners` was parsing an
empty file.

These were the metrics before the change:

    $ go test -benchmem -bench=.
    goos: darwin
    goarch: arm64
    pkg: github.com/hairyhenderson/go-codeowners
    BenchmarkParseCodeowners-10      2677503               465.3 ns/op          4096 B/op          1 allocs/op
    PASS
    ok      github.com/hairyhenderson/go-codeowners 2.255s

After the change it's not showing more realistic data:

    $ go test -benchmem -bench=.
    goos: darwin
    goarch: arm64
    pkg: github.com/hairyhenderson/go-codeowners
    BenchmarkParseCodeowners-10        31054             38910 ns/op           61424 B/op        641 allocs/op
    PASS
    ok      github.com/hairyhenderson/go-codeowners 3.529s
This greatly improves the allocations:

    $ go test -benchmem -bench=.
    goos: darwin
    goarch: arm64
    pkg: github.com/hairyhenderson/go-codeowners
    BenchmarkParseCodeowners-10        70927             16857 ns/op           27211 B/op        242 allocs/op
    PASS
    ok      github.com/hairyhenderson/go-codeowners 5.297s

This reduces the allocations in 62%:

    $ benchstat main.log precompile.log
    goos: darwin
    goarch: arm64
    pkg: github.com/hairyhenderson/go-codeowners
                       │  main.log   │           precompile.log            │
                       │   sec/op    │   sec/op     vs base                │
    ParseCodeowners-10   38.72µ ± 2%   17.63µ ± 6%  -54.46% (p=0.000 n=10)

                       │   main.log   │            precompile.log            │
                       │     B/op     │     B/op      vs base                │
    ParseCodeowners-10   59.95Ki ± 0%   26.56Ki ± 0%  -55.70% (p=0.000 n=10)

                       │  main.log  │           precompile.log           │
                       │ allocs/op  │ allocs/op   vs base                │
    ParseCodeowners-10   641.0 ± 0%   242.0 ± 0%  -62.25% (p=0.000 n=10)
These functions are faster than the regexp ones.
Copy link
Owner

@hairyhenderson hairyhenderson left a comment

Choose a reason for hiding this comment

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

Thanks for this @inkel!

@hairyhenderson hairyhenderson merged commit 16522d7 into hairyhenderson:main Sep 27, 2024
5 checks passed
hairyhenderson pushed a commit that referenced this pull request Sep 27, 2024
* fix(perf): Fix benchmark

The reader was being read only once in the first iteration, and
afterwards it was always empty, thus `parseCodeowners` was parsing an
empty file.

These were the metrics before the change:

    $ go test -benchmem -bench=.
    goos: darwin
    goarch: arm64
    pkg: github.com/hairyhenderson/go-codeowners
    BenchmarkParseCodeowners-10      2677503               465.3 ns/op          4096 B/op          1 allocs/op
    PASS
    ok      github.com/hairyhenderson/go-codeowners 2.255s

After the change it's not showing more realistic data:

    $ go test -benchmem -bench=.
    goos: darwin
    goarch: arm64
    pkg: github.com/hairyhenderson/go-codeowners
    BenchmarkParseCodeowners-10        31054             38910 ns/op           61424 B/op        641 allocs/op
    PASS
    ok      github.com/hairyhenderson/go-codeowners 3.529s

* feat(perf): Precompile getPattern regexps

This greatly improves the allocations:

    $ go test -benchmem -bench=.
    goos: darwin
    goarch: arm64
    pkg: github.com/hairyhenderson/go-codeowners
    BenchmarkParseCodeowners-10        70927             16857 ns/op           27211 B/op        242 allocs/op
    PASS
    ok      github.com/hairyhenderson/go-codeowners 5.297s

This reduces the allocations in 62%:

    $ benchstat main.log precompile.log
    goos: darwin
    goarch: arm64
    pkg: github.com/hairyhenderson/go-codeowners
                       │  main.log   │           precompile.log            │
                       │   sec/op    │   sec/op     vs base                │
    ParseCodeowners-10   38.72µ ± 2%   17.63µ ± 6%  -54.46% (p=0.000 n=10)

                       │   main.log   │            precompile.log            │
                       │     B/op     │     B/op      vs base                │
    ParseCodeowners-10   59.95Ki ± 0%   26.56Ki ± 0%  -55.70% (p=0.000 n=10)

                       │  main.log  │           precompile.log           │
                       │ allocs/op  │ allocs/op   vs base                │
    ParseCodeowners-10   641.0 ± 0%   242.0 ± 0%  -62.25% (p=0.000 n=10)

* feat(perf): Replace regexp replace with strings replace

These functions are faster than the regexp ones.
@inkel inkel deleted the reduce-allocations branch September 27, 2024 19:30
@inkel
Copy link
Contributor Author

inkel commented Sep 27, 2024

Happy to contribute!

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