code review 158500043: doc/go_mem.html: don't be clever

doc/go_mem.html: don't be clever

Add a short introductory section saying what most Go
programmers really need to know, which is that you
shouldn't have to read this document to understand
the behavior of your program.

[r, 2014-10-25 18:18:43]

Hello golang-codereviews@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go/

[bradfitz, 2014-10-25 18:23:45]

Typo in protect.
 On Oct 25, 2014 11:18 AM, <r@golang.org> wrote:

> Reviewers: golang-codereviews,
>
> Message:
> Hello golang-codereviews@googlegroups.com,
>
> I'd like you to review this change to
> https://code.google.com/p/go/
>
>
> Description:
> doc/go_mem.html: don't be clever
>
> Add a short introductory section saying what most Go
> programmers really need to know, which is that you
> shouldn't have to read this document to understand
> the behavior of your program.
>
> Please review this at https://codereview.appspot.com/158500043/
>
> Affected files (+22, -0 lines):
>   M doc/go_mem.html
>
>
> Index: doc/go_mem.html
> ===================================================================
> --- a/doc/go_mem.html
> +++ b/doc/go_mem.html
> @@ -21,6 +21,28 @@
>  observe values produced by writes to the same variable in a different
> goroutine.
>  </p>
>
> +
> +<h2>Advice</h2>
> +
> +<p>
> +Avoid sharing data between simultaneously executing goroutines.
> +</p>
> +
> +<p>
> +If you must share, protetct the data with channel operations or other
> synchronization primitives
> +such as those in the <a href="/pkg/sync/"><code>sync</code></a>
> +and <a href="/pkg/sync/atomic/"><code>sync/atomic</code></a> packages.
> +</p>
> +
> +<p>
> +If you must read the rest of this document to understand the behavior of
> your program,
> +you are being too clever.
> +</p>
> +
> +<p>
> +Don't be clever.
> +</p>
> +
>  <h2>Happens Before</h2>
>
>  <p>
>
>
> --
> You received this message because you are subscribed to the Google Groups
> "golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
>

[r, 2014-10-25 19:51:29]

Hello golang-codereviews@googlegroups.com, bradfitz@golang.org (cc:
golang-codereviews@googlegroups.com),

Please take another look.

[bradfitz, 2014-10-25 20:05:20]

LGTM

[btracey, 2014-10-25 23:14:24]

https://codereview.appspot.com/158500043/diff/20001/doc/go_mem.html
File doc/go_mem.html (right):

https://codereview.appspot.com/158500043/diff/20001/doc/go_mem.html#newcode28
doc/go_mem.html:28: Avoid sharing data between simultaneously executing
goroutines.
As I understand it, shared data is fine if it's read-only. Maybe this should
say:
"Avoid modifying data that is shared among multiple goroutines."

https://codereview.appspot.com/158500043/diff/20001/doc/go_mem.html#newcode39
doc/go_mem.html:39: you are being too clever.
If I hadn't followed the discussion toward this change, I would interpret this
as "don't read this document". Perhaps something like:

"If the behavior of your program depends on the minutiea of this document, you
are being too clever."

[adg1, 2014-10-25 23:33:24]

https://codereview.appspot.com/158500043/diff/20001/doc/go_mem.html
File doc/go_mem.html (right):

https://codereview.appspot.com/158500043/diff/20001/doc/go_mem.html#newcode28
doc/go_mem.html:28: Avoid sharing data between simultaneously executing
goroutines.
On 2014/10/25 23:14:24, btracey wrote:
> As I understand it, shared data is fine if it's read-only. Maybe this should
> say:
> "Avoid modifying data that is shared among multiple goroutines."

I agree this needs to be finessed; there are ~zero Go programs that use
goroutines that do not share data between them.

[r, 2014-10-25 23:59:04]

Hello golang-codereviews@googlegroups.com, bradfitz@golang.org,
tracey.brendan@gmail.com, adg@google.com (cc:
golang-codereviews@googlegroups.com),

Please take another look.

[r, 2014-10-26 00:03:32]

"If I hadn't followed the discussion toward this change, I would
interpret this as "don't read this document"."

