Issue 101330056: code review 101330056: doc: ban data races
Descriptiondoc: ban data races
The current wording tries to give at least some guarantees
to programs with data races.
This is (1) not very useful, as races on primitive types can be easily
fixed using sync/atomic package, (2) most likely is not implemented,
in particular in gccgo which I believe assumes race-free programs,
(3) can inhibit useful optimizations in future (e.g. code generation
optimizations or concurrent garbage collection).
I believe this classifies as "bug fix" with respect to Go1 promise.
Patch Set 1 #Patch Set 2 : diff -r a4f413948849 https://dvyukov%40google.com@code.google.com/p/go/ #Patch Set 3 : diff -r a4f413948849 https://dvyukov%40google.com@code.google.com/p/go/ #
Total comments: 1
MessagesTotal messages: 16
Hello golang-codereviews@googlegroups.com (cc: khr@golang.org, r@golang.org, rsc@golang.org), 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.
I agree with these points, I think this should land (I'm deliberately avoid those four consonants)
Sign in to reply to this message.
https://codereview.appspot.com/101330056/diff/40001/doc/go_mem.html File doc/go_mem.html (right): https://codereview.appspot.com/101330056/diff/40001/doc/go_mem.html#newcode63 doc/go_mem.html:63: Any such data race results in <i>undefined behavior</i>. Perhaps ironically, the Go spec and memory model docs never define the term "undefined behavior."
Sign in to reply to this message.
R=rsc@golang.org (assigned by r@golang.org)
Sign in to reply to this message.
I am inclined to set a moratorium on all changes to this document. Nothing about a typical Go user's experience is improved by continuing to refine the wording here. Failing that, at the very least any change needs to be motivated not by hypotheticals but by real examples of compiler analysis or rewrites or optimizations that we do today (or want to do very soon) that are ruled out by the current wording. Russ
Sign in to reply to this message.
Ian, can you confirm that gccgo implements current guarantees for racing memory accesses? If the middle end treats Go code as C code, that I would expect it to not be the case. As far as I know, gcc sometimes emits 2 32-bit stores instead of a single 64-bit store on amd64. Rick, can you comment something on racy pointer memory accesses versus concurrent garbage collection? Can races on pointers be a problem? Russ, do you still want to modify this doc to specify how atomic operations interact with MM? There is an open issue for that. On Mon, Jun 23, 2014 at 3:58 PM, Russ Cox <rsc@golang.org> wrote: > I am inclined to set a moratorium on all changes to this document. Nothing > about a typical Go user's experience is improved by continuing to refine the > wording here. > > Failing that, at the very least any change needs to be motivated not by > hypotheticals but by real examples of compiler analysis or rewrites or > optimizations that we do today (or want to do very soon) that are ruled out > by the current wording. > > Russ >
Sign in to reply to this message.
If I could, I would replace this entire doc with
Go Memory Model
Use locks and channels. Don't be clever.
People can get way too obsessed with language lawyering here.
Russ
Sign in to reply to this message.
I'd be happy to start the document with: Go Memory Model Use locks and channels. Don't be clever. For thoroughness, the rest of this document explains things in more detail, but if you need to read this document to decide if your program is correct, you are being clever. On Mon, Jun 23, 2014 at 5:04 PM, Russ Cox <rsc@golang.org> wrote: > If I could, I would replace this entire doc with > > Go Memory Model > > Use locks and channels. Don't be clever. > > People can get way too obsessed with language lawyering here. > > Russ
Sign in to reply to this message.
LGTM. On Tue, Jun 24, 2014 at 10:06 AM, Rob Pike <r@golang.org> wrote: > I'd be happy to start the document with: > > Go Memory Model > > Use locks and channels. Don't be clever. > > For thoroughness, the rest of this document explains things in more > detail, but if you need to read this document to decide if your > program is correct, you are being clever. > > > On Mon, Jun 23, 2014 at 5:04 PM, Russ Cox <rsc@golang.org> wrote: >> If I could, I would replace this entire doc with >> >> Go Memory Model >> >> Use locks and channels. Don't be clever. >> >> People can get way too obsessed with language lawyering here. >> >> Russ
Sign in to reply to this message.
It's not as simple. People can do weird things with channels as well.
And then somebody needs to answer the question whether it's correct
code or not. For example consider:
c := make(chan int, 1)
s := ""
go func() {
s = "foo"
c <- 1
}()
go func() {
select {
case c <- 1:
default:
println(s)
}
}()
This program uses only channels for synchronization. Is it guaranteed
to print "foo"?
On Mon, Jun 23, 2014 at 5:06 PM, Rob Pike <r@golang.org> wrote:
> I'd be happy to start the document with:
>
> Go Memory Model
>
> Use locks and channels. Don't be clever.
>
> For thoroughness, the rest of this document explains things in more
> detail, but if you need to read this document to decide if your
> program is correct, you are being clever.
>
>
> On Mon, Jun 23, 2014 at 5:04 PM, Russ Cox <rsc@golang.org> wrote:
>> If I could, I would replace this entire doc with
>>
>> Go Memory Model
>>
>> Use locks and channels. Don't be clever.
>>
>> People can get way too obsessed with language lawyering here.
>>
>> Russ
Sign in to reply to this message.
On Mon, Jun 23, 2014 at 5:06 PM, Rob Pike <r@golang.org> wrote: > I'd be happy to start the document with: > > Go Memory Model > > Use locks and channels. Don't be clever. > > For thoroughness, the rest of this document explains things in more > detail, but if you need to read this document to decide if your > program is correct, you are being clever. Yes please!
Sign in to reply to this message.
On Jun 23, 2014, at 7:06 PM, Rob Pike <r@golang.org> wrote: > I'd be happy to start the document with: > > Go Memory Model > > Use locks and channels. Don't be clever. > > For thoroughness, the rest of this document explains things in more > detail, but if you need to read this document to decide if your > program is correct, you are being clever. I know this statement was a joke based on truth, but I am someone who did not have any formal training in concurrent programming, and I work with people who think in parallelism rather than concurrency. I appreciated the detail and technicality in the memory model. It shows a formal way to think about concurrent programming (your talks give the high-level vision). I don’t know enough about compilers and runtimes to have an opinion about the proposed changes, but I just wanted to say that I appreciate the technicalness of the memory model. I appreciate the desire for it to be legible for most users, but at the same time I appreciate it being correct. Just my two cents. > > > On Mon, Jun 23, 2014 at 5:04 PM, Russ Cox <rsc@golang.org> wrote: >> If I could, I would replace this entire doc with >> >> Go Memory Model >> >> Use locks and channels. Don't be clever. >> >> People can get way too obsessed with language lawyering here. >> >> Russ > > -- > You received this message because you are subscribed to a topic in the Google Groups "golang-codereviews" group. > To unsubscribe from this topic, visit https://groups.google.com/d/topic/golang-codereviews/IUWZ5ADlIoY/unsubscribe. > To unsubscribe from this group and all its topics, send an email to golang-codereviews+unsubscribe@googlegroups.com. > For more options, visit https://groups.google.com/d/optout.
Sign in to reply to this message.
On Mon, Jun 23, 2014 at 8:16 PM, Dmitry Vyukov <dvyukov@google.com> wrote: > It's not as simple. People can do weird things with channels as well. > And then somebody needs to answer the question whether it's correct > code or not. For example consider: > > c := make(chan int, 1) > s := "" > go func() { > s = "foo" > c <- 1 > }() > go func() { > select { > case c <- 1: > default: > println(s) > } > }() > > This program uses only channels for synchronization. Is it guaranteed > to print "foo"? > Exactly my point. Use locks and channels. *Don't be clever.* Russ
Sign in to reply to this message.
On Mon, Jun 23, 2014 at 5:01 PM, Dmitry Vyukov <dvyukov@google.com> wrote: > Ian, can you confirm that gccgo implements current guarantees for > racing memory accesses? If the middle end treats Go code as C code, > that I would expect it to not be the case. As far as I know, gcc > sometimes emits 2 32-bit stores instead of a single 64-bit store on > amd64. I don't think the current memory model is intended to guarantee that given a concurrent read and write that the read will observe either a non-write or a complete write. What if you retry this CL without mentioning "code generation optimizations" in the description? Because I think this CL is mostly a simplification. It's not obvious that we care about code generation optimizations in this area. But I think it's fine to simplify this doc. Ian
Sign in to reply to this message.
Until there is an actual demonstrated need to change this doc, please don't. Each of us has more useful things to do. Russ
Sign in to reply to this message.
R=close To the author of this CL: The Go project has moved to Gerrit Code Review. If this CL should be continued, please see the latest version of https://golang.org/doc/contribute.html for instructions on how to set up Git and the Go project's Gerrit codereview plugin, and then create a new change with your current code. If there has been discussion on this CL, please give a link to it (golang.org/cl/101330056 is best) in the description in your new CL. Thanks very much.
Sign in to reply to this message.
|
