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 (*Value).Swap and (*Value).CompareAndSwap #39351

Open
carnott-snap opened this issue Jun 1, 2020 · 32 comments
Open

sync/atomic: add (*Value).Swap and (*Value).CompareAndSwap #39351

carnott-snap opened this issue Jun 1, 2020 · 32 comments

Comments

@carnott-snap
Copy link

@carnott-snap carnott-snap commented Jun 1, 2020

Between a recent use case, and previous tickets (#11260, #20164, #26728), there is renewed interest in implementing Swap and CompareAndSwap for sync/atomic.Value.

The new interface serves to be more ergonomic, eschew the use of unsafe, and make atomic.Value have parity with the atomic.XxxPointer methods. There are some key differences between how the atomic.Value.XxxSwap and atomic.XxxSwapPointer work, e.g. panic on inconsistent types or nil, but these are fundamental to how atomic.Value works, and thus an accepted complexity.

Interface definition follows:

package atomic

// Swap stores new into Value and returns the previous value. It returns nil if
// the Value is empty.
//
// All calls to Swap for a given Value must use values of the same concrete
// type. Swap of an inconsistent type panics, as does Swap(nil).
func (v *Value) Swap(new interface{}) (old interface{})

// CompareAndSwapPointer executes the compare-and-swap operation for the Value.
//
// All calls to CompareAndSwap for a given Value must use values of the same
// concrete type. CompareAndSwap of an inconsistent type panics, as does
// CompareAndSwap(nil, old).
func (v *Value) CompareAndSwap(new, old interface{}) (swapped bool)
@gopherbot gopherbot added this to the Proposal milestone Jun 1, 2020
@gopherbot gopherbot added the Proposal label Jun 1, 2020
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jun 1, 2020

See also #11260, #20164 and #26728.

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals Jun 1, 2020
@carnott-snap
Copy link
Author

@carnott-snap carnott-snap commented Jun 1, 2020

See also #11260, #20164 and #26728.

Thanks for the pointers; I did look, but it appears searching is hard with the wrong keywords.

One thing that is not called out in the linked tickets is the warning on the unsafe package. My use case does work with atomic.SwapPointer, but I am unwilling to accept the risks presented by the unsafe package:

Packages that import unsafe may be non-portable and are not protected by the
Go 1 compatibility guidelines.
@oguzyildiz1991
Copy link

@oguzyildiz1991 oguzyildiz1991 commented Jun 2, 2020

In my case, I need atomic.Swap because I have a variable that collects batched statistics (many go routines write to it), and one go routine is responsible of replacing that batch with a new empty batch atomically and export the old one. I had to use atomic.SwapPointer to swap and atomic.LoadPointer to get it from the writers because Mutexes had too much lock contention.

@bcmills
Copy link
Member

@bcmills bcmills commented Jun 2, 2020

Note: we could also call this atomic.Value.Swap. It would be the same implementation, but with a name more closely aligns with atomic.SwapPointer.

I don't understand this comment. Swap is not at all the same operation as LoadOrStoreLoadOrStore is more like a special case of CompareAndSwap, where the value to be compared is “no value stored”.

@bcmills
Copy link
Member

@bcmills bcmills commented Jun 2, 2020

I do think there might be some value in a (*atomic.Value).LoadOrStore, though — in some cases, I think that could help users to avoid panics resulting from the “same concrete type” restriction, particularly if the value being stored is some concrete implementation of the error interface.

@bcmills
Copy link
Member

@bcmills bcmills commented Jun 2, 2020

One thing that is not called out in the linked tickets is the warning on the unsafe package.

Note that #20164 specifically calls out avoidance of unsafe as one of the main motivating factors.

Also note that both #20164 and #26728 were withdrawn for lack of compelling concrete use-cases, so it would be very helpful to see some concrete examples of where the proposed method would be used.

@carnott-snap
Copy link
Author

@carnott-snap carnott-snap commented Jun 2, 2020

I don't understand this comment. Swap is not at all the same operation as LoadOrStoreLoadOrStore is more like a special case of CompareAndSwap, where the value to be compared is “no value stored”.

That is fair and I may have gotten carried away with being compatible to sync.Map and company. I am open to either LoadOrStore, Swap, or CompareAndSwap, as they all meet my needs.

Note that #20164 specifically calls out avoidance of unsafe as one of the main motivating factors.

atomic.Value seems to be the preferred replacement for the atomic.*Pointer methods in code that avoids package unsafe.

According to @bradfitz in CL 41930, unsafe.Pointer is strongly discouraged outside of the sync, runtime, and reflect packages.

While the above is called out, it felt like a preference and did not mention the non-portable and compatibility implications. My apologies if this was obvious subtext in #20164.

Also note that both #20164 and #26728 were withdrawn for lack of compelling concrete use-cases, so it would be very helpful to see some concrete examples of where the proposed method would be used.

This was submitted on behalf of @oguzyildiz1991's use case. Let me know if more detail would be beneficial. Was the resolution of other tickets that unsafe.Pointer was good enough?

@rsc
Copy link
Contributor

@rsc rsc commented Jun 3, 2020

The resolution on #26728 said:

There aren't many environments that reject unsafe anymore, and we shouldn't contort the library for those.

It would be fine for someone who has a compelling use for these to create a new issue proposing to add Swap and CompareAndSwap (or to reopen this one, if you have permission). For now, given the lack of a real-world use case for them, we will err on the side of leaving it out.

Is there a new real-world use case where Value.Swap is helpful?

@oguzyildiz1991
Copy link

@oguzyildiz1991 oguzyildiz1991 commented Jun 3, 2020

how about the one I described above? @rsc

@carnott-snap
Copy link
Author

@carnott-snap carnott-snap commented Jun 3, 2020

There aren't many environments that reject unsafe anymore, and we shouldn't contort the library for those.

My concern is less about the inability to use unsafe, and more my unwillingness. If I import "unsafe", I loose the Go 1 compatibility guarantee. I think it is untenable to say that you need to do that just to use the sync/atomic package.

@rsc rsc moved this from Incoming to Active in Proposals Jun 3, 2020
@bcmills
Copy link
Member

@bcmills bcmills commented Jun 3, 2020

@carnott-snap, if you're worried about formal compatibility policies in conjunction with sync/atomic, you might also take a look at #5045. (sync/atomic itself doesn't have well-described behavior yet, although we have a fairly clear picture of what the definition of that behavior should be.)

