Issue 120140044: code review 120140044: sync/atomic: add Value (Closed)
Descriptionsync/atomic: add Value
A Value provides an atomic load and store of a consistently typed value.
It's intended to be used with copy-on-write idiom (see the example).
Performance:
BenchmarkValueRead 50000000 21.7 ns/op
BenchmarkValueRead-2 200000000 8.63 ns/op
BenchmarkValueRead-4 300000000 4.33 ns/op
Patch Set 1 #Patch Set 2 : diff -r 14c560b87afb https://dvyukov%40google.com@code.google.com/p/go/ #Patch Set 3 : diff -r 14c560b87afb https://dvyukov%40google.com@code.google.com/p/go/ #
Total comments: 2
Patch Set 4 : diff -r b6a42b0aeef6 https://dvyukov%40google.com@code.google.com/p/go/ #Patch Set 5 : diff -r b6a42b0aeef6 https://dvyukov%40google.com@code.google.com/p/go/ #Patch Set 6 : diff -r b6a42b0aeef6 https://dvyukov%40google.com@code.google.com/p/go/ #
Total comments: 1
Patch Set 7 : diff -r 163fae12c83b https://dvyukov%40google.com@code.google.com/p/go/ #
Total comments: 6
Patch Set 8 : diff -r 3f916fdee080 https://dvyukov%40google.com@code.google.com/p/go/ #Patch Set 9 : diff -r 3f916fdee080 https://dvyukov%40google.com@code.google.com/p/go/ #Patch Set 10 : diff -r a8843f90d292b6a04f7ab69a168971afb2ac46ef https://dvyukov%40google.com@code.google.com/p/go/ #Patch Set 11 : diff -r a8843f90d292b6a04f7ab69a168971afb2ac46ef https://dvyukov%40google.com@code.google.com/p/go/ #Patch Set 12 : diff -r a8843f90d292b6a04f7ab69a168971afb2ac46ef https://dvyukov%40google.com@code.google.com/p/go/ #Patch Set 13 : diff -r f7f98d34ccaaf832b775285629578264e7016b30 https://dvyukov%40google.com@code.google.com/p/go/ #Patch Set 14 : diff -r 179dc6045b9ed51299378f7a9fd3d6f9c9c80b8b https://dvyukov%40google.com@code.google.com/p/go/ #Patch Set 15 : diff -r 179dc6045b9ed51299378f7a9fd3d6f9c9c80b8b https://dvyukov%40google.com@code.google.com/p/go/ #
Total comments: 9
Patch Set 16 : diff -r e0bf1fc9dd8ed6029e45c9941d3f8dcce5ac9c8e https://dvyukov%40google.com@code.google.com/p/go/ #
Total comments: 4
Patch Set 17 : diff -r d290f48255fb933439916159ecd5301c5bb5c301 https://dvyukov%40google.com@code.google.com/p/go/ #Patch Set 18 : diff -r c157d3790300da127c5e9958e0565a70b22fb1fe https://dvyukov%40google.com@code.google.com/p/go/ #Patch Set 19 : diff -r 6138449ef60f03ebd2d46ffdeefa381b800dbedb https://dvyukov%40google.com@code.google.com/p/go/ #Patch Set 20 : diff -r 6138449ef60f03ebd2d46ffdeefa381b800dbedb https://dvyukov%40google.com@code.google.com/p/go/ #
MessagesTotal messages: 42
https://codereview.appspot.com/120140044/diff/40001/src/pkg/sync/atomic/value... File src/pkg/sync/atomic/value_test.go (right): https://codereview.appspot.com/120140044/diff/40001/src/pkg/sync/atomic/value... src/pkg/sync/atomic/value_test.go:51: if err == nil || err.(string) != nilErr { you can just say if err != nilErr
Sign in to reply to this message.
Why disallow Store(nil)?
Sign in to reply to this message.
On 2014/07/24 23:54:58, josharian wrote: > Why disallow Store(nil)? What would be a use case for this?
Sign in to reply to this message.
On 2014/07/24 23:54:58, josharian wrote: > Why disallow Store(nil)? What would be a use case for this?
Sign in to reply to this message.
https://codereview.appspot.com/120140044/diff/40001/src/pkg/sync/atomic/value... File src/pkg/sync/atomic/value_test.go (right): https://codereview.appspot.com/120140044/diff/40001/src/pkg/sync/atomic/value... src/pkg/sync/atomic/value_test.go:51: if err == nil || err.(string) != nilErr { On 2014/07/24 17:48:38, rsc wrote: > you can just say > > if err != nilErr > Done.
Sign in to reply to this message.
Added sketch of a second example for the component.
Sign in to reply to this message.
Added Value.Read benchmark: BenchmarkValueRead 50000000 21.7 ns/op BenchmarkValueRead-2 200000000 8.63 ns/op BenchmarkValueRead-4 300000000 4.33 ns/op Not bad, but could be better. Filed: https://code.google.com/p/go/issues/detail?id=8421 https://code.google.com/p/go/issues/detail?id=8422
Sign in to reply to this message.
https://codereview.appspot.com/120140044/diff/100001/src/pkg/sync/atomic/asm_... File src/pkg/sync/atomic/asm_amd64.s (right): https://codereview.appspot.com/120140044/diff/100001/src/pkg/sync/atomic/asm_... src/pkg/sync/atomic/asm_amd64.s:155: MOVQ 8(BP), CX // load word Can this be CMOV?
Sign in to reply to this message.
On 2014/07/25 20:05:15, ruiu wrote: > https://codereview.appspot.com/120140044/diff/100001/src/pkg/sync/atomic/asm_... > File src/pkg/sync/atomic/asm_amd64.s (right): > > https://codereview.appspot.com/120140044/diff/100001/src/pkg/sync/atomic/asm_... > src/pkg/sync/atomic/asm_amd64.s:155: MOVQ 8(BP), CX // load word > Can this be CMOV? I will check whether CMOVNZQ is supported by 6g. If not, them I would prefer to leave it as in the first version. These issues: https://code.google.com/p/go/issues/detail?id=8421 https://code.google.com/p/go/issues/detail?id=8422 results in much more significant loss.
Sign in to reply to this message.
> > Why disallow Store(nil)? > > What would be a use case for this? A polling goroutine updates a shared cache stored in an atomic.Value; http handlers use this cached data. In this scenario, if the polling goroutine fails, it is preferable to have no data than to have stale data. A natural way to tell the http handlers that the cached data is unavailable is to Store(nil), as opposed to having to create a sentinel value or add a pointless bool. This is not theoretical--we have a use case like this right now. We currently use a RWMutex to protect the cache and use nil to represent "no data available". I'd love to switch this to an atomic.Value.
Sign in to reply to this message.
On 2014/07/28 18:13:20, josharian wrote: > > > Why disallow Store(nil)? > > > > What would be a use case for this? > > A polling goroutine updates a shared cache stored in an atomic.Value; http > handlers use this cached data. In this scenario, if the polling goroutine fails, > it is preferable to have no data than to have stale data. A natural way to tell > the http handlers that the cached data is unavailable is to Store(nil), as > opposed to having to create a sentinel value or add a pointless bool. > > This is not theoretical--we have a use case like this right now. We currently > use a RWMutex to protect the cache and use nil to represent "no data available". > I'd love to switch this to an atomic.Value. I see. If we allow storing of nil, it will complicate implementation of type-stability and will require Value to be 3 words. Sentinel value does not look all that bad (it can include error if necessary as well). In either case I would like to hear from Rob before we start evolving this component. Just in case, you can do it today with atomic.Store/LoadPointer (if you are not afraid of unsafe).
Sign in to reply to this message.
> In either case I would like to hear from Rob before we start evolving this > component. SGTM. I won't push on this further—the implementation consequences are reason enough.
Sign in to reply to this message.
Can you send a separate CL showing how this would be used in encoding/gob and with benchmark results for the use? Thanks.
Sign in to reply to this message.
On 2014/08/05 04:34:06, rsc wrote: > Can you send a separate CL showing how this would be used in encoding/gob and > with benchmark results for the use? > > Thanks. Updated https://codereview.appspot.com/111230043 to use atomic.Value, benchmark results are updated as well.
Sign in to reply to this message.
Yesterday I checked in https://codereview.appspot.com/117520043, which hoisted a call to userType out of the struct-encoding closure, and the original complainant said it resolved the issue for him. It didn't remove the lock in all cases, though, so a faster lock can still help, but the urgency may be gone. Are you sync'ed past that CL? -rob On Tue, Aug 5, 2014 at 4:37 AM, <dvyukov@google.com> wrote: > On 2014/08/05 04:34:06, rsc wrote: >> >> Can you send a separate CL showing how this would be used in > > encoding/gob and >> >> with benchmark results for the use? > > >> Thanks. > > > Updated https://codereview.appspot.com/111230043 to use atomic.Value, > benchmark results are updated as well. > > > https://codereview.appspot.com/120140044/
Sign in to reply to this message.
On Tue, Aug 5, 2014 at 5:48 PM, Rob Pike <r@golang.org> wrote: > Yesterday I checked in https://codereview.appspot.com/117520043, which > hoisted a call to userType out of the struct-encoding closure, and the > original complainant said it resolved the issue for him. It didn't > remove the lock in all cases, though, so a faster lock can still help, > but the urgency may be gone. > > Are you sync'ed past that CL? Yes, I am on the very tip: parent: 20524:21b205310478 tip runtime: remove outdated comment > On Tue, Aug 5, 2014 at 4:37 AM, <dvyukov@google.com> wrote: >> On 2014/08/05 04:34:06, rsc wrote: >>> >>> Can you send a separate CL showing how this would be used in >> >> encoding/gob and >>> >>> with benchmark results for the use? >> >> >>> Thanks. >> >> >> Updated https://codereview.appspot.com/111230043 to use atomic.Value, >> benchmark results are updated as well. >> >> >> https://codereview.appspot.com/120140044/
Sign in to reply to this message.
I like the API. https://codereview.appspot.com/120140044/diff/110001/src/pkg/sync/atomic/valu... File src/pkg/sync/atomic/value.go (right): https://codereview.appspot.com/120140044/diff/110001/src/pkg/sync/atomic/valu... src/pkg/sync/atomic/value.go:16: // It returns nil if there has been no call to Store. s/.$/ for this Value./ https://codereview.appspot.com/120140044/diff/110001/src/pkg/sync/atomic/valu... src/pkg/sync/atomic/value.go:22: // All calls to Store for a given Value must use values of the same type. s/type/concrete &/ https://codereview.appspot.com/120140044/diff/110001/src/pkg/sync/atomic/valu... src/pkg/sync/atomic/value.go:33: panic("sync: store of nil value into Value") s;sync;atomic; or ;sync/atomic; here and below
Sign in to reply to this message.
So what is the decision with this component? Do I need to add implementations for other platforms? https://codereview.appspot.com/120140044/diff/110001/src/pkg/sync/atomic/valu... File src/pkg/sync/atomic/value.go (right): https://codereview.appspot.com/120140044/diff/110001/src/pkg/sync/atomic/valu... src/pkg/sync/atomic/value.go:16: // It returns nil if there has been no call to Store. On 2014/08/05 20:59:01, r wrote: > s/.$/ for this Value./ Done. https://codereview.appspot.com/120140044/diff/110001/src/pkg/sync/atomic/valu... src/pkg/sync/atomic/value.go:22: // All calls to Store for a given Value must use values of the same type. On 2014/08/05 20:59:01, r wrote: > s/type/concrete &/ Done. https://codereview.appspot.com/120140044/diff/110001/src/pkg/sync/atomic/valu... src/pkg/sync/atomic/value.go:33: panic("sync: store of nil value into Value") On 2014/08/05 20:59:01, r wrote: > s;sync;atomic; or ;sync/atomic; here and below Done.
Sign in to reply to this message.
Yes, please implement the others. Then please do a real hg mail so that it shows up on the list.
Sign in to reply to this message.
Hello rsc@golang.org, josharian@gmail.com, r@golang.org, ruiu@google.com (cc: golang-codereviews@googlegroups.com), I'd like you to review this change to https://dvyukov%40google.com@code.google.com/p/go/
Sign in to reply to this message.
Added TestValueConcurrent test
Sign in to reply to this message.
I have not tested arm code, just compiled if. Dave, can you please test it. Most likely it does not work.
Sign in to reply to this message.
ping
Sign in to reply to this message.
Waiting for tests on other architectures. Or should we submit and watch the builders break? -rob On Tue, Aug 12, 2014 at 1:35 PM, <dvyukov@google.com> wrote: > ping > > https://codereview.appspot.com/120140044/
Sign in to reply to this message.
so you're not doing atomic stores yet, right? https://codereview.appspot.com/120140044/diff/270001/src/pkg/sync/atomic/valu... File src/pkg/sync/atomic/value_arm.s (right): https://codereview.appspot.com/120140044/diff/270001/src/pkg/sync/atomic/valu... src/pkg/sync/atomic/value_arm.s:14: MOVW 4(R0), R2 // load word use conditional execution could save the branch. MOVW (R0), R1 // load type CMP $0, R1 MOVW.NE 4(R0), R2 // load word MOVW.EQ $0, R2 MOVW R1, type+4(FP) MOVW R2, type+8(FP) https://codereview.appspot.com/120140044/diff/270001/src/pkg/sync/atomic/valu... src/pkg/sync/atomic/value_arm.s:39: RET no need to RET here https://codereview.appspot.com/120140044/diff/270001/src/pkg/sync/atomic/valu... src/pkg/sync/atomic/value_arm.s:42: RET same
Sign in to reply to this message.
On Wed, Aug 13, 2014 at 12:43 AM, <minux@golang.org> wrote: > so you're not doing atomic stores yet, right? I am doing atomic stores, but just not CAS. It does not affect correctness. > https://codereview.appspot.com/120140044/diff/270001/src/pkg/sync/atomic/valu... > File src/pkg/sync/atomic/value_arm.s (right): > > https://codereview.appspot.com/120140044/diff/270001/src/pkg/sync/atomic/valu... > src/pkg/sync/atomic/value_arm.s:14: MOVW 4(R0), R2 // load word > use conditional execution could save the branch. > > MOVW (R0), R1 // load type > CMP $0, R1 > MOVW.NE 4(R0), R2 // load word > MOVW.EQ $0, R2 > MOVW R1, type+4(FP) > MOVW R2, type+8(FP) > > https://codereview.appspot.com/120140044/diff/270001/src/pkg/sync/atomic/valu... > src/pkg/sync/atomic/value_arm.s:39: RET > no need to RET here > > https://codereview.appspot.com/120140044/diff/270001/src/pkg/sync/atomic/valu... > src/pkg/sync/atomic/value_arm.s:42: RET > same > > https://codereview.appspot.com/120140044/
Sign in to reply to this message.
https://codereview.appspot.com/120140044/diff/270001/src/pkg/sync/atomic/valu... File src/pkg/sync/atomic/value_arm.s (right): https://codereview.appspot.com/120140044/diff/270001/src/pkg/sync/atomic/valu... src/pkg/sync/atomic/value_arm.s:14: MOVW 4(R0), R2 // load word On 2014/08/12 20:43:07, minux wrote: > use conditional execution could save the branch. > > MOVW (R0), R1 // load type > CMP $0, R1 > MOVW.NE 4(R0), R2 // load word > MOVW.EQ $0, R2 > MOVW R1, type+4(FP) > MOVW R2, type+8(FP) Done. https://codereview.appspot.com/120140044/diff/270001/src/pkg/sync/atomic/valu... src/pkg/sync/atomic/value_arm.s:24: BEQ value_store_nil Does ARM support long conditional jumps? Can I do just BEQ ·valueStoreNilPanic(SB) ? https://codereview.appspot.com/120140044/diff/270001/src/pkg/sync/atomic/valu... src/pkg/sync/atomic/value_arm.s:39: RET On 2014/08/12 20:43:07, minux wrote: > no need to RET here Done. https://codereview.appspot.com/120140044/diff/270001/src/pkg/sync/atomic/valu... src/pkg/sync/atomic/value_arm.s:42: RET On 2014/08/12 20:43:07, minux wrote: > same Done.
Sign in to reply to this message.
On 2014/08/12 20:53:28, dvyukov wrote: > https://codereview.appspot.com/120140044/diff/270001/src/pkg/sync/atomic/valu... > File src/pkg/sync/atomic/value_arm.s (right): > > https://codereview.appspot.com/120140044/diff/270001/src/pkg/sync/atomic/valu... > src/pkg/sync/atomic/value_arm.s:14: MOVW 4(R0), R2 // load word > On 2014/08/12 20:43:07, minux wrote: > > use conditional execution could save the branch. > > > > MOVW (R0), R1 // load type > > CMP $0, R1 > > MOVW.NE 4(R0), R2 // load word > > MOVW.EQ $0, R2 > > MOVW R1, type+4(FP) > > MOVW R2, type+8(FP) > > Done. > > https://codereview.appspot.com/120140044/diff/270001/src/pkg/sync/atomic/valu... > src/pkg/sync/atomic/value_arm.s:24: BEQ value_store_nil > Does ARM support long conditional jumps? Can I do just > BEQ ·valueStoreNilPanic(SB) > ? > > https://codereview.appspot.com/120140044/diff/270001/src/pkg/sync/atomic/valu... > src/pkg/sync/atomic/value_arm.s:39: RET > On 2014/08/12 20:43:07, minux wrote: > > no need to RET here > > Done. > > https://codereview.appspot.com/120140044/diff/270001/src/pkg/sync/atomic/valu... > src/pkg/sync/atomic/value_arm.s:42: RET > On 2014/08/12 20:43:07, minux wrote: > > same > > Done. minux, can you please review and test arm code?
Sign in to reply to this message.
On Tue, Aug 12, 2014 at 4:45 PM, Dmitry Vyukov <dvyukov@google.com> wrote: > On Wed, Aug 13, 2014 at 12:43 AM, <minux@golang.org> wrote: > > so you're not doing atomic stores yet, right? > > I am doing atomic stores, but just not CAS. It does not affect correctness. but you're storing the type and value pointers separately (not atomic). would that be a problem?
Sign in to reply to this message.
https://codereview.appspot.com/120140044/diff/270001/src/pkg/sync/atomic/valu... File src/pkg/sync/atomic/value_arm.s (right): https://codereview.appspot.com/120140044/diff/270001/src/pkg/sync/atomic/valu... src/pkg/sync/atomic/value_arm.s:24: BEQ value_store_nil On 2014/08/12 20:53:28, dvyukov wrote: > Does ARM support long conditional jumps? Can I do just > BEQ ·valueStoreNilPanic(SB) > ? in theory it's possible, but our toolchain doesn't support it.
Sign in to reply to this message.
On Wed, Aug 13, 2014 at 1:08 AM, minux <minux@golang.org> wrote: > > On Tue, Aug 12, 2014 at 4:45 PM, Dmitry Vyukov <dvyukov@google.com> wrote: >> >> On Wed, Aug 13, 2014 at 12:43 AM, <minux@golang.org> wrote: >> > so you're not doing atomic stores yet, right? >> >> I am doing atomic stores, but just not CAS. It does not affect >> correctness. > > but you're storing the type and value pointers separately (not atomic). > would that be a problem? If user uses the component correctly, then it's irrelevant, because user must store the same type. If user code has a bug, then there is small window of inconsistency between the check and store that can lead to missed detection of the user bug. But we do not promise to detect it and most likely it will be detected in the next execution (this data race can't be lurking in the shadows for long time) and it is reliably detected on amd64/386. I don't have an arm machine, there are no chances I will get the CAS code right blindly. If you wish you can improve the code in a separate change (or if you give me the code to copy-paste).
Sign in to reply to this message.
https://codereview.appspot.com/120140044/diff/270001/src/pkg/sync/atomic/valu... File src/pkg/sync/atomic/value_arm.s (right): https://codereview.appspot.com/120140044/diff/270001/src/pkg/sync/atomic/valu... src/pkg/sync/atomic/value_arm.s:24: BEQ value_store_nil On 2014/08/12 21:10:51, minux wrote: > On 2014/08/12 20:53:28, dvyukov wrote: > > Does ARM support long conditional jumps? Can I do just > > BEQ ·valueStoreNilPanic(SB) > > ? > in theory it's possible, but our toolchain doesn't > support it. Acknowledged.
Sign in to reply to this message.
On Tue, Aug 12, 2014 at 5:15 PM, Dmitry Vyukov <dvyukov@google.com> wrote: > On Wed, Aug 13, 2014 at 1:08 AM, minux <minux@golang.org> wrote: > > > > On Tue, Aug 12, 2014 at 4:45 PM, Dmitry Vyukov <dvyukov@google.com> > wrote: > >> > >> On Wed, Aug 13, 2014 at 12:43 AM, <minux@golang.org> wrote: > >> > so you're not doing atomic stores yet, right? > >> > >> I am doing atomic stores, but just not CAS. It does not affect > >> correctness. > > > > but you're storing the type and value pointers separately (not atomic). > > would that be a problem? > > If user uses the component correctly, then it's irrelevant, because > user must store the same type. > If user code has a bug, then there is small window of inconsistency > between the check and store that can lead to missed detection of the > user bug. But we do not promise to detect it and most likely it will > be detected in the next execution (this data race can't be lurking in > the shadows for long time) and it is reliably detected on amd64/386. > > I don't have an arm machine, there are no chances I will get the CAS > code right blindly. If you wish you can improve the code in a separate > change (or if you give me the code to copy-paste). > OK. Let's do it as a follow-up.
Sign in to reply to this message.
This doesn't need to happen today. Please let Minux work on power64 for the rest of the week.
Sign in to reply to this message.
I don't understand why we are using the word 'component'. I've never seen that word in Go before. Change the description to just 'sync/atomic: add Value'. And if you do need to refer to what Value is somewhere else, it's a type.
Sign in to reply to this message.
R=rsc I still need to read the assembly. The rest looks good. https://codereview.appspot.com/120140044/diff/290001/src/pkg/sync/atomic/nora... File src/pkg/sync/atomic/norace.go (right): https://codereview.appspot.com/120140044/diff/290001/src/pkg/sync/atomic/nora... src/pkg/sync/atomic/norace.go:9: import ( drop () https://codereview.appspot.com/120140044/diff/290001/src/pkg/sync/atomic/valu... File src/pkg/sync/atomic/value.go (right): https://codereview.appspot.com/120140044/diff/290001/src/pkg/sync/atomic/valu... src/pkg/sync/atomic/value.go:33: if raceenabled { I'd like to avoid having any assembly making calls to other code. Make this if x == nil { panic("sync/atomic: store of nil value into Value") } if !valueStore(v, x) { panic("sync/atomic: store of inconsistently typed value into Value") } and change the prototype of valueStore to return a bool. This will also shorten the assembly, which is always nice.
Sign in to reply to this message.
https://codereview.appspot.com/120140044/diff/290001/src/pkg/sync/atomic/nora... File src/pkg/sync/atomic/norace.go (right): https://codereview.appspot.com/120140044/diff/290001/src/pkg/sync/atomic/nora... src/pkg/sync/atomic/norace.go:9: import ( On 2014/08/20 18:28:50, rsc wrote: > drop () Done. https://codereview.appspot.com/120140044/diff/290001/src/pkg/sync/atomic/valu... File src/pkg/sync/atomic/value.go (right): https://codereview.appspot.com/120140044/diff/290001/src/pkg/sync/atomic/valu... src/pkg/sync/atomic/value.go:33: if raceenabled { On 2014/08/20 18:28:51, rsc wrote: > I'd like to avoid having any assembly making calls to other code. Make this > > if x == nil { > panic("sync/atomic: store of nil value into Value") > } > if !valueStore(v, x) { > panic("sync/atomic: store of inconsistently typed value into Value") > } > > and change the prototype of valueStore to return a bool. > > This will also shorten the assembly, which is always nice. good idea! Done
Sign in to reply to this message.
On 2014/08/20 18:23:44, rsc wrote: > I don't understand why we are using the word 'component'. I've never seen > that word in Go before. Change the description to just 'sync/atomic: add > Value'. And if you do need to refer to what Value is somewhere else, it's a > type. fixed PTAL
Sign in to reply to this message.
Right now it looks to me like the assembly for load loads the type word and
then the data word. The assembly for store does some checking of the type
word, then stores the type word, then stores the data word. It seems to me
that a simultaneous load and first store might result in a load returning a
typed nil pointer. Effectively you could have:
store writes type word
load executes, reading type word and uninitialized (zero, likely incorrect)
data word
store writes data word
This needs to be fixed.
I had suggested writing these in assembly because the data word was
ambiguously typed and because I thought they were straightforward. I fixed
the ambiguous typing as of CL 130240043, and it's now clear to me that
these are not straightforward. I now think we should write them in Go,
using CompareAndSwapPointer. Even if we roll back the data word change for
Go 1.4, using CompareAndSwapPointer won't break anything. And there will be
only one copy of this subtle code instead of three.
I suggest the following (untested).
// valueStore must distinguish the first store from later stores,
// both to implement the check for consistent typing and
// to make sure a concurrent load does not observe a partial first store.
// To do this, valueStore checks for v.type == 0 and tries to cas it from 0
to ^0.
// If successful, this call is the first store: it writes v.data and then
updates
// v.type to the actual type. Otherwise, valueStore must wait until v.type
!= ^0
// and then can check v.type and write only v.data.
//
// valueLoad reads v.type first. If v.type is 0 or -1, no store has
completed
// and valueLoad can return nil. Otherwise it returns {v.type, v.word}.
type ifaceWords struct {
typ unsafe.Pointer
data unsafe.Pointer
}
func valueStore(v *Value, x interface{}) bool {
vp := (*ifaceWords)(unsafe.Pointer(v))
xp := (*ifaceWords)(&x)
for {
typ := LoadPointer(&vp.typ)
if typ == nil {
// Attempt to start first store.
if !CompareAndSwapPointer(&vp.typ, nil, unsafe.Pointer(^uintptr(0))) {
continue
}
// Complete first store.
StorePointer(&vp.data, x.data)
StorePointer(&vp.type, x.type)
return true
}
if uintptr(typ) == ^uintptr(0) {
// First store in progress. Wait.
continue
}
// First store completed. Check type and overwrite data.
if typ != xp.typ {
return false
}
StorePointer(&vp.data, x.data)
return true
}
}
func valueLoad(v *Value) (x interface{}) {
vp := (*ifaceWords)(unsafe.Pointer(v))
typ := LoadPointer(&vp.typ)
if typ == nil || uintptr(typ) == ^uintptr(0) {
// First store not yet completed.
return nil
}
data := LoadPointer(&vp.data)
xp := (*ifaceWords)(&x)
xp.typ = typ
xp.data = data
return
}
Sign in to reply to this message.
Done PTAL
Sign in to reply to this message.
LGTM
Sign in to reply to this message.
I've recreated this CL as: https://codereview.appspot.com/136710045 It's all broken after pkg move. It does not apply in a new client: $ hg clpatch 120140044 unexpected chunk header line: @@ --266,6 +-266,16 @@ and in old client I can't upload it: $ hg upload 120140044 abort: src/sync/atomic/norace.go@c79b24056eb1: not found in manifest!
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
