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

sync/atomic: add typed atomic values #50860

Closed
rsc opened this issue Jan 27, 2022 · 25 comments
Closed

sync/atomic: add typed atomic values #50860

rsc opened this issue Jan 27, 2022 · 25 comments

Comments

@rsc
Copy link
Contributor

rsc commented Jan 27, 2022

In June 2021 I posted a series of articles about memory models, ending with an article about changes I thought we should make to the Go memory model. See https://research.swtch.com/mm especially https://research.swtch.com/gomm.

Then I opened a GitHub Discussion to discuss these changes; see #47141.

Based on that discussion, I propose to add the following types to sync/atomic: Bool, Int32, Int64, Uint32, Uint64, Uintptr, Pointer[T any].

These have methods appropriate to the type. They all have Load, Store, Swap, and CompareAndSwap methods. The integers also have an Add method (Bool and Pointer[T] do not).

The exact details can be viewed in pending CL 381317 prepared for concreteness.

I have filed a separate proposal, #50859, for documentation fixes arising from the June discussion.


Generics? A natural question is why the types are not something like atomic.Val[bool], atomic.Val[int32], and so on. The main answer is that the APIs are different for different types, and generics provides no way to accomplish that.

Specifically, Bool and Pointer[T] should not have an Add method, while the integers should.

Another reason is that there is no way to write a single constraint that works for this set of types. The way to write a generic pointer constraint, for atomic.Val[*byte], is to say [T ~*E, E any], but there is no way to add on the other types. If we do

type Val[T interface { ~*E | ~bool | ~int32 | ... }, E any] struct { ... }

then any use that infers bool, int32, or so on for T will not have any idea what to infer for E, requiring the programmer to write

atomic.Val[bool, DOESNOTMATTER]

where DOESNOTMATTER is any type at all. All in all, trying to use generics is pretty awkward here.

Uses are awkward too: atomic.Val[int32] is not as nice as plain atomic.Int32.

We could potentially introduce a single generic for the ints, as in atomic.Int[int32], but that removes awkwardness in the implementation at the cost of introducing awkwardness (repetition) at all the call sites. That's usually the wrong tradeoff, including here.

(Finally there is the matter of what the implementation body would look like in the generic functions, but all the preceding concerns prevent us from even reaching that one.)

The non-generic API is simpler to explain and easier to use.

@rsc rsc added the Proposal label Jan 27, 2022
@gopherbot gopherbot added this to the Proposal milestone Jan 27, 2022
@gopherbot
Copy link

Change https://golang.org/cl/381317 mentions this issue: sync/atomic: add typed atomic values

@rogpeppe
Copy link
Contributor

[Sorry, I didn't notice the "DO NOT REVIEW" in the CL and added this comment there where it wasn't possible to remove]

Absolutely. Pointer in particular makes life so much easier.

I wonder if it might be worth declaring a generic interface that covers all of those new types:

type ValueOf[T any] interface {
	Load() T
	Store(T)
	Swap(T) T
	CompareAndSwap(T, T) bool
}

(I don't know what the preferred spelling for "Of" types is these days).

Unfortunately these types aren't automatically assignable to that interface (cf #41176) but I think the type is still useful (and makes it clear that they all conform to the same pattern too).

@rogpeppe
Copy link
Contributor

One other thought: under this proposal you have to use a different type for an value that's atomically accessed.
In general that's a Good Thing, but I can imagine scenarios where that's not possible (for example,
atomic access to elements of a slice which is later exposed in a public API). The original atomic operations
are still available, of course, but LoadPointer and StorePointer are sufficiently awkward (and error-prone) to
use that I wonder if it might be worth introducing generic helper functions for those. For example:

func LoadPointerOf[T any](addr **T) *T
func StorePointerOf[T any](addr **T, val *T)
func CompareAndSwapPointerOf[T any](addr **T, old, new *T) (swapped bool)

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) Jan 27, 2022
@rsc
Copy link
Contributor Author

rsc commented Jan 28, 2022

Re LoadPointerOf, etc, I'd rather take the conservative route and wait until there is a significant, demonstrated need. I think the different type will be correct for the vast majority of uses, and part of the goal of the new API is to get people to think of each value as either always atomic or never atomic. That's not strictly required, but it helps avoid bugs, so I don't want to make it easier to backslide on that.

