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.

Sign up for GitHub

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

Jump to bottom

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

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

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

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

Comments

@sdmnebulon
Copy link

sdmnebulon commented Aug 31, 2023 •
edited by panjf2000

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 •
edited

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
@mauri870
sync: prevent (*Map).Range from always escaping
88c6787 
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 mauri870 mentioned this issue Sep 1, 2023
Closed
@gopherbot
Copy link

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
@mauri870
sync: prevent (*Map).Range from always escaping
508a10e 
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
@mauri870
sync: prevent (*Map).Range from always escaping
fcbedb4 
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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. 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
Milestone
Backlog
Development

Successfully merging a pull request may close this issue.

sync: prevent (*Map).Range from always escaping mauri870/go
5 participants
@ianlancetaylor @bcmills @gopherbot @mauri870 @sdmnebulon