/cc @aclements @randall77

@howardjohn Thanks so much for the issue and the detailed information. Looking into it.

@howardjohn thank you for the report! Just a comment that in the future you can also use benchstat https://godoc.org/golang.org/x/perf/cmd/benchstat for an isolated summary of benchmark results.

@howardjohn I can reproduce, and I think I know the source of the problem.

First let me say that I tried pretty hard to reproduce the profiles you attached but I was unable to using the commands in the original post. Every time I compared 1.14 and 1.13 profiles the diff between them was completely different. Part of the problem is that because the number of iterations isn't fixed, each run could be doing different amounts of work. It's harder to compare along these lines.

Next, I need to point out that there's a memory leak in the benchmark. Every time BenchmarkEDS gets invoked, the heap size increases. This is especially noticeable with GODEBUG=gctrace=1 running and -count=N where N>1, but the leak also affects N=1 if the number of benchmark iterations are not fixed (since BenchmarkEDS gets called multiple times by the testing framework).

So, given this, I collected profiles on the BenchmarkEDS/100/100 benchmark with -benchtime=1000x to fix the iteration count. This is roughly the same amount of work as -benchtime=10s.

In the end, I consistently get two profiles which implicate mapaccess1, runtime.interhash, and callees as the culprit in the diff. Specifically, aeshash64 is gone in the 1.14 profile, replaced with aeshashbody (called through runtime.typehash) which takes more time.

For documentation purposes, the command I ran (after cloning istio at the PR mentioned above; if you don't do that the benchmark won't run) was:

perflock go test ./pilot/pkg/proxy/envoy/v2/ -bench=BenchmarkEDS/100$/100$ -count=1 -run=^$ -benchmem -benchtime=1000x -cpuprofile=cpu.prof

Pinging @randall77 since @cherrymui suggested you were involved in some map changes in the Go 1.14 cycle, and I don't know much about the map implementation.

Uses of runtime.aeshash64 in Go 1.13 should have been replaced by uses of runtime.memhash64 in Go 1.14.

Oh, I see, this program must be using hash maps with keys of interface types. See https://golang.org/cl/191198. Perhaps we should have a release note for that?

@ianlancetaylor Yeah that's right. @cherrymui and I dug a little deeper and it's actually map with keys of type reflect.Type that is causing >70% of the mapaccess1 calls. :)

Might be worth adding a special case for pointers in runtime.typehash, to call the sized memhash function rather than the general one. For 1.14.1.

Certainly we could swap the order of the regular memory check and the kind switch, and add a kindPtr case.

I think if regular memory check is true, we could special case 32-bit and 64-bit sizes?

i am not sure if this is related, but i saw some differences in map access performance, too. In my case I am not using interface{} as a key, but instead a map[int]int

https://github.com/SlinSo/random/blob/master/mapaccess_test.go

pprof excerpt:
go1.13.8:
0.73s 9.53% 82.64% 0.73s 9.53% runtime.aeshash64

go1.14rc1:
1230ms 15.81% 66.20% 1410ms 18.12% runtime.memhash64Fallback
140ms 1.80% 97.43% 140ms 1.80% runtime.memhash64

That's a bug. Yikes!
Fix coming.

(Unrelated to this issue, though.)

Change https://golang.org/cl/219338 mentions this issue: runtime: special case interface hashing for pointers

@randall77 is the intent for this to be tracked as a back port candidate? Ian had mentioned in #37086 (comment)

Might be worth adding a special case for pointers in runtime.typehash, to call the sized memhash function rather than the general one. For 1.14.1.

@gopherbot please open a backport issue.

I don't think this performance regression qualifies for a backport, but I'll let the release guys make that call.

Backport issue(s) opened: #37420 (for 1.12), #37421 (for 1.13).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases.

@randall77 oddly it didn't backport for Go1.14.1, it only created PRs for 1.12 and 1.13.

Made #37613 for 1.14.