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

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

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"

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.
@inkel
Copy link
Contributor Author

inkel commented Sep 25, 2024

@hairyhenderson done!

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