@carnott-snap
Copy link
Author

@carnott-snap carnott-snap commented Jun 4, 2020

While I am now also concerned about the exact details of the sync/atomic package, it is still covered under the Go 1 compatibility guarantee. It seems like sync/atomic's use of unsafe.Pointer is stable, and could be covered by the guarantee, but since the Pointer symbol is in the unsafe package, you take a risk.

To be clear, I am arguing that using atomic.CompareAndSwapPointer (or LoadPointer, StorePointer, SwapPointer) is unsafe, thus the argument made in previous tickets, that atomic.Value.Swap is not required, is untenable. There exist other solutions, like exposing a safe atomic.Pointer or accepting interface{}, but I think atomic.Value.Something is the most straightforward.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jun 4, 2020

@carnott-snap I want to clarify that nothing is going to go wrong if you convert in and out of unsafe.Pointer. Go is not a language that tolerates the tools trying to trick users. Maybe importing unsafe is risky in some formal sense, but in reality it is not.

@carnott-snap
Copy link
Author

@carnott-snap carnott-snap commented Jun 4, 2020

That is not what the package documentation states. If it should be updated, that is fine, but it is stated in no uncertain terms:

Package unsafe contains operations that step around the type safety of Go programs. 

Packages that import unsafe may be non-portable and are not protected by the Go 1 compatibility guidelines. 
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jun 4, 2020

I agree that unsafe is risky in a formal sense. I even tried to say that.

