emersion / go-msgauth

🔏 A Go library and tools for DKIM, DMARC and Authentication-Results
MIT License
162 stars 51 forks source link

perf: Allocation reduction #61

Closed 56KBs closed 7 months ago

56KBs commented 7 months ago

Improve performance/reduce allocations of within DKIM & Authres with changes:

Replace strings.SplitN where N=2 with strings.Index, as this does not complicate the code, but does have the advantage of not needing to allocate a slice of strings with. These changes would look even more simple through the use of strings.Cut, however this was added in Go 1.18 and I didn't want to force a minimum version bump for the sake of this change (Although 1.18 is now out of support) - If this would be preferred I can do this instead.

Additional calls to SplitN where N=2 exist in the codebase, but they are in the milter code, or they are in code that has no unit tests so I have not made any changes there.

I have replaced the use of a regex to reduce the whitespace within header canonicalization, as regex within Go can be quite slow and given the simplistic nature of the regex it can be easily replaced by using FieldsFunc and Join.

$ benchstat.exe master.txt split-enhancements.txt
goos: windows
goarch: amd64
pkg: github.com/emersion/go-msgauth/authres
cpu: AMD Ryzen 5 5600X 6-Core Processor
                │ master.txt  │       split-enhancements.txt        │
                │   sec/op    │   sec/op     vs base                │
AuthresParse-12   8.285µ ± 6%   7.423µ ± 5%  -10.40% (p=0.000 n=10)

                │  master.txt  │        split-enhancements.txt        │
                │     B/op     │     B/op      vs base                │
AuthresParse-12   8.297Ki ± 0%   7.391Ki ± 0%  -10.92% (p=0.000 n=10)

                │ master.txt  │       split-enhancements.txt       │
                │  allocs/op  │ allocs/op   vs base                │
AuthresParse-12   115.00 ± 0%   86.00 ± 0%  -25.22% (p=0.000 n=10)

pkg: github.com/emersion/go-msgauth/dkim
                                           │  master.txt  │       split-enhancements.txt        │
                                           │    sec/op    │   sec/op     vs base                │
RelaxedCanonicalizer_CanonicalizeHeader-12   3164.0n ± 6%   892.1n ± 6%  -71.81% (p=0.000 n=10)
Verify-12                                     46.66µ ± 6%   44.68µ ± 7%   -4.25% (p=0.019 n=10)

                                           │  master.txt  │        split-enhancements.txt        │
                                           │     B/op     │     B/op      vs base                │
RelaxedCanonicalizer_CanonicalizeHeader-12     508.0 ± 0%     416.0 ± 0%  -18.11% (p=0.000 n=10)
Verify-12                                    16.16Ki ± 0%   14.83Ki ± 0%   -8.20% (p=0.000 n=10)

                                           │ master.txt │       split-enhancements.txt       │
                                           │ allocs/op  │ allocs/op   vs base                │
RelaxedCanonicalizer_CanonicalizeHeader-12   28.00 ± 0%   16.00 ± 0%  -42.86% (p=0.000 n=10)
Verify-12                                    157.0 ± 0%   115.0 ± 0%  -26.75% (p=0.000 n=10)
emersion commented 7 months ago

In principle these changes look like a good idea to me! I haven't looked at the code yet, will get to it in the next few days.

56KBs commented 7 months ago

Here are a couple examples of the changes making use of strings.Cut from Go 1.18+.

authres/parse.go

func parseParam(s string) (k string, v string, err error) {
    key, value, ok := strings.Cut(s, "=")
    if !ok {
        return "", "", errors.New("msgauth: malformed authentication method and value")
    }

    return strings.TrimSpace(strings.ToLower(key)), strings.TrimSpace(value), nil
}

dkim/verify.go

    // Parse algos
    keyAlgo, hashAlgo, ok := strings.Cut(stripWhitespace(params["a"]), "-")
    if !ok {
        return verif, permFailError("malformed algorithm name")
    }
emersion commented 7 months ago

These changes would look even more simple through the use of strings.Cut, however this was added in Go 1.18 and I didn't want to force a minimum version bump for the sake of this change (Although 1.18 is now out of support) - If this would be preferred I can do this instead.

I would be completely okay with bumping our minimum required Go version to 1.18.

emersion commented 7 months ago

Code LGTM either way