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

cmd/compile: big map literal creation performance #43020

Closed
trams opened this issue Dec 5, 2020 · 3 comments
Closed

cmd/compile: big map literal creation performance #43020

trams opened this issue Dec 5, 2020 · 3 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Performance
Milestone

Comments

@trams
Copy link

trams commented Dec 5, 2020

I noticed that creating big maps (>=9 elements) using map literals are slow if values are not constant
So this code

return map[string]float {
    "key1": SOME_COMPUTED_ABOVE_VALUE,
    "key2": SOME_COMPUTED_ABOVE_VALUE,
    // more keys here
    "keyN": SOME_COMPUTED_ABOVE_VALUE,
}

works twice as slow then this one

// some code above
result := make(map[string]float, SIZE) // SIZE >= N
result["key1"] = SOME_COMPUTED_ABOVE_VALUE
result["key2"] = SOME_COMPUTED_ABOVE_VALUE
// more keys here
result["keyN"] = SOME_COMPUTED_ABOVE_VALUE
return result

See a post
https://trams.github.io/golang-map-literal-performance/
plus here are benchmarks I used
https://github.com/trams/goplayground/blob/main/map_make_test.go

I asked about this in go developers mailing list and they asked to create a ticket

@josharian mentioned this line in the code cmd/compile/internal/gc/sinit.go:757

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

go1.15.5 linux

Does this issue reproduce with the latest release?

Yes

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

Linux, AMD64

go env Output
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/alex/.cache/go-build"
GOENV="/home/alex/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/alex/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/alex/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"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/alex/go/src/github.com/trams/goption/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build550590063=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Created a map using map literal

What did you expect to see?

I expected go to allocate enough space for all keys so there won't be any additional allocation and performance would be as good as creating a map manually (and specifying appropriate capacity manually using make)

What did you see instead?

There was not enough space allocated so during creation there was a bunch of additional allocs which is shown by benchmarks

@randall77
Copy link
Contributor

Yes, this is a current limitation of how maps are sized.
We initially size the maps with the number of static entries. An entry is static only when both the key and value are static.
We could really count an entry towards the initial size if just the key is static. The value doesn't have to be.

The organization of this code in the compiler makes it kinda tough, but it should be possible.

@randall77 randall77 added this to the Unplanned milestone Dec 5, 2020
@ALTree ALTree changed the title Big map literal creation performance cmd/compile: big map literal creation performance Dec 5, 2020
@cagedmantis cagedmantis added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Dec 7, 2020
@trams
Copy link
Author

trams commented Feb 4, 2022

I just revisited this with a newer version of go compiler and I still observe this effect with 1.17.6
I thought I would just share this here

@gopherbot
Copy link

Change https://golang.org/cl/383634 mentions this issue: cmd/compile: include all entries in map literal hint size

@dmitshur dmitshur modified the milestones: Unplanned, Go1.19 Mar 4, 2022
@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 Mar 4, 2022
@golang golang locked and limited conversation to collaborators Mar 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Performance
Projects
None yet
Development

No branches or pull requests

5 participants