Go: sync: Once uses, but doesn't need atomic operations

[JelteF]

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

1.9

Does this issue reproduce with the latest release?

Yes, code is here: https://github.com/golang/go/blob/master/src/sync/once.go#L35-L46

What did you expect to see?

To see synchronised access to o.done, but not excessively synchronising.

What did you see instead?

It's synchronised using atomic load and store and using a mutex.

Proposed fix

As far as I can tell there is no reason why load and store to o.done have to be atomic. The write is already synchronised using the mutex and before running f a second time the value of o.done is checked inside the mutex. So my fix would simply be to use non atomic operations on o.done.

I don't mind creating a PR myself, but would like to be sure that I'm not overlooking something before doing so.

[davecheney]

Please review this change which added the fast path atomic operation

https://github.com/golang/go/commit/93dde6b0e6c0de50a47f9dc5f3ac7205c36742aa

On Mon, Oct 2, 2017 at 11:02 PM, Jelte Fennema [e-mail]
wrote:

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

1.9
Does this issue reproduce with the latest release?

Yes, code is here: https://github.com/golang/go/
blob/master/src/sync/once.go#L35-L46
What did you expect to see?

To see synchronised access to o.done, but not excessively synchronising.
What did you see instead?

It's synchronised using atomic load and store and using a mutex.
Proposed fix

As far as I can tell there is no reason why load and store to o.done have
to be atomic. The write is already synchronised using the mutex and before
running f a second time the value of o.done is checked inside the mutex.
So my fix would simply be to use non atomic operations on o.done.

I don't mind creating a PR myself, but would like to be sure that I'm not
overlooking something before doing so.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
https://github.com/golang/go/issues/22104, or mute the thread
https://github.com/notifications/unsubscribe-auth/AAAcAyEIRWO8v3XTjvNrtQbZnHNZgq7eks5soNDegaJpZM4Pqitb
.

[JelteF]

I was super confused at first that the exact oposite change would have an advantage. But after looking more closely it adds both the fast path and the atomic operation in the same PR. What I'm suggesting is doing the fast path change without the atomic operation, just do it non atomic (maybe with a boolean like it was before).

[JelteF]

Related question, are there docs somewhere on how to run these benchmarks myself?

[davecheney]

@JelteF

What I'm suggesting is doing the fast path change without the atomic operation, just do it non atomic (maybe with a boolean like it was before).

that would potentially introduce a data race

Related question, are there docs somewhere on how to run these benchmarks myself?

These are standard Go testing benchmarks.

[randall77]

The atomic load is required. Without it, the second caller of sync.Once may not see all of the modifications done by the first call to sync.Once.

[skabbes]

@randall77 I believe in this case stale reads are OK since the slow path actually has to relock and recheck anyway. This is not to say that there isn't a race there, but I believe on modern platforms with support for hardware atomic read / write with 32bit memory this would be impossible.

@JelteF I think you'd need some sort of atomic.LoadUint32RelaxedMemoryOrdering() operation if you wanted this to be race free... on all platforms.

[skabbes]

Oh wow, and also need to have correct alignment for atomic operations.

I think the added complexity isn't worth it, it isn't the _go way_.

[randall77]

@skabbes: You're right that the atomic load is not required for the slow path. But the atomic load is required for the fast path, when it reads a 1 and immediately returns. There needs to be some synchronization so that the second+ callers of Once.Do can see all the writes that f() did in the first call to Once.Do.

[skabbes]

Completely didn't think about that as an API guarantee, but it definitely is. Kudos.

[davecheney]

Thank you for the discussion. I’m going to close this as there is no obvious change required.

If you wish to continue to discuss this, please see https://golang.org/wiki/Questions for good places to ask. Thanks.

[JelteF]

Thanks for the feedback everyone. I learnt something new about dataraces, namely that they are possible even when the read and assignment is guarded by a mutex. I checked out moving the atomic load to the slow instead of the fast path and the race detector was still complaining. Even with your comments it's still not clear to me why that would not be a valid solution.

[davecheney]

@JelteF let's take this off the issue tracker, please see https://golang.org/wiki/Questions for good places to ask. Thanks.