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 · 18 comments
Closed

sync/atomic: add typed atomic values #50860

rsc opened this issue Jan 27, 2022 · 18 comments

Comments

@rsc
Copy link
Contributor

@rsc 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.

@gopherbot
Copy link

@gopherbot gopherbot commented Jan 27, 2022

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

@rogpeppe
Copy link
Contributor

@rogpeppe rogpeppe commented Jan 27, 2022

[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

@rogpeppe rogpeppe commented Jan 27, 2022

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 Jan 27, 2022
@rsc
Copy link
Contributor Author

@rsc 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
Member

@bcmills 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

@zephyrtronium zephyrtronium commented Jan 28, 2022

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

@prattmic prattmic commented Jan 28, 2022

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

@mknyszek mknyszek commented Jan 28, 2022

+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 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 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 Feb 2, 2022
@rsc
Copy link
Contributor Author

@rsc 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

@nightlyone nightlyone commented Feb 9, 2022

@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 rsc commented Feb 16, 2022

Does anyone object to accepting this proposal?

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

@rsc 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 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 removed this from the Proposal milestone Mar 2, 2022
@rsc rsc added this to the Backlog milestone Mar 2, 2022
@flowchartsman
Copy link

@flowchartsman flowchartsman commented Mar 4, 2022

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
Member

@bcmills 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

@flowchartsman flowchartsman commented Mar 4, 2022

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Proposals
Accepted
Development

No branches or pull requests

9 participants