@bcmills
Copy link
Contributor

bcmills commented Jan 28, 2022

To me, one of the advantages of this proposal is that by defining the atomic types independently it sidesteps the potential alignment issues (#19057, #27577) surrounding atomic access of non-atomic types. I think that's a strong benefit, and would rather not give it up.

However, I do think it would be reasonable to define when unsafe.Pointer may be used to convert a pointer to an atomic type to or from a suitably-aligned non-atomic type.

@zephyrtronium
Copy link
Contributor

One other thought: under this proposal you have to use a different type for an value that's atomically accessed. In general that's a Good Thing, but I can imagine scenarios where that's not possible (for example, atomic access to elements of a slice which is later exposed in a public API). The original atomic operations are still available, of course, but LoadPointer and StorePointer are sufficiently awkward (and error-prone) to use that I wonder if it might be worth introducing generic helper functions for those.

I assume this code is valid:

func CompareAndSwapPointerOf[T any](addr **T, old, new *T) (swapped bool) {
	return atomic.CompareAndSwapPointer(
		(*unsafe.Pointer)(unsafe.Pointer(addr)),
		unsafe.Pointer(old),
		unsafe.Pointer(new),
	)
}

I.e., I assume that there is no difference in representation between a pointer to a concrete type and a pointer to a type parameter type. Then, the implementations of those functions are straightforward, so it is easy for packages to define them when they need them.

@prattmic
Copy link
Member

For internal implementation reasons, the runtime itself uses a separate atomic package from sync/atomic. A few months ago we added types similar to this proposal in runtime/internal/atomic (biggest difference is the lack of a generic atomic.Pointer).

To provide some concrete examples of use you can take a look at the relation chain on https://go.dev/cl/356169.

My personal experience with these thus far has been very positive. Having atomic types makes it difficult/impossible to forget to use an atomic access, which was very easy before. Additionally, at least in the runtime nearly all of our atomic values are basic integral types, so lack of generics doesn't feel too limiting and in fact makes them simpler to type.

@mknyszek
Copy link
Contributor

+1 to resolving atomic alignment issues here, and IMO it should happen sooner rather than later. I don't think we should wait for a general alignment-fixing proposal to land (like #19057) before fixing it, because it means users can rewrite their atomic code to actually work straight out of the gate, and I think it's going to take substantially more time to land any kind of general fix to the alignment issues.

Also, +1 to a generic Pointer[T].

@rsc
Copy link
Contributor Author

rsc commented Feb 2, 2022

Complete agreement about arranging, through some kind of magic, that atomic.Int64/atomic.Uint64 carry proper alignment.

@rsc
Copy link
Contributor Author

rsc commented Feb 2, 2022

@bcmills

However, I do think it would be reasonable to define when unsafe.Pointer may be used to convert a pointer to an atomic type to or from a suitably-aligned non-atomic type.

I assume you mean things like converting a *int32 to a *atomic.Int32. I am not sure I really want to guarantee any conditions about safety of that conversion, even though I did use it in the test. (Tests are allowed to assume things about their own package's internals!)

Saying anything about that conversion seems like taking on a constraint for little benefit, and it points people in a generally bad direction we'd rather they didn't go. People who need mixed atomic and non-atomic access can still use the old API, which is not going away.

@rsc rsc moved this from Incoming to Active in Proposals (old) Feb 2, 2022
@rsc
Copy link
Contributor Author

rsc commented Feb 2, 2022

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@nightlyone
Copy link
Contributor

@rsc please note that there has been prior work on this also suggesting such an API before https://pkg.go.dev/github.com/nightlyone/atomic

@rsc
Copy link
Contributor Author

rsc commented Feb 16, 2022

Does anyone object to accepting this proposal?

@rsc rsc moved this from Active to Likely Accept in Proposals (old) Feb 23, 2022
@rsc
Copy link
Contributor Author

rsc commented Feb 23, 2022

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

@rsc
Copy link
Contributor Author

rsc commented Mar 2, 2022

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

@rsc rsc changed the title proposal: sync/atomic: add typed atomic values sync/atomic: add typed atomic values Mar 2, 2022
@rsc rsc modified the milestones: Proposal, Backlog Mar 2, 2022
@flowchartsman
Copy link

This is very similar to https://github.com/uber-go/atomic. One thing they did with that library was providing support for JSON marshal/unmarshal, by calling Load() and Store() respectively:

// MarshalJSON encodes the wrapped bool into JSON.
func (x *Bool) MarshalJSON() ([]byte, error) {
	return json.Marshal(x.Load())
}

// UnmarshalJSON decodes a bool from JSON.
func (x *Bool) UnmarshalJSON(b []byte) error {
	var v bool
	if err := json.Unmarshal(b, &v); err != nil {
		return err
	}
	x.Store(v)
	return nil
}

Is there any problem with doing that here?

@bcmills
Copy link
Contributor

bcmills commented Mar 4, 2022

@flowchartsman, the sync/atomic package is very low-level and cannot import encoding/json in order to call json.Unmarshal.

@flowchartsman
Copy link

Ah, good point. I suppose a simple wrapper will be enough to get the best of both worlds when needed.

@gopherbot
Copy link

Change https://go.dev/cl/410127 mentions this issue: sync/atomic: note that alignment responsibility doesn't apply to types

@gopherbot
Copy link

Change https://go.dev/cl/410131 mentions this issue: cmd/compile/internal/escape: escape values with >PtrSize alignment

gopherbot pushed a commit that referenced this issue Jun 3, 2022
For #50860.

Change-Id: I8e117f00c5da230d0dc398aaed417fe5e64a5b22
Reviewed-on: https://go-review.googlesource.com/c/go/+/410127
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Michael Pratt <mpratt@google.com>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
gopherbot pushed a commit that referenced this issue Jun 3, 2022
After CL 381317 there exist values that may have an alignment greater
than the pointer size for that platform. Specifically, atomic.{Ui|I}nt64
may be aligned to 8 bytes on a 32-bit platform. If such a value, or
a container for the value, gets stack-allocated, it's possible that it
won't be aligned correctly, because the maximum alignment we enforce on
stacks is governed by the pointer size. Changing that would be a
significant undertaking, so just escape these values to the heap
instead, where we're sure they'll actually be aligned correctly.

Change is by rsc@, I'm just shepherding it through code review.

For #50860.

Change-Id: I51669561c0a13ecb84f821020e144c58cb528418
Reviewed-on: https://go-review.googlesource.com/c/go/+/410131
Run-TryBot: Michael Knyszek <mknyszek@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
@abhinav
Copy link
Contributor

abhinav commented Jun 10, 2022

@flowchartsman, the sync/atomic package is very low-level and cannot import encoding/json in order to call json.Unmarshal.

One of the reasons for JSON support on the objects is that it's easy for someone to attempt to serialize a struct that has an atomic field and get meaningless output (likely {}). If this is something we want to solve, a couple other options are:

  1. implement MarshalJSON, UnmarshalJSON manually with strconv.FormatInt and strconv.ParseInt
  2. special-case the atomic types in the encoding/json package

I lean (2).

(I can make a separate proposal for this if you think this warrants a longer discussion.)

@EdSchouten
Copy link

EdSchouten commented Jul 27, 2022

Hey @rsc! The new atomic types are pretty sweet. Thanks for adding those.

Q: Are there any plans to add a method similar to C/C++'s atomic_init()? I think you can currently only initialize them by calling .Store() on them. Even though it works, it does add an unnecessary atomic operation.

@gopherbot
Copy link

Change https://go.dev/cl/428362 mentions this issue: all: make sure *Pointer[T]'s methods are intended inlining

gopherbot pushed a commit that referenced this issue Sep 6, 2022
Updates #50860

Change-Id: I65bced707e50364b16edf4b087c541cf19bb1778
Reviewed-on: https://go-review.googlesource.com/c/go/+/428362
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Bryan Mills <bcmills@google.com>
Auto-Submit: Cuong Manh Le <cuong.manhle.vn@gmail.com>
@dmitshur dmitshur modified the milestones: Backlog, Go1.19 Sep 6, 2022
@lmb
Copy link
Contributor

lmb commented Feb 23, 2023

Change https://go.dev/cl/410127 mentions this issue: sync/atomic: note that alignment responsibility doesn't apply to types

Is atomic.Pointer also guaranteed to be aligned correctly?

@bcmills
Copy link
Contributor

bcmills commented Feb 23, 2023

Yes.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Development

No branches or pull requests