That is exactly the message I am trying to send. Almost no one needs to
read this document.

-rob

[adg1, 2014-10-26 00:04:18]

LGTM

[btracey, 2014-10-26 00:06:51]

On Saturday, October 25, 2014 5:03:30 PM UTC-7, Rob 'Commander' Pike wrote:
> "If I hadn't followed the discussion toward this change, I would
> interpret this as "don't read this document"."
> 
> 
> That is exactly the message I am trying to send. Almost no one needs to read
this document.
> 
> 
> 
> -rob

I didn't realize the intent was to discourage people from reading the document.
(I personally found it interesting, but that's beside the point).

LGTM

[r, 2014-10-26 00:17:29]

You can read it if you want, but you shouldn't *need* to. The point of the
concurrency primitives in Go is to make concurrency easy to use. For those
who care more about ultimate performance and want to implement lock-free
algorithms and fine-tune memory barriers, semaphores, and condition
variables, Go's memory model is inadequate. That is on purpose.

For the far smaller fraction of the programming world that truly
*understands* these issues (a subset that does not include me, which is why
Go has these primitives), this document is woefully incomplete.

We are fine with that. Stronger: we want that.

Follow the advice in this new section and your programs will be correct and
you should be happy that's all you need to know.

-rob

[btracey, 2014-10-26 00:28:47]

I agree with the intent. My suggestion was merely that the line reads as “Don’t
read this document” rather than “You shouldn’t need to read this document”. The
line does say what you mean, and so it’s fine to keep your wording (hence the
LGTM). I was attempting to suggest a wording which says “the behavior of your
code shouldn’t depend on the specifics of this document” rather than warding off
people who are searching for understanding. It’s fine as is though. 

 
On Oct 25, 2014, at 5:17 PM, Rob Pike <r@golang.org> wrote:

> You can read it if you want, but you shouldn't need to. The point of the
concurrency primitives in Go is to make concurrency easy to use. For those who
care more about ultimate performance and want to implement lock-free algorithms
and fine-tune memory barriers, semaphores, and condition variables, Go's memory
model is inadequate. That is on purpose.
> 
> For the far smaller fraction of the programming world that truly understands
these issues (a subset that does not include me, which is why Go has these
primitives), this document is woefully incomplete.
> 
> We are fine with that. Stronger: we want that.
> 
> Follow the advice in this new section and your programs will be correct and
you should be happy that's all you need to know.
> 
> -rob
> 

[iant, 2014-10-27 03:32:25]

LGTM

[rsc, 2014-10-27 21:02:17]

LGTM

[dsymonds, 2014-10-27 23:52:05]

LGTM

[iant, 2014-10-27 23:56:03]

It occurs to me that this will become the only text in the memory model that
refers to the sync/atomic package.  Which is OK but serves as a reminder that we
should some day add something about the sync/atomic package to the memory model
document.

[r, 2014-10-28 00:08:56]

*** Submitted as https://code.google.com/p/go/source/detail?r=11b6d55be064 ***

doc/go_mem.html: don't be clever

Add a short introductory section saying what most Go
programmers really need to know, which is that you
shouldn't have to read this document to understand
the behavior of your program.

LGTM=bradfitz, adg, tracey.brendan, iant, rsc, dsymonds
R=golang-codereviews, bradfitz, tracey.brendan, adg, iant, rsc, dsymonds
CC=golang-codereviews
https://codereview.appspot.com/158500043

[gobot, 2014-10-28 00:10:34]

This CL appears to have broken the solaris-amd64-smartos builder.
See http://build.golang.org/log/4b39eefb2559e491924c94a161fa7a6222142d81

[adg1, 2014-10-28 00:18:08]

Such irony, this false positive comes just the day after we removed this
builder from the flaky builders list.

On Tue Oct 28 2014 at 11:10:35 AM <gobot@golang.org> wrote:

> This CL appears to have broken the solaris-amd64-smartos builder.
> See http://build.golang.org/log/4b39eefb2559e491924c94a161fa7a6222142d81
>
> https://codereview.appspot.com/158500043/
>

[aram2, 2014-10-28 10:05:45]

Indeed!

-- 
Aram Hăvărneanu