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

Jump to bottom

cmd/compile: map literals should be allocated on the stack if not escaping #21830

Closed
martisch opened this issue Sep 10, 2017 · 9 comments
Closed

cmd/compile: map literals should be allocated on the stack if not escaping #21830

martisch opened this issue Sep 10, 2017 · 9 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.10

Comments

@martisch
Copy link
Contributor

martisch commented Sep 10, 2017 •
edited

tip and go1.9, go1.8.3 (earlier releases not tested)

package main

func main() {
	m := map[int]int{}
	m[0] = 0
}

./main.go:4:18: main map[int]int literal does not escape

However looking at the disassembly reveals that the hmap struct was not allocated on the stack.
Since the hmap pointer passed as argument to makemap is nil.

main.go:4 0x104f6e4 48890424 MOVQ AX, 0(SP)
main.go:4 0x104f6e8 48c744240800000000 MOVQ $0x0, 0x8(SP)
main.go:4 0x104f6f1 48c744241000000000 MOVQ $0x0, 0x10(SP) <- *hmap
main.go:4 0x104f6fa 48c744241800000000 MOVQ $0x0, 0x18(SP)
main.go:4 0x104f703 e8d874fbff CALL runtime.makemap(SB)

This is due to the escape information of the maplit not being copied to the OMAKE node
in maplit in cmd/compile/internal/sinit.go.

Found this while writing tests for a new makemap implementation CL.

Fix incoming.

Clarification:

Map literals were always allocated on the heap as if they were escaping even so the escape pass marked them as non escaping. This is not a correctness problem or will lead to corruption but there was a performance gain that was missed by not allocating map literals on the stack. Passing a nil hmap pointer to makemap just makes makemap allocate the hmap on the heap instead.
@martisch martisch added this to the Go1.10 milestone Sep 10, 2017
@martisch martisch self-assigned this Sep 10, 2017
@martisch martisch added the NeedsFix The path to resolution is known, but the work has not been done. label Sep 10, 2017
@gopherbot
Copy link

Change https://golang.org/cl/62790 mentions this issue: cmd/compile: preserve escape information for map literals

@odeke-em
Copy link
Member

@martisch since this is a regression from Go1.8, should we be backporting this to Go1.9.1?

@randall77
Copy link
Contributor

@odeke-em , probably not. I don't think performance regressions should be backported unless they are egregious.

@odeke-em
Copy link
Member

Ahh I see, thanks @randall77 for this update on some criteria for backporting.

@martisch
Copy link
Contributor Author

I agree with not backporting. The performance regression is not big since non escaping map literals do not come up that often as far as i have seen. It generates work and has potential to break other parts of the distribution.

It is also not a regression from 1.8. I only tested it with 1.9 and not further back. I updated the original report.

@odeke-em
Copy link
Member

Roger that @martisch, thanks.

@davecheney
Copy link
Contributor

davecheney commented Sep 11, 2017 via email

@martisch
Copy link
Contributor Author

martisch commented Sep 11, 2017 •
edited

The issue #21830 that the CL 62790 fixes and was referenced in the CL is explained above. Map literals were always allocated on the heap as if they were escaping even so the escape pass marked them as non escaping. This is not a correctness problem or will lead to corruption but there was a performance gain that was missed by not allocating map literals on the stack. Passing a nil hmap pointer to makemap just makes makemap allocate the hmap on the heap instead. In hindsight i could have made that clearer in the description and title to avoid confusion. Updated the title and added more description. Please let me know if my analyses of the effects of the problem or description is wrong or unclear.

The part before the "breaks" sentence is: "has potential to" and the "It" refers to backporting. I wanted to say that backporting any change in general from tip to e.g. 1.9 has potential of side effects that may not be present at tip due to other changes interacting with CLs at tip. I am not saying this is the case here but in general there is always a risk. So backporting in CLs in general creates additional testing work and risks introducing CLs that themselfs introduce new regressions. Which means there should be enough reason to offset these risks.

Since this issue is not an issue that could cause programs to behave wrong according to the spec (as far as i know) but more a performance benefit i do not think this performance improvement warrants a backport.

The possibility of having stack allocated map literals after the CL is submitted however might lead to an issue (i dont know of any but there always is a potential) that some compiler part or optimization that has never dealt with stack allocated map literals before in 1.9 interacts badly with the backport. Again i am not saying this is the case here i am just saying why risk a backport if there is no correctness problem in relation to this CL for 1.9 but the backport might risk some bad interactions whatever they might be.

[Updated the text and comment after first submission of this comment]

@martisch martisch changed the title cmd/compile: map literals are never allocated on the stack cmd/compile: map literals should be allocated on the stack if not escaping Sep 11, 2017
@davecheney
Copy link
Contributor

davecheney commented Sep 11, 2017 via email

@golang golang locked and limited conversation to collaborators Sep 11, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.10
Development

No branches or pull requests
5 participants
@davecheney @martisch @odeke-em @randall77 @gopherbot