I am saying that in practice nothing is going to go wrong if you convert in and out of unsafe.Pointer. I promise. But it's hard to write down that kind of rule formally, and, frankly, it's not worth the time. There is more to a language than formalism; there is also practice.

If you can only accept a formal policy, then you will presumably continue to avoid unsafe. But I"m not sure that that in itself is a strong enough reason for us to add a new method to atomic.Value. Not that I'm especially opposed to a new method, but in the past we've struggled to see a reason for one.

@carnott-snap
Copy link
Author

@carnott-snap carnott-snap commented Jun 4, 2020

Thanks for the full context. As this was lost on me, I still think there is room to improve the package documentation wording to indicate this, but I respect that the flavour may be lost on other readers. Is it worth trying to improve the wording, or does everybody agree this is the best message to send to potential unsafe consumers?

@OneOfOne
Copy link
Contributor

@OneOfOne OneOfOne commented Jun 4, 2020

@ianlancetaylor while that is true, it limits people in way, for example I had to completely move from App Engine for multiple projects because they refuse any app with unsafe (or syscall) outside the stdlib.

Adding LoadOrStore or CompareAndSwap to Value would make life a lot easier

Also having it in the stdlib would allow compiler intrinsic later on to make it more optimized and supported on the platforms that doesn't allow unsafe.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jun 4, 2020

My understanding is that App Engine no longer has that constraint.

@OneOfOne
Copy link
Contributor

@OneOfOne OneOfOne commented Jun 4, 2020

I haven't tried in a few years, but at least it's in their official FAQ I linked above.

@carnott-snap
Copy link
Author

@carnott-snap carnott-snap commented Jun 4, 2020

I can confirm that documentation is woefully out of date: unsafe, syscall, cgo, and assembly all work with >=1.11. I also know direct network access works, and think writing to disk should too.

@rsc
Copy link
Contributor

@rsc rsc commented Jun 10, 2020

I don't fully understand how all the writers are coordinating in #39351 (comment), but looking at the sync/atomic API, it does seem odd that we have Load, Store, Swap, and CompareAndSwap for all the integer types and unsafe.Pointer, but then we only have Load and Store for Value.

I'm trying to remember whether there was an implementation reason for leaving them out. With the dynamic constraint that the underlying concrete type of the stored interface can't change, it should just be a plain single-word swap/cas. Unless there's some implementation subtlety I'm missing, it sounds like maybe we should add them.

Thoughts?

@randall77
Copy link
Contributor

@randall77 randall77 commented Jun 10, 2020

The API is a bit cumbersome. In order to do Swap/CompareAndSwap, you have to allocate a atomic.Value, then Store a value there (of the correct type), and only then can you Swap/CompareAndSwap a new value into the Value.

  • What happens if you CompareAndSwap something of different dynamic type? (panic, presumably?)
  • What happens if you CompareAndSwap with the new value being nil?
  • How do we do the compare of two Values both containing the integer 2, but which were separately allocated (so the interface data pointer is different)? Do we need to call the type's equal function?
@OneOfOne
Copy link
Contributor

@OneOfOne OneOfOne commented Jun 10, 2020

@randall77

What happens if you CompareAndSwap something of different dynamic type? (panic, presumably?)

  • I'd assume the same thing as Store, panic if the types don't match.

What happens if you CompareAndSwap with the new value being nil?

  • Panic if the type isn't a pointer, otherwise work as expected, also Store will have to be modified since it panics or just follow the Store impl and allow typed nil. (https://play.golang.org/p/vZwyErdDJc9)

How do we do the compare of two Values both containing the integer 2, but which were separately allocated (so the interface data pointer is different)? Do we need to call the type's equal function?

  • Since we can't store mismatched types it should be (maybe?) trivial to call the type's equal fn.
@rsc
Copy link
Contributor

@rsc rsc commented Jun 17, 2020

@randall77, I was expecting

func (v *Value) Swap(new interface{}) interface{}

and

