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

hash/maphash: Comparable[string] causes unnecessary allocation #70560

Closed
aclements opened this issue Nov 25, 2024 · 3 comments
Closed

hash/maphash: Comparable[string] causes unnecessary allocation #70560

aclements opened this issue Nov 25, 2024 · 3 comments
Labels
NeedsFix The path to resolution is known, but the work has not been done. Performance
Milestone

Comments

@aclements
Copy link
Member

Go version

go version devel go1.24-fb5fa2a839 Mon Nov 25 18:39:27 2024 +0000 linux/amd64

Output of go env in your module/workspace:

AR='ar'
CC='gcc'
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_ENABLED='1'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
CXX='g++'
GCCGO='/usr/bin/gccgo'
GO111MODULE=''
GOAMD64='v1'
GOARCH='amd64'
GOAUTH='netrc'
GOBIN=''
GOCACHE='/home/austin/.cache/go-build'
GODEBUG=''
GOENV='/home/austin/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFIPS140='off'
GOFLAGS=''
GOGCCFLAGS='-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build1361679833=/tmp/go-build -gno-record-gcc-switches'
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMOD='/dev/null'
GOMODCACHE='/home/austin/r/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='linux'
GOPATH='/home/austin/r/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/home/austin/go.tmp'
GOSUMDB='sum.golang.org'
GOTELEMETRY='on'
GOTELEMETRYDIR='/home/austin/.config/go/telemetry'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/home/austin/go.tmp/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='devel go1.24-fb5fa2a839 Mon Nov 25 18:39:27 2024 +0000'
GOWORK=''
PKG_CONFIG='pkg-config'

What did you do?

I ran the following using go test -bench . comparable_test.go: https://play.golang.org/p/jv23SKNWLy6

What did you see happen?

I saw the following results:

goos: linux
goarch: amd64
cpu: 11th Gen Intel(R) Core(TM) i7-1185G7 @ 3.00GHz
BenchmarkMaphashString-8   	30859322	        38.03 ns/op	       2 B/op	       1 allocs/op
BenchmarkMapString-8       	55735060	        20.58 ns/op	       0 B/op	       0 allocs/op
BenchmarkMaphashInt-8      	173806796	         7.706 ns/op	       0 B/op	       0 allocs/op
PASS

Notably, BenchmarkMaphashString has one allocation per op.

What did you expect to see?

I expected to see zero allocations per iteration in any of these benchmarks because the result of hashing a string is independent of that string's address. I discussed this issue at length in #54670 (comment) (possibly at too much length!). While the current implementation deals with the issue of stack pointers by unconditionally escaping them, it doesn't currently handle the more subtle case of strings, as demonstrated by this benchmark.

cc @randall77 @golang/compiler

@aclements aclements added this to the Go1.25 milestone Nov 25, 2024
@GoVeronicaGo GoVeronicaGo added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Nov 25, 2024
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/632715 mentions this issue: hash/maphash, cmd/compile: make Comparable[string] not escape its argument

@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Dec 2, 2024
@cherrymui
Copy link
Member

Discussed with @dmitshur , polishing a new feature (to make it more conformant to the proposal) is within the scope of the freeze. So we can target 1.24 for this.

@cherrymui cherrymui modified the milestones: Go1.25, Go1.24 Dec 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsFix The path to resolution is known, but the work has not been done. Performance
Projects
None yet
Development

No branches or pull requests

6 participants