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

Variable escapes to the heap when passed to sync/atomic function #31509

Closed
davecheney opened this issue Apr 17, 2019 · 6 comments
Closed

Variable escapes to the heap when passed to sync/atomic function #31509

davecheney opened this issue Apr 17, 2019 · 6 comments
Labels
FrozenDueToAge WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Milestone
Go1.13

Comments

@davecheney
Copy link
Contributor

davecheney commented Apr 17, 2019 •
edited

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

% go version
go version devel +33e5da48d5 Wed Apr 17 00:05:41 2019 +0000 darwin/amd64

Does this issue reproduce with the latest release?

yes

What did you do?

package main

import (
        "sync/atomic"
)

// counter holds an atomically incrementing counter.
type counter uint64

func (c *counter) get() uint64 {
        return atomic.LoadUint64((*uint64)(c))
}
func (c *counter) inc() uint64 {
        return atomic.AddUint64((*uint64)(c), 1)
}
func (c *counter) reset() uint64 {
        return atomic.SwapUint64((*uint64)(c), 0)
}

func f() uint64 {
        var c counter
        c.inc()
        c.get()
        return c.reset()
}

func main() {
        f()
}

What did you expect to see?

c does not escape the scope of f()

What did you see instead?

./counter.go:10:7: leaking param: c
./counter.go:13:7: leaking param: c
./counter.go:16:7: leaking param: c
./counter.go:21:6: moved to heap: c
./counter.go:28:3: moved to heap: c
@randall77
Copy link
Contributor

We could fix this by annotating those functions as non-escaping.

Why is this an issue, though? There's no reason to be using atomic operations on the stack. That memory is not accessible by more than one thread. If anything, maybe this issue should be "replace sync/atomic operations on stack variables with regular loads and stores"? (Kinda half joking, but the motivation here escapes me.)

Is there a library or something that is using these operations, that you want to call using the address of a stack object? It's unclear to me from your example what that might be.

@davecheney
Copy link
Contributor Author

@randall77 thanks for your reply. This was an example intended to show to myself that mid stack inlining and atomic intrinsics reduce the final output of counter.inc (for example) to a locked xaddq. The only weird thing was the call to newobject to push c onto the heap. But you're right that theres no value in using atomics on stack variables; as you highlight, the memory is not shared with another thread.

Taking a trip through sync/atomic and runtime/internal/atomic it's a sea of forwarding assembly and annotations on functions that are overridden by intrinsics. Perhaps the fix is to add no escape to the Load* variants in runtime/internal/atomic/atomic_amd64x.go (maybe others)?

@agnivade
Copy link
Contributor

Dup of #31509 ?

@cespare
Copy link
Contributor

cespare commented Apr 18, 2019

Dup of #31509 ?

That's this one.

@randall77
Copy link
Contributor

The routines in runtime/internal/atomic are already marked as noescape. I think to fix this we'd need noescape annotations in sync/atomic/doc.go. Those are the declarations used by escape analysis when using sync/atomic.

I'd still like to see a real-world case where this would be worth fixing.

@julieqiu julieqiu added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Apr 22, 2019
@julieqiu julieqiu added this to the Go1.13 milestone Apr 22, 2019
@rsc
Copy link
Contributor

rsc commented Apr 30, 2019

Duplicate of #16241.

@rsc rsc closed this as completed Apr 30, 2019
@golang golang locked and limited conversation to collaborators Apr 29, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Projects
None yet
Milestone
Go1.13
Development

No branches or pull requests
7 participants
@davecheney @rsc @cespare @agnivade @julieqiu @randall77 @gopherbot