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

compare with EqualFold instead of ToLower #48

Closed
pierrre opened this issue Mar 31, 2021 · 2 comments · Fixed by #49
Closed

compare with EqualFold instead of ToLower #48

pierrre opened this issue Mar 31, 2021 · 2 comments · Fixed by #49

Comments

@pierrre
Copy link
Contributor

pierrre commented Mar 31, 2021

I did a profiling of my application, and noticed that msgauth/dkim is comparing strings with ToLower.

go-msgauth/dkim/header.go

Lines 152 to 158 in e98a2ee

key = strings.ToLower(key)
at := p.picked[key]
for i := len(p.h) - 1; i >= 0; i-- {
kv := p.h[i]
k, _ := parseHeaderField(kv)
if strings.ToLower(k) != key {

if strings.ToLower(k) == "from" {

Each time this function is called, it allocates a new string, which creates more work for the garbage collector.
I think there is an "allocation free" alternative: EqualFold.

WDYT ?
I can submit a benchmark if needed.

@emersion
Copy link
Owner

Sure, EqualFold LGTM

@pierrre
Copy link
Contributor Author

pierrre commented Mar 31, 2021

For reference, here is a very simple micro-benchmark:

package main

import (
	"strings"
	"testing"
)

func BenchmarkToLower(b *testing.B) {
	for i := 0; i < b.N; i++ {
		if strings.ToLower("ABCDEFGHIJ") != "abcdefghij" {
			b.Fatal("not equal")
		}
	}
}

func BenchmarkEqualFold(b *testing.B) {
	for i := 0; i < b.N; i++ {
		if !strings.EqualFold("ABCDEFGHIJ", "abcdefghij") {
			b.Fatal("not equal")
		}
	}
}

The result

BenchmarkToLower
BenchmarkToLower-12      	26062184	        42.60 ns/op	      16 B/op	       1 allocs/op
BenchmarkEqualFold
BenchmarkEqualFold-12    	41775878	        27.07 ns/op	       0 B/op	       0 allocs/op

I tried to integrate the benchmark to msgauth, but I don't know a simple way to write it.

emersion pushed a commit that referenced this issue Mar 31, 2021
It doesn't allocate a new string each time.

Fixes #48
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 a pull request may close this issue.

2 participants