func (v *Value) CompareAndSwap(old, new interface{}) (swapped bool)

preserving the constraints that Value.Store itself has (underlying type can't change, new cannot be nil).

If we preserve the constraints of Value.Store, is there any implementation difficulty with Swap and CompareAndSwap?

@randall77
Copy link
Contributor

@randall77 randall77 commented Jun 17, 2020

I think they are doable. Swap is straightforward using atomic.SwapPointer.

CompareAndSwap is quite a bit more complicated.

func (v *Value) CompareAndSwap(old, new interface{}) (swapped bool) {
    if old.Type != new.Type { panic }
    if old.Type != v.Type { panic }
    p := v.Data
    if t.direct {
        if p != old.Data { return false }
    } else {
        if !old.Type.Equal(p, old.Data) { return false } // using type's equality function
    }
    return CompareAndSwapPointer(&v.Data, p, new.Data)
}
@carnott-snap
Copy link
Author

@carnott-snap carnott-snap commented Jun 17, 2020

@randall77: I am onboard with this modification to my initial proposal and will fixup the description to match.

@carnott-snap carnott-snap changed the title proposal: sync/atomic: add Value.LoadOrStore proposal: sync/atomic: add (*Value).Swap and (*Value).CompareAndSwap Jun 17, 2020
@rsc
Copy link
Contributor

@rsc rsc commented Jun 24, 2020

Based on the discussion above, this seems like a likely accept.

@rsc rsc moved this from Active to Likely Accept in Proposals Jun 24, 2020
@rsc
Copy link
Contributor

@rsc rsc commented Jul 8, 2020

No change in consensus, so accepted.

@rsc rsc moved this from Likely Accept to Accepted in Proposals Jul 8, 2020
@rsc rsc modified the milestones: Proposal, Backlog Jul 8, 2020
@rsc rsc changed the title proposal: sync/atomic: add (*Value).Swap and (*Value).CompareAndSwap sync/atomic: add (*Value).Swap and (*Value).CompareAndSwap Jul 8, 2020
@rogpeppe
Copy link
Contributor

@rogpeppe rogpeppe commented Jul 9, 2020

I thought it might be worth mentioning that if/when generics come along, it's easy to make the atomic.*Pointer functions type-safe, although they'd need new names:

For example:

func SafeCompareAndSwapPointer(type T)(addr **T, old, new *T) (swapped bool) {
	return CompareAndSwapPointer(
		(*unsafe.Pointer)(unsafe.Pointer(addr)),
		unsafe.Pointer(old),
		unsafe.Pointer(new),
	)
}
@gopherbot
Copy link

@gopherbot gopherbot commented Jul 9, 2020

Change https://golang.org/cl/241678 mentions this issue: sync/atomic: add (*Value).Swap and (*Value).CompareAndSwap

@litao-2071
Copy link

@litao-2071 litao-2071 commented Jan 26, 2021

I think they are doable. Swap is straightforward using atomic.SwapPointer.

CompareAndSwap is quite a bit more complicated.

func (v *Value) CompareAndSwap(old, new interface{}) (swapped bool) {
    if old.Type != new.Type { panic }
    if old.Type != v.Type { panic }
    p := v.Data
    if t.direct {
        if p != old.Data { return false }
    } else {
        if !old.Type.Equal(p, old.Data) { return false } // using type's equality function
    }
    return CompareAndSwapPointer(&v.Data, p, new.Data)
}

if type of swap value is different, panic happened in CompareAndSwapPointer function, I don't think we need to deal with it again.

@randall77
Copy link
Contributor

@randall77 randall77 commented Jan 26, 2021

@litao-2071 I don't understand what you're trying to say. CompareAndSwapPointer doesn't know the types that are being swapped. And it never panics (unless maybe if v is nil?).
You might want to look at the CL referenced above. It contains actual code instead of my pseudocode, which might clear things up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Proposals
Accepted
Linked pull requests

Successfully merging a pull request may close this issue.

None yet