-
Notifications
You must be signed in to change notification settings - Fork 17.9k
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: We can avoid extra mapaccess in "m[k] op= r" #23661
Comments
Change https://golang.org/cl/91557 mentions this issue: |
Related: #17133 |
As a proof of patch, generated Go's Assembler for test.go:
|
There is no runtime.mapaccess1_fast in Go's asm
|
Added a benchmark tests go/test/bench/go1/map_test.go, 3 runs of "go/bin/go test -bench Map" Before optimization: After optimization: All 3 runs demonstrates minor improvement for "insert" and ~10% or "update". This is expected. |
What version of Go are you using (
go version
)?1.9.2
Does this issue reproduce with the latest release?
yes
What operating system and processor architecture are you using (
go env
)?darwin_amd64
What did you do?
Optimization for map[i] op= r, except /= and %= (it has possibility of /= 0 and %= 0 that we can not optimize using proposed strategy)
What did you expect to see?
"map[k] op= r" should be using only single call:
func mapassign(t *maptype, h *hmap, key unsafe.Pointer) unsafe.Pointer
What did you see instead?
"map[k] op= r" is using:
func mapaccess*(..) unsafe.Pointer
func mapassign*(...) unsafe.Pointer
We can avoid mapaccess in most cases (except /=).
Description (will simplify some details that are not important here):
This is slightly related to #21687
Currently orderexpr performs transformation of node with OP OASOP (op=) into OAS (=):
l op= r into l = l op r
For map
m[k] op= r
It creates safe expressions:
tmp1 := m[k]
m[k] = tmp1 op r
then walkexpr replaces it with:
tmp1 := mapaccess(m, k) // this never changes m but returns default value if necessary.
tmp2 := massign(m, k) // inserts if necessary
tmp2 = tmp1 op r
We always evaluate r before massign (see test fixedbugs/issue22881.go).
Consider this, mapaccess is not necessary.
Except the m[k] /= r , you may see example at fixedbugs/issue22881.go, f6 where r == 0 causes panic. Other Op doesn't have this problem.
Implementation trick is to do it during walkexpr when m[k] is already replaced with massign.
The text was updated successfully, but these errors were encountered: