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

proposal: atomic: add (*Value).Swap #20164

Closed
bcmills opened this issue Apr 28, 2017 · 18 comments
Closed

proposal: atomic: add (*Value).Swap #20164

bcmills opened this issue Apr 28, 2017 · 18 comments

Comments

@bcmills
Copy link
Member

@bcmills bcmills commented Apr 28, 2017

atomic.Value seems to be the preferred replacement for the atomic.*Pointer methods in code that avoids package unsafe. Unfortunately, atomic.Value doesn't support swaps, making it much less powerful than the equivalent Pointer methods.

When investigating sync.RWMutex usage in the standard library (#17973), I discovered an RWMutex in crypto/tls guarding two fields, sessionTicketKeys and originalConfig, that are always updated independently. It's trivial to replace sessionTicketKeys with an atomic.Value, but originalConfig needs an atomic swap.

More generally, it would be nice if atomic.Value were as complete a replacement as possible for unsafe.Pointer with atomic operations.


Adding CompareAndSwap was discussed previously (#11260).
The major arguments against at the time seem to have been:

  1. Inapplicability for incomparable types. (#11260 (comment))
  2. Lack of concrete use-case. (#11260 (comment))
  3. Availability of unsafe.Pointer as an alternative. (#11260 (comment))

To address those points directly:

  1. Swap, unlike CompareAndSwap, does not require comparability. (Personally I think it would be good to add CompareAndSwap too and simply panic for uncomparable types, but as I don't have a use-case for that I would prefer to keep it out-of-scope for this proposal.)
  2. I have identified a concrete use-case in the crypto/tls package.
  3. According to @bradfitz in CL 41930, unsafe.Pointer is strongly discouraged outside of the sync, runtime, and reflect packages.

(@dvyukov, @josharian, @OneOfOne, @cespare, @adg, @rsc)

@gopherbot gopherbot added this to the Proposal milestone Apr 28, 2017
@gopherbot gopherbot added the Proposal label Apr 28, 2017
@rsc
Copy link
Contributor

@rsc rsc commented Apr 28, 2017

I'm very skeptical that code in crypto/tls is even correct. What is going on there?

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Apr 28, 2017

The only time the field is set is immediately before calling newConfig.serverInitOnce.Do(newConfig.serverInit), so I think this code could be simplified by simply passing the originalConfig value as an argument to serverInit. As far as I can see originalConfig is being used to pass an argument through sync.Once.Do, but that is more easily done using a closure.

The field was added in https://golang.org/cl/30790. CC @agl.

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 28, 2017

CL https://golang.org/cl/42137 mentions this issue.

@agl
Copy link
Contributor

@agl agl commented Apr 28, 2017

Yes, it's passing an argument to the once function. https://go-review.googlesource.com/c/42137/ does the same thing with closures if that's useful.

@rsc
Copy link
Contributor

@rsc rsc commented May 15, 2017

Thanks @agl.

Given that CL 42137 eliminates the motivation for atomic.Swap, @bcmills do you have any other motivation for it?

@bcmills
Copy link
Member Author

@bcmills bcmills commented May 15, 2017

Not at the moment. If you'd like to shelve this until we find another use-case, that's fine with me.

gopherbot pushed a commit that referenced this issue May 16, 2017
…Config.

Updates #20164.

Change-Id: Ib900095e7885f25cd779750674a712c770603ca8
Reviewed-on: https://go-review.googlesource.com/42137
Reviewed-by: Russ Cox <rsc@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@rsc rsc closed this Jun 12, 2017
@mlitvin
Copy link

@mlitvin mlitvin commented Feb 15, 2018

My two cents

I had today a semi-real use case.

It was real because I was writing a code where I actually went and looked for Value.Swap because I wanted to use it and was a bit surprised not to find it.

However the fact that it is not there didn't blocked me, I my specific use case using simple locks instead of atomic value would most probably be doable without significant performance degradation of the whole system, and if it was critical I would probably be able to either use unsafe.Pointer, or fork atomic.Value.

The problem is that any of the solutions (including using locks) seems to be on the same order of complication as implementing Value.Swap.

So why not do it? The difference between Value.Set and Value.Swap is probably around 5 lines of code, its not a big burden for the standard library.

@dvyukov
Copy link
Member

@dvyukov dvyukov commented Feb 15, 2018

@mlitvin How do you use mutex? From your text it seems that you use mutex on both write side and read side. Value is meant for read-mostly scenarios. In such case, if you need swap, you put mutex only around write side and do Load+Store under the mutex, which effectively gives you Swap without any performance degradation (since it's read-mostly scenario), and code complexity is almost equivalent to Swap since this is all local to write side. This also saves you from potential atomicity violations between multiple writers (which are presumably present, since if there is only 1 writer, then Load+Store is equivalent to Swap).

@mlitvin
Copy link

@mlitvin mlitvin commented Feb 15, 2018

@dvyukov, putting mutex only around the write side is a good trick (that I haven't thought of) and I may use it. Thanks!

That lead to a different suggestion: assume that we don't think that that the potential uses of Value.Swap justify duplicating or complicating Value.Set. We could add an implementation of Swap based on write side lock, and see if it has users. And based on that we can decide if it deserve to be optimized or not.

@dvyukov
Copy link
Member

@dvyukov dvyukov commented Feb 15, 2018

And what if it does not have users? Or, if it has users, but they use it wrongly?
This is not so about lines of code or optimization, this is much more about interfaces. If we start putting stuff into standard library public interface just to test some hypothesizes, it will grow without a limit with multiple ways of doing the same thing and with lots of ways which provoke bugs.
This should be done the other way around -- show dozens of existing packages in the wild that all duplicate this Swap pattern (which is perfectly implementable without standard library help), then we can consider including it into standard library.

@mlitvin
Copy link

@mlitvin mlitvin commented Feb 16, 2018

@dvyukov
Copy link
Member

@dvyukov dvyukov commented Feb 16, 2018

Well, at the very least we need to see a range of real cases where Value.Swap would be a good fit. We tried, but we failed. I also tried to do a mental experiment and just image some of such cases, but also failed to find any.
Staffing things into any library just in case when we don't see nor can't imagine any uses for it is wrong.

Argument that can won here is very simple: show how badly it is needed in real programs.

Re 2: Swap will not guide developers to write better code, it will guide developers to write complex code with bugs. If we go this route later we will also need CompareAndSwap which will provoke even more complex and more subtly broken code. Mutex gives both simple code and is complete (allows to do Swap, CompareAndSwap and everything else).

If the Mutex on write side trick is useful but non-trivial, we should write better docs and provide examples for this.

@mlitvin
Copy link

@mlitvin mlitvin commented Feb 16, 2018

@dvyukov
Copy link
Member

@dvyukov dvyukov commented Feb 16, 2018

How do producers create new state?

@mlitvin
Copy link

@mlitvin mlitvin commented Feb 16, 2018

@dvyukov
Copy link
Member

@dvyukov dvyukov commented Feb 16, 2018

Yes, if there are multiple producers then they generally must not lose updates from each other, which implies some synchronization between them. And the base state a producer used to create the new state is the old state which then needs to be cancelled.

The question still holds: is it actually used in real life anywhere? how widespread is this?
Note: this is a very small, tricky and frequently bogus subset of an already tricky and small subset of synchronization patterns. And it is already almost perfectly handled by Mutex+Load+Store. It's not possible nor reasonable to support all possible cases with a special function provided for that particular case. That's why we have a general-purpose language that allows one to build things from other things.

@mlitvin
Copy link

@mlitvin mlitvin commented Feb 16, 2018

@dvyukov
Copy link
Member

@dvyukov dvyukov commented Feb 16, 2018

As I guess those kind of arguments cannot be won.

No, these arguments can easily be won for something like fmt.Printf and json.Decode, It happened for majority of std lib functionality. You can also find very recent examples if you look at release notes. This is really about this particular case. I don't see logic behind generalizing "SendPacketAndAdjustMyChairIfNotWednesday should not be added to std lib" to "nothing should be added to std lib".

I tried to look at uses of atomic.SwapUintptr and found about one and a half.

Whatever it was, it's there now. And it does not justify anything in this case, right? I am more than sure we would like to adjust some things in hindsight. But that's the point: adding things is easy, removing -- impossible.

FiloSottile pushed a commit to FiloSottile/go that referenced this issue Oct 12, 2018
…Config.

Updates golang#20164.

Change-Id: Ib900095e7885f25cd779750674a712c770603ca8
Reviewed-on: https://go-review.googlesource.com/42137
Reviewed-by: Russ Cox <rsc@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
FiloSottile pushed a commit to FiloSottile/go that referenced this issue Oct 12, 2018
…Config.

Updates golang#20164.

Change-Id: Ib900095e7885f25cd779750674a712c770603ca8
Reviewed-on: https://go-review.googlesource.com/42137
Reviewed-by: Russ Cox <rsc@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@golang golang locked and limited conversation to collaborators Feb 16, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
7 participants
You can’t perform that action at this time.