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: explicit zeroing memory returned by newobject #19027

Closed
mdempsky opened this issue Feb 10, 2017 · 7 comments
Closed

cmd/compile: explicit zeroing memory returned by newobject #19027

mdempsky opened this issue Feb 10, 2017 · 7 comments
Labels
FrozenDueToAge
Milestone
Go1.9

Comments

@mdempsky
Copy link
Member

mdempsky commented Feb 10, 2017 •
edited

Compiling this code at 249aca5:

package p

type x [256][]byte

//go:noinline
func (*x) m() {}

func f() {
        a := x{}
        a.m()
}

generates a call to newobject (to heap allocate a) followed by code to zero it out (e.g., the call to typedmemmove).

There's a couple related questions:

  1. newobject returns zero'd memory, so why are we still generating explicit zeroing code? Replacing a := x{} with var a x eliminates the explicit zero'ing.
  2. typedmemclr would be better for explicit zero'ing, and CL 34566 actually fixes this... except I'm not sure why. (Edit: @cherrymui pointed out why in the CL.)
  3. Why is a being heap allocated anyway? Shouldn't escape analysis be able to identify that (*x).m's parameters don't escape? (Edit: This appears to be cmd/compile: argument escapes when it should not #18001.)

/cc @randall77 @cherrymui @josharian @aclements @dr2chase
@mdempsky mdempsky added this to the Go1.9 milestone Feb 10, 2017
@cherrymui
Copy link
Member

It still uses statictmp probably because CL https://go-review.googlesource.com/c/35261/ doesn't cover array?

I'm working on the new write barrier insertion code for #17583 (almost ready). Along that line I have prepared some changes that will remove the explicit zeroing (given that it doesn't use statictmp).

For the escape analysis, probably related to #18001?

@mdempsky
Copy link
Member Author

It still uses statictmp probably because CL https://go-review.googlesource.com/c/35261/ doesn't cover array?

That explains why we were using statictmp originally. I don't understand though why s/OASWB/OAS/ would cause us to stop using statictmp though, since it's still an array literal assignment.

@mdempsky
Copy link
Member Author

For the escape analysis, probably related to #18001?

Yes, that seems to be the same bug.

@cherrymui
Copy link
Member

That explains why we were using statictmp originally. I don't understand though why s/OASWB/OAS/ would cause us to stop using statictmp though, since it's still an array literal assignment.

I replied in your CL.

@gopherbot
Copy link

CL https://golang.org/cl/36836 mentions this issue.

gopherbot pushed a commit that referenced this issue Mar 3, 2017
@cherrymui
cmd/compile: remove zeroing after newobject
c8eaeb8 
The Zero op right after newobject has been removed. But this rule
does not cover Store of constant zero (for SSA-able types). Add
rules to cover Store op as well.

Updates #19027.

Change-Id: I5d2b62eeca0aa9ce8dc7205b264b779de01c660b
Reviewed-on: https://go-review.googlesource.com/36836
Run-TryBot: Cherry Zhang <cherryyz@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
@josharian
Copy link
Contributor

@cherrymui is this fixed? If not, what remains to be done?

@cherrymui
Copy link
Member

I believe this is fixed. Thanks.
Closing.

@golang golang locked and limited conversation to collaborators May 18, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
Go1.9
Development

No branches or pull requests
4 participants
@mdempsky @josharian @gopherbot @cherrymui