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

sync: map.Range has started allocating as of Go 1.20 #62404

Closed
ghost opened this issue Aug 31, 2023 · 7 comments
Closed

sync: map.Range has started allocating as of Go 1.20 #62404

ghost opened this issue Aug 31, 2023 · 7 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance
Milestone

Comments

@ghost
Copy link

ghost commented Aug 31, 2023

What version of Go are you using (go version)?

$ go version
go version go1.20.4 linux/amd64

Does this issue reproduce with the latest release?

Yes (with go 1.21.0)

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE="on"
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/murthy/.cache/go-build"
GOENV="/home/murthy/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/murthy/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/murthy/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.20.4"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/dev/null"
GOWORK=""
CGO_CFLAGS="-O2 -g"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-O2 -g"
CGO_FFLAGS="-O2 -g"
CGO_LDFLAGS="-O2 -g"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build2467870538=/tmp/go-build -gno-record-gcc-switches"

What did you do?

The latest versions of go runtime added a 16Byte allocation to sync map Range function. This was not the case in prior releases.
I have tested go 1.19.5 (which does not do the allocation) and go 1.20.4 and go 1.21.0 (both of which do the allocation).

package smap

import (
	"sync"
	"testing"
)

var myMap sync.Map

func BenchmarkRange(b *testing.B) {
	b.ReportAllocs()
	b.ResetTimer()

	for i := 0; i < b.N; i++ {
		myMap.Range(func(k interface{}, v interface{}) bool {
			return true
		})

	}
}

func init() {
	for ind := 0; ind < 100; ind++ {
		myMap.Store(ind, (ind*2 + 1))
	}
}

What did you expect to see?

[murthy@testbuild] 12:35:32 $ /usr/local/go1.19.5/bin/go test -bench Range
goos: linux
goarch: amd64
pkg: smap/go1195
cpu: AMD EPYC 7352 24-Core Processor
BenchmarkRange-16    	  840378	      1418 ns/op	       0 B/op	       0 allocs/op
PASS
ok  	smap/go1195	2.041s
[murthy@testbuild] 12:35:39 $

What did you see instead?

[murthy@testbuild] 12:36:03 $ go version
go version go1.20.4 linux/amd64
[murthy@testbuild] 12:36:05 $ go test -bench Range
goos: linux
goarch: amd64
pkg: smap/go1204
cpu: AMD EPYC 7352 24-Core Processor
BenchmarkRange-16    	  782552	      1512 ns/op	      16 B/op	       1 allocs/op
PASS
ok  	smap/go1204	2.029s
[murthy@testbuild] 12:36:12 $
@ianlancetaylor ianlancetaylor changed the title syncaffected/package: sync: map.Range has started allocating as of Go 1.20 Aug 31, 2023
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Aug 31, 2023
@ianlancetaylor
Copy link
Contributor

CC @bcmills

@ianlancetaylor ianlancetaylor added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Aug 31, 2023
@ianlancetaylor ianlancetaylor added this to the Backlog milestone Aug 31, 2023
@mauri870
Copy link
Member

Seems like this allocation comes from the change from atomic.Value to atomic.Pointer in this CL https://go-review.googlesource.com/c/go/+/426074

@ianlancetaylor
Copy link
Contributor

CC @dsnet

@mauri870
Copy link
Member

mauri870 commented Sep 1, 2023

Specifically, what is allocating memory is the empty value for the return:

func (m *Map) loadReadOnly() readOnly {
	if p := m.read.Load(); p != nil {
		return *p
	}
	return readOnly{}
}

@bcmills
Copy link
Contributor

bcmills commented Sep 1, 2023

Ah, probably escape analysis. What we need here is some manual memory management. 🙃

The line in Range that says:

m.read.Store(&read)

should be changed to instead make a copy of the variable, and store a pointer to that copy so that it only escapes in that one code path. (I guess the compiler doesn't do live-range splitting for local variables.)

mauri870 added a commit to mauri870/go that referenced this issue Sep 1, 2023
After the change from CL 426074 the Range method on Map always escape
the read variable, generating an allocation.

Since the compiler doesn't do live-range splitting for local variables
we need to use some hints to only escape in that particular branch.

Fixes golang#62404
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/524976 mentions this issue: sync: prevent (*Map).Range from always escaping

mauri870 added a commit to mauri870/go that referenced this issue Sep 1, 2023
After the change from CL 426074 the Range method on Map always escape
the read variable, generating an allocation.

Since the compiler doesn't do live-range splitting for local variables
we need to use some hints to only escape in that particular branch.

Fixes golang#62404
mauri870 added a commit to mauri870/go that referenced this issue Sep 1, 2023
After the change from CL 426074 the Range method on Map always escape
the read variable, generating an allocation.

Since the compiler doesn't do live-range splitting for local variables
we need to use some hints to only escape in that particular branch.

Fixes golang#62404
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/525235 mentions this issue: sync: eliminate the intermediate variable in map.Range

@golang golang locked and limited conversation to collaborators Sep 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants