Issue 6948043: code review 6948043: doc: add race detector manual (Closed)
|
Created:
11 years, 4 months ago by dvyukov Modified:
11 years, 3 months ago Reviewers:
CC:
minux1, fss, rsc, adg, adg1, golang-dev Visibility:
Public. |
Descriptiondoc: add race detector manual
Patch Set 1 #Patch Set 2 : diff -r 89e5cabaa09a https://dvyukov%40google.com@code.google.com/p/go/ #Patch Set 3 : diff -r 89e5cabaa09a https://dvyukov%40google.com@code.google.com/p/go/ #Patch Set 4 : diff -r 89e5cabaa09a https://dvyukov%40google.com@code.google.com/p/go/ #
Total comments: 18
Patch Set 5 : diff -r a9edbc33dcee https://dvyukov%40google.com@code.google.com/p/go/ #Patch Set 6 : diff -r a9edbc33dcee https://dvyukov%40google.com@code.google.com/p/go/ #
Total comments: 7
Patch Set 7 : diff -r 8ac6a466383d https://dvyukov%40google.com@code.google.com/p/go/ #
Total comments: 23
Patch Set 8 : diff -r 9d15015fc6e2 https://dvyukov%40google.com@code.google.com/p/go/ #Patch Set 9 : diff -r 9d15015fc6e2 https://dvyukov%40google.com@code.google.com/p/go/ #Patch Set 10 : diff -r 9d15015fc6e2 https://dvyukov%40google.com@code.google.com/p/go/ #
Total comments: 10
Patch Set 11 : diff -r e7cd0a82d669 https://dvyukov%40google.com@code.google.com/p/go/ #
Total comments: 3
Patch Set 12 : diff -r 72648c5c21a1 https://dvyukov%40google.com@code.google.com/p/go/ #Patch Set 13 : diff -r 72648c5c21a1 https://dvyukov%40google.com@code.google.com/p/go/ #
Total comments: 2
Patch Set 14 : diff -r f4e5087c1c19 https://dvyukov%40google.com@code.google.com/p/go/ #Patch Set 15 : diff -r 21096a13f14b https://dvyukov%40google.com@code.google.com/p/go/ #
MessagesTotal messages: 28
Hello golang-dev@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.
https://codereview.appspot.com/6948043/diff/3/doc/articles/race_detector.html File doc/articles/race_detector.html (right): https://codereview.appspot.com/6948043/diff/3/doc/articles/race_detector.html... doc/articles/race_detector.html:6: <h2 id="Introduction">Introduction</h2> if you don't need to quote a specific section of this article by URL (like tip.golang.org/doc/articles/race_detector.html#Introduction), i think you can just drop the id="xx" part. it won't affect the toc. https://codereview.appspot.com/6948043/diff/3/doc/articles/race_detector.html... doc/articles/race_detector.html:22: c <- true s/</</g there are other instances. https://codereview.appspot.com/6948043/diff/3/doc/articles/race_detector.html... doc/articles/race_detector.html:89: You can pass some options to the race detector by means of `GORACE` environment variable. The format is: <code>GORACE</code> https://codereview.appspot.com/6948043/diff/3/doc/articles/race_detector.html... doc/articles/race_detector.html:115: You may start with just running your tests under the race detector (go test -race). However sometimes tests have limited coverage, especially with respect to concurrency. The race detector finds only races that actually happen in the execution, it can't find races in code paths that were not executed. So it may be beneficial to run the whole program built with -race under a realistic workload, frequently it discovers much more bugs than tests. <code>go test -race</code> https://codereview.appspot.com/6948043/diff/3/doc/articles/race_detector.html... doc/articles/race_detector.html:191: The fix is simple, one just needs to introduce new variables in the goroutines (note :=): <code>:=</code> https://codereview.appspot.com/6948043/diff/3/doc/articles/race_detector.html... doc/articles/race_detector.html:203: If the following code is called from several goroutines, it leads to bad races on the services map. <code>services</code> https://codereview.appspot.com/6948043/diff/3/doc/articles/race_detector.html... doc/articles/race_detector.html:242: Data races can happen on variables of primitive types as well (bool, int, int64), like in the following example: <code>bool</code>, etc. https://codereview.appspot.com/6948043/diff/3/doc/articles/race_detector.html... doc/articles/race_detector.html:271: To fix such data race one can use (aside from chan and sync.Mutex) package sync/atomic, which provides atomic operations on primitive types. sync/atomic functions solve all of the above issues. <code>chan</code>, etc. <a href="/pkg/sync/atomic><code>sync/atomic</code></a> https://codereview.appspot.com/6948043/diff/3/doc/articles/race_detector.html... doc/articles/race_detector.html:303: The data race detector significantly increases both memory consumption and execution time. The concrete numbers highly dependent on the particular program, but some reference numbers would be: memory consumption ~5-10x, execution time ~2-20x. "The concrete numbers _are_ highly dependent on .."?
Sign in to reply to this message.
https://codereview.appspot.com/6948043/diff/3/doc/articles/race_detector.html File doc/articles/race_detector.html (right): https://codereview.appspot.com/6948043/diff/3/doc/articles/race_detector.html... doc/articles/race_detector.html:6: <h2 id="Introduction">Introduction</h2> On 2012/12/13 19:29:50, minux wrote: > if you don't need to quote a specific section of this article by URL > (like tip.golang.org/doc/articles/race_detector.html#Introduction), > i think you can just drop the id="xx" part. > > it won't affect the toc. Done. https://codereview.appspot.com/6948043/diff/3/doc/articles/race_detector.html... doc/articles/race_detector.html:22: c <- true On 2012/12/13 19:29:50, minux wrote: > s/</</g > > there are other instances. Done. https://codereview.appspot.com/6948043/diff/3/doc/articles/race_detector.html... doc/articles/race_detector.html:89: You can pass some options to the race detector by means of `GORACE` environment variable. The format is: On 2012/12/13 19:29:50, minux wrote: > <code>GORACE</code> Done. https://codereview.appspot.com/6948043/diff/3/doc/articles/race_detector.html... doc/articles/race_detector.html:115: You may start with just running your tests under the race detector (go test -race). However sometimes tests have limited coverage, especially with respect to concurrency. The race detector finds only races that actually happen in the execution, it can't find races in code paths that were not executed. So it may be beneficial to run the whole program built with -race under a realistic workload, frequently it discovers much more bugs than tests. On 2012/12/13 19:29:50, minux wrote: > <code>go test -race</code> Done. https://codereview.appspot.com/6948043/diff/3/doc/articles/race_detector.html... doc/articles/race_detector.html:191: The fix is simple, one just needs to introduce new variables in the goroutines (note :=): On 2012/12/13 19:29:50, minux wrote: > <code>:=</code> Done. https://codereview.appspot.com/6948043/diff/3/doc/articles/race_detector.html... doc/articles/race_detector.html:203: If the following code is called from several goroutines, it leads to bad races on the services map. On 2012/12/13 19:29:50, minux wrote: > <code>services</code> Done. https://codereview.appspot.com/6948043/diff/3/doc/articles/race_detector.html... doc/articles/race_detector.html:242: Data races can happen on variables of primitive types as well (bool, int, int64), like in the following example: On 2012/12/13 19:29:50, minux wrote: > <code>bool</code>, etc. Done. https://codereview.appspot.com/6948043/diff/3/doc/articles/race_detector.html... doc/articles/race_detector.html:271: To fix such data race one can use (aside from chan and sync.Mutex) package sync/atomic, which provides atomic operations on primitive types. sync/atomic functions solve all of the above issues. On 2012/12/13 19:29:50, minux wrote: > <code>chan</code>, etc. > <a href="/pkg/sync/atomic><code>sync/atomic</code></a> Done.
Sign in to reply to this message.
Hello golang-dev@googlegroups.com, minux.ma@gmail.com (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
LGTM modulo the template issue below. please wait for others before submitting. https://codereview.appspot.com/6948043/diff/4/doc/articles/race_detector.html File doc/articles/race_detector.html (right): https://codereview.appspot.com/6948043/diff/4/doc/articles/race_detector.html... doc/articles/race_detector.html:3: "Template": true does this document really need templates? If you want to separate all the Go code segment out into their own files under /doc/progs, and also make run.bash test their compilation you can keep this to true but please extract all the big code segments out. you can see doc/articles/image_package.html for examples.
Sign in to reply to this message.
Some minor details. https://codereview.appspot.com/6948043/diff/4/doc/articles/race_detector.html File doc/articles/race_detector.html (right): https://codereview.appspot.com/6948043/diff/4/doc/articles/race_detector.html... doc/articles/race_detector.html:9: Data races are one of the most common and hardest to debug types of bugs in concurrent systems. A data race occurs when two goroutines access the same variable w/o proper synchronization and at least one of the accesses is write. See the <a href="/ref/mem">The Go Memory Model</a> for details. Isn't "without" better than "w/o"? https://codereview.appspot.com/6948043/diff/4/doc/articles/race_detector.html... doc/articles/race_detector.html:35: Fortunately, Go includes built-in data race detector. The usage is very simple -- you just need to add -race flag to go command: s/includes built-in/includes a built-in/ s/flag to go command/flag to the go command/ https://codereview.appspot.com/6948043/diff/4/doc/articles/race_detector.html... doc/articles/race_detector.html:121: Here are some example of typical data races. All of them can be automatically detected with the race detector. s/example/examples/
Sign in to reply to this message.
Done. Thanks! https://codereview.appspot.com/6948043/diff/4/doc/articles/race_detector.html File doc/articles/race_detector.html (right): https://codereview.appspot.com/6948043/diff/4/doc/articles/race_detector.html... doc/articles/race_detector.html:9: Data races are one of the most common and hardest to debug types of bugs in concurrent systems. A data race occurs when two goroutines access the same variable w/o proper synchronization and at least one of the accesses is write. See the <a href="/ref/mem">The Go Memory Model</a> for details. On 2012/12/14 12:16:05, fss wrote: > Isn't "without" better than "w/o"? Done. https://codereview.appspot.com/6948043/diff/4/doc/articles/race_detector.html... doc/articles/race_detector.html:35: Fortunately, Go includes built-in data race detector. The usage is very simple -- you just need to add -race flag to go command: On 2012/12/14 12:16:05, fss wrote: > s/includes built-in/includes a built-in/ > > s/flag to go command/flag to the go command/ Done. https://codereview.appspot.com/6948043/diff/4/doc/articles/race_detector.html... doc/articles/race_detector.html:121: Here are some example of typical data races. All of them can be automatically detected with the race detector. On 2012/12/14 12:16:05, fss wrote: > s/example/examples/ Done.
Sign in to reply to this message.
I tried to tighten much of the text. I am not 100% sure it belongs here. Maybe it should be a package-level doc comment for runtime/race. Will leave that decision for adg. https://codereview.appspot.com/6948043/diff/6003/doc/articles/race_detector.html File doc/articles/race_detector.html (right): https://codereview.appspot.com/6948043/diff/6003/doc/articles/race_detector.h... doc/articles/race_detector.html:9: Data races are one of the most common and hardest to debug types of bugs in concurrent systems. A data race occurs when two goroutines access the same variable without proper synchronization and at least one of the accesses is write. See the <a href="/ref/mem">The Go Memory Model</a> for details. s/without proper synchronization/concurrently/ https://codereview.appspot.com/6948043/diff/6003/doc/articles/race_detector.h... doc/articles/race_detector.html:13: Here is an example of a data race on map variable that can lead to crashes and memory corruptions: s/on map variable // https://codereview.appspot.com/6948043/diff/6003/doc/articles/race_detector.h... doc/articles/race_detector.html:35: Fortunately, Go includes a built-in data race detector. The usage is very simple -- you just need to add -race flag to the go command: s/The usage.*/To use it, add the -race flag to the go command: https://codereview.appspot.com/6948043/diff/6003/doc/articles/race_detector.h... doc/articles/race_detector.html:48: When the race detector finds a data race in the program, it prints an informative report. The report contains stack traces for conflicting accesses, as well as stacks where the involved goroutines were created. You may see an example below: s/an informative/a/ s/You may see an example below/For example/ https://codereview.appspot.com/6948043/diff/6003/doc/articles/race_detector.h... doc/articles/race_detector.html:89: You can pass some options to the race detector by means of <code>GORACE</code> environment variable. The format is: The GORACE environment variable sets race detector options. The format is: https://codereview.appspot.com/6948043/diff/6003/doc/articles/race_detector.h... doc/articles/race_detector.html:99: <li> log_path: Tells race detector to write reports to 'log_path.pid' file. The special values are 'stdout' and 'stderr'. The default is 'stderr'.</li> log_path (default stderr): The race detector writes its report to a file named log_path.pid. The special names stdout and stderr cause reports to be written to standard output and standard error, respectively. https://codereview.appspot.com/6948043/diff/6003/doc/articles/race_detector.h... doc/articles/race_detector.html:100: <li> exitcode: Override exit status of the process if something was reported. Default value is 66.</li> exitcode (default 66): The exit status to use when exiting after a detected race. https://codereview.appspot.com/6948043/diff/6003/doc/articles/race_detector.h... doc/articles/race_detector.html:101: <li> strip_path_prefix: Allows to strip beginnings of file paths in reports to make them more concise.</li> strip_path_prefix (default ""): Strip this prefix from all reported file paths, to make reports more concise. https://codereview.appspot.com/6948043/diff/6003/doc/articles/race_detector.h... doc/articles/race_detector.html:102: <li> history_size: Per-goroutine history size, controls how many previous memory accesses are remembered per goroutine. Possible values are [0..7]. history_size=0 amounts to 32K memory accesses. Each next value doubles the amount of memory accesses, up to history_size=7 that amounts to 4M memory accesses. The default value is 1 (64K memory accesses). Try to increase this value when you see "failed to restore the stack" in reports. However, it can significantly increase memory consumption.</li> history_size (default 1): The per-goroutine memory access history is 32K * 2**history_size elements. Increasing this value can avoid a "failed to restore the stack" error in reports, but at the cost of increased memory usage. https://codereview.appspot.com/6948043/diff/6003/doc/articles/race_detector.h... doc/articles/race_detector.html:115: You may start with just running your tests under the race detector (<code>go test -race</code>). However sometimes tests have limited coverage, especially with respect to concurrency. The race detector finds only races that actually happen in the execution, it can't find races in code paths that were not executed. So it may be beneficial to run the whole program built with -race under a realistic workload, frequently it discovers much more bugs than tests. To start, run your tests using the race detector (go test -race). The race detector only finds races that happen at runtime, so it can't find races in code paths that are not executed. If your tests have incomplete coverage, you may find more races by running a binary built with -race under a realistic workload. https://codereview.appspot.com/6948043/diff/6003/doc/articles/race_detector.h... doc/articles/race_detector.html:121: Here are some examples of typical data races. All of them can be automatically detected with the race detector. s/examples of // s/automatically // https://codereview.appspot.com/6948043/diff/6003/doc/articles/race_detector.h... doc/articles/race_detector.html:141: Closures capture variables by reference rather than by value, so the reads of the <code>i</code> variable in the goroutines race with <code>i</code> increment in the loop statement. Such program typically outputs 55555 instead of expected 01234. The program can be fixed by explicitly making a copy of the loop counter: The variable i in the function literal is the same variable used by the loop, so the read in the goroutine races with the loop increment. (This program typically prints 55555, not 01234.) The program can be fixed by making a copy of the variable: https://codereview.appspot.com/6948043/diff/6003/doc/articles/race_detector.h... doc/articles/race_detector.html:191: The fix is simple, one just needs to introduce new variables in the goroutines (note <code>:=</code>): The fix is to introduce new ... https://codereview.appspot.com/6948043/diff/6003/doc/articles/race_detector.h... doc/articles/race_detector.html:203: If the following code is called from several goroutines, it leads to bad races on the <code>services</code> map. Concurrent reads and writes of a map are not safe: https://codereview.appspot.com/6948043/diff/6003/doc/articles/race_detector.h... doc/articles/race_detector.html:213: func GetService(name string) net.Addr { s/Get/Lookup/ https://codereview.appspot.com/6948043/diff/6003/doc/articles/race_detector.h... doc/articles/race_detector.html:219: It can be fixed by protecting the accesses with a mutex: To make the code safe, protect the accesses with a mutex: https://codereview.appspot.com/6948043/diff/6003/doc/articles/race_detector.h... doc/articles/race_detector.html:232: func GetService(name string) net.Addr { s/Get/Lookup/ https://codereview.appspot.com/6948043/diff/6003/doc/articles/race_detector.h... doc/articles/race_detector.html:267: Even such "innocent" data races can lead to hard to debug problems caused by (1) non-atomicity of the memory accesses, (2) interference with compiler optimizations and (3) processor memory access reordering issues. “innocent” https://codereview.appspot.com/6948043/diff/6003/doc/articles/race_detector.h... doc/articles/race_detector.html:271: To fix such data race one can use (aside from <code>chan</code> and <code>sync.Mutex</code>) package <a href="/pkg/sync/atomic"><code>sync/atomic</code></a>, which provides atomic operations on primitive types. <a href="/pkg/sync/atomic"><code>sync/atomic</code></a> functions solve all of the above issues. A typical fix for this race is to use a channel or a mutex. To preserve the lock-free behavior, one can also use the sync/atomic pcakage. https://codereview.appspot.com/6948043/diff/6003/doc/articles/race_detector.h... doc/articles/race_detector.html:294: <h2>Supported Platforms</h2> s/Platforms/Systems/ https://codereview.appspot.com/6948043/diff/6003/doc/articles/race_detector.h... doc/articles/race_detector.html:297: Supported platforms are darwin/amd64, linux/amd64 and windows/amd64. The race detector runs on darwin/amd64, linux/amd64, and windows/amd64. https://codereview.appspot.com/6948043/diff/6003/doc/articles/race_detector.h... doc/articles/race_detector.html:300: <h2>Runtime Overheads</h2> s/Overheads/Overhead/ https://codereview.appspot.com/6948043/diff/6003/doc/articles/race_detector.h... doc/articles/race_detector.html:303: The data race detector significantly increases both memory consumption and execution time. The concrete numbers are highly dependent on the particular program, but some reference numbers would be: memory consumption ~5-10x, execution time ~2-20x. The cost of race detection varies by program, but for a typical program, memory usage may increase by 5-10x and execution time by 2-20x.
Sign in to reply to this message.
On 2012/12/17 01:10:41, rsc wrote: > I tried to tighten much of the text. I am not 100% sure it belongs here. Maybe > it should be a package-level doc comment for runtime/race. Will leave that > decision for adg. I'm happy for it to go here.
Sign in to reply to this message.
https://codereview.appspot.com/6948043/diff/3/doc/articles/race_detector.html File doc/articles/race_detector.html (right): https://codereview.appspot.com/6948043/diff/3/doc/articles/race_detector.html... doc/articles/race_detector.html:6: <h2 id="Introduction">Introduction</h2> On 2012/12/13 19:29:50, minux wrote: > if you don't need to quote a specific section of this article by URL > (like tip.golang.org/doc/articles/race_detector.html#Introduction), > i think you can just drop the id="xx" part. > > it won't affect the toc. Nice to include good anchors, though. I'd leave it.
Sign in to reply to this message.
On 2012/12/17 03:02:17, adg wrote: > https://codereview.appspot.com/6948043/diff/3/doc/articles/race_detector.html > File doc/articles/race_detector.html (right): > > https://codereview.appspot.com/6948043/diff/3/doc/articles/race_detector.html... > doc/articles/race_detector.html:6: <h2 id="Introduction">Introduction</h2> > On 2012/12/13 19:29:50, minux wrote: > > if you don't need to quote a specific section of this article by URL > > (like tip.golang.org/doc/articles/race_detector.html#Introduction), > > i think you can just drop the id="xx" part. > > > > it won't affect the toc. > > Nice to include good anchors, though. I'd leave it. Aha! Here is what they do! Added them back.
Sign in to reply to this message.
All done. Thanks! On 2012/12/17 01:10:41, rsc wrote: > I tried to tighten much of the text. I am not 100% sure it belongs here. Maybe > it should be a package-level doc comment for runtime/race. Will leave that > decision for adg. > > https://codereview.appspot.com/6948043/diff/6003/doc/articles/race_detector.html > File doc/articles/race_detector.html (right): > > https://codereview.appspot.com/6948043/diff/6003/doc/articles/race_detector.h... > doc/articles/race_detector.html:9: Data races are one of the most common and > hardest to debug types of bugs in concurrent systems. A data race occurs when > two goroutines access the same variable without proper synchronization and at > least one of the accesses is write. See the <a href="/ref/mem">The Go Memory > Model</a> for details. > s/without proper synchronization/concurrently/ > > https://codereview.appspot.com/6948043/diff/6003/doc/articles/race_detector.h... > doc/articles/race_detector.html:13: Here is an example of a data race on map > variable that can lead to crashes and memory corruptions: > s/on map variable // > > https://codereview.appspot.com/6948043/diff/6003/doc/articles/race_detector.h... > doc/articles/race_detector.html:35: Fortunately, Go includes a built-in data > race detector. The usage is very simple -- you just need to add -race flag to > the go command: > s/The usage.*/To use it, add the -race flag to the go command: > > https://codereview.appspot.com/6948043/diff/6003/doc/articles/race_detector.h... > doc/articles/race_detector.html:48: When the race detector finds a data race in > the program, it prints an informative report. The report contains stack traces > for conflicting accesses, as well as stacks where the involved goroutines were > created. You may see an example below: > s/an informative/a/ > s/You may see an example below/For example/ > > https://codereview.appspot.com/6948043/diff/6003/doc/articles/race_detector.h... > doc/articles/race_detector.html:89: You can pass some options to the race > detector by means of <code>GORACE</code> environment variable. The format is: > The GORACE environment variable sets race detector options. > The format is: > > https://codereview.appspot.com/6948043/diff/6003/doc/articles/race_detector.h... > doc/articles/race_detector.html:99: <li> log_path: Tells race detector to write > reports to 'log_path.pid' file. The special values are 'stdout' and 'stderr'. > The default is 'stderr'.</li> > log_path (default stderr): The race detector writes its report to a file named > log_path.pid. The special names stdout and stderr cause reports to be written to > standard output and standard error, respectively. > > https://codereview.appspot.com/6948043/diff/6003/doc/articles/race_detector.h... > doc/articles/race_detector.html:100: <li> exitcode: Override exit status of the > process if something was reported. Default value is 66.</li> > exitcode (default 66): The exit status to use when exiting after a detected > race. > > https://codereview.appspot.com/6948043/diff/6003/doc/articles/race_detector.h... > doc/articles/race_detector.html:101: <li> strip_path_prefix: Allows to strip > beginnings of file paths in reports to make them more concise.</li> > strip_path_prefix (default ""): Strip this prefix from all reported file paths, > to make reports more concise. > > https://codereview.appspot.com/6948043/diff/6003/doc/articles/race_detector.h... > doc/articles/race_detector.html:102: <li> history_size: Per-goroutine history > size, controls how many previous memory accesses are remembered per goroutine. > Possible values are [0..7]. history_size=0 amounts to 32K memory accesses. > Each next value doubles the amount of memory accesses, up to history_size=7 that > amounts to 4M memory accesses. The default value is 1 (64K memory accesses). > Try to increase this value when you see "failed to restore the stack" in > reports. However, it can significantly increase memory consumption.</li> > history_size (default 1): The per-goroutine memory access history is 32K * > 2**history_size elements. Increasing this value can avoid a "failed to restore > the stack" error in reports, but at the cost of increased memory usage. > > https://codereview.appspot.com/6948043/diff/6003/doc/articles/race_detector.h... > doc/articles/race_detector.html:115: You may start with just running your tests > under the race detector (<code>go test -race</code>). However sometimes tests > have limited coverage, especially with respect to concurrency. The race > detector finds only races that actually happen in the execution, it can't find > races in code paths that were not executed. So it may be beneficial to run the > whole program built with -race under a realistic workload, frequently it > discovers much more bugs than tests. > To start, run your tests using the race detector (go test -race). > The race detector only finds races that happen at runtime, so it can't find > races > in code paths that are not executed. If your tests have incomplete coverage, > you may find more races by running a binary built with -race under a realistic > workload. > > https://codereview.appspot.com/6948043/diff/6003/doc/articles/race_detector.h... > doc/articles/race_detector.html:121: Here are some examples of typical data > races. All of them can be automatically detected with the race detector. > s/examples of // > s/automatically // > > https://codereview.appspot.com/6948043/diff/6003/doc/articles/race_detector.h... > doc/articles/race_detector.html:141: Closures capture variables by reference > rather than by value, so the reads of the <code>i</code> variable in the > goroutines race with <code>i</code> increment in the loop statement. Such > program typically outputs 55555 instead of expected 01234. The program can be > fixed by explicitly making a copy of the loop counter: > The variable i in the function literal is the same variable used by the loop, so > the read in the goroutine races with the loop increment. (This program typically > prints 55555, not 01234.) The program can be fixed by making a copy of the > variable: > > https://codereview.appspot.com/6948043/diff/6003/doc/articles/race_detector.h... > doc/articles/race_detector.html:191: The fix is simple, one just needs to > introduce new variables in the goroutines (note <code>:=</code>): > The fix is to introduce new ... > > https://codereview.appspot.com/6948043/diff/6003/doc/articles/race_detector.h... > doc/articles/race_detector.html:203: If the following code is called from > several goroutines, it leads to bad races on the <code>services</code> map. > Concurrent reads and writes of a map are not safe: > > https://codereview.appspot.com/6948043/diff/6003/doc/articles/race_detector.h... > doc/articles/race_detector.html:213: func GetService(name string) net.Addr { > s/Get/Lookup/ > > https://codereview.appspot.com/6948043/diff/6003/doc/articles/race_detector.h... > doc/articles/race_detector.html:219: It can be fixed by protecting the accesses > with a mutex: > To make the code safe, protect the accesses with a mutex: > > https://codereview.appspot.com/6948043/diff/6003/doc/articles/race_detector.h... > doc/articles/race_detector.html:232: func GetService(name string) net.Addr { > s/Get/Lookup/ > > https://codereview.appspot.com/6948043/diff/6003/doc/articles/race_detector.h... > doc/articles/race_detector.html:267: Even such "innocent" data races can lead to > hard to debug problems caused by (1) non-atomicity of the memory accesses, (2) > interference with compiler optimizations and (3) processor memory access > reordering issues. > “innocent” > > https://codereview.appspot.com/6948043/diff/6003/doc/articles/race_detector.h... > doc/articles/race_detector.html:271: To fix such data race one can use (aside > from <code>chan</code> and <code>sync.Mutex</code>) package <a > href="/pkg/sync/atomic"><code>sync/atomic</code></a>, which provides atomic > operations on primitive types. <a > href="/pkg/sync/atomic"><code>sync/atomic</code></a> functions solve all of the > above issues. > A typical fix for this race is to use a channel or a mutex. > To preserve the lock-free behavior, one can also use the sync/atomic pcakage. > > https://codereview.appspot.com/6948043/diff/6003/doc/articles/race_detector.h... > doc/articles/race_detector.html:294: <h2>Supported Platforms</h2> > s/Platforms/Systems/ > > https://codereview.appspot.com/6948043/diff/6003/doc/articles/race_detector.h... > doc/articles/race_detector.html:297: Supported platforms are darwin/amd64, > linux/amd64 and windows/amd64. > The race detector runs on darwin/amd64, linux/amd64, and windows/amd64. > > https://codereview.appspot.com/6948043/diff/6003/doc/articles/race_detector.h... > doc/articles/race_detector.html:300: <h2>Runtime Overheads</h2> > s/Overheads/Overhead/ > > https://codereview.appspot.com/6948043/diff/6003/doc/articles/race_detector.h... > doc/articles/race_detector.html:303: The data race detector significantly > increases both memory consumption and execution time. The concrete numbers are > highly dependent on the particular program, but some reference numbers would be: > memory consumption ~5-10x, execution time ~2-20x. > The cost of race detection varies by program, but for a typical program, memory > usage may increase by 5-10x and execution time by 2-20x.
Sign in to reply to this message.
Hello minux.ma@gmail.com, franciscossouza@gmail.com, rsc@golang.org, adg@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
https://codereview.appspot.com/6948043/diff/14001/doc/articles/race_detector.... File doc/articles/race_detector.html (right): https://codereview.appspot.com/6948043/diff/14001/doc/articles/race_detector.... doc/articles/race_detector.html:9: Data races are one of the most common and hardest to debug types of bugs in concurrent systems. A data race occurs when two goroutines access the same variable concurrently and at least one of the accesses is write. See the <a href="/ref/mem">The Go Memory Model</a> for details. s/write/a write/ https://codereview.appspot.com/6948043/diff/14001/doc/articles/race_detector.... doc/articles/race_detector.html:13: Here is an example of a data race that can lead to crashes and memory corruptions: s/corruptions/corruption/ https://codereview.appspot.com/6948043/diff/14001/doc/articles/race_detector.... doc/articles/race_detector.html:35: Fortunately, Go includes a built-in data race detector. To use it, add the -race flag to the go command: <code>-race</code> https://codereview.appspot.com/6948043/diff/14001/doc/articles/race_detector.... doc/articles/race_detector.html:39: $ go test -race mypkg // to test the package align these comments https://codereview.appspot.com/6948043/diff/14001/doc/articles/race_detector.... doc/articles/race_detector.html:89: The GORACE environment variable sets race detector options. The format is: <code>GORACE</code> https://codereview.appspot.com/6948043/diff/14001/doc/articles/race_detector.... doc/articles/race_detector.html:99: <li>log_path (default stderr): The race detector writes its report to a file named <code>log_path</code> <code>stderr</code> etc https://codereview.appspot.com/6948043/diff/14001/doc/articles/race_detector.... doc/articles/race_detector.html:221: var services map[string]net.Addr s/services/service/ https://codereview.appspot.com/6948043/diff/14001/doc/articles/race_detector.... doc/articles/race_detector.html:238: var mu sync.Mutex var ( service map[string]net.Addr serviceMu sync.Mutex ) https://codereview.appspot.com/6948043/diff/14001/doc/articles/race_detector.... doc/articles/race_detector.html:312: The race detector runs on darwin/amd64, linux/amd64, and windows/amd64. <code> around os/arch
Sign in to reply to this message.
Done. PTAL. https://codereview.appspot.com/6948043/diff/14001/doc/articles/race_detector.... File doc/articles/race_detector.html (right): https://codereview.appspot.com/6948043/diff/14001/doc/articles/race_detector.... doc/articles/race_detector.html:99: <li>log_path (default stderr): The race detector writes its report to a file named On 2012/12/19 02:57:26, adg wrote: > <code>log_path</code> > <code>stderr</code> > etc Please check that I correctly guessed etc https://codereview.appspot.com/6948043/diff/17001/doc/articles/race_detector.... File doc/articles/race_detector.html (right): https://codereview.appspot.com/6948043/diff/17001/doc/articles/race_detector.... doc/articles/race_detector.html:126: you may find more races by running a binary built with <code>-race</code> under a realistic Added <code> here as well https://codereview.appspot.com/6948043/diff/17001/doc/articles/race_detector.... doc/articles/race_detector.html:297: atomic.StoreInt64(&w.last, time.Now().UnixNano()) Replaced & with & https://codereview.appspot.com/6948043/diff/17001/doc/articles/race_detector.... doc/articles/race_detector.html:304: if atomic.LoadInt64(&w.last) < time.Now().Add(-10*time.Second).UnixNano() { Replaced & with &
Sign in to reply to this message.
Instead of worrying about all the escaping, it might be better to split the code samples out into Go files, and include them with the "code" template function. See the other articles for inspiration. On 19 Dec 2012 17:42, <dvyukov@google.com> wrote: > > Done. PTAL. > > > > https://codereview.appspot.com/6948043/diff/14001/doc/articles/race_detector.... > File doc/articles/race_detector.html (right): > > https://codereview.appspot.com/6948043/diff/14001/doc/articles/race_detector.... > doc/articles/race_detector.html:99: <li>log_path (default stderr): The > race detector writes its report to a file named > On 2012/12/19 02:57:26, adg wrote: >> >> <code>log_path</code> >> <code>stderr</code> >> etc > > > Please check that I correctly guessed etc > > https://codereview.appspot.com/6948043/diff/17001/doc/articles/race_detector.... > File doc/articles/race_detector.html (right): > > https://codereview.appspot.com/6948043/diff/17001/doc/articles/race_detector.... > doc/articles/race_detector.html:126: you may find more races by running > a binary built with <code>-race</code> under a realistic > Added <code> here as well > > https://codereview.appspot.com/6948043/diff/17001/doc/articles/race_detector.... > doc/articles/race_detector.html:297: atomic.StoreInt64(&w.last, > time.Now().UnixNano()) > Replaced & with & > > https://codereview.appspot.com/6948043/diff/17001/doc/articles/race_detector.... > doc/articles/race_detector.html:304: if atomic.LoadInt64(&w.last) < > time.Now().Add(-10*time.Second).UnixNano() { > Replaced & with & > > https://codereview.appspot.com/6948043/
Sign in to reply to this message.
You don't have to do it but it might end up nicer. :-) On 19 Dec 2012 18:21, "Andrew Gerrand" <adg@golang.org> wrote: > Instead of worrying about all the escaping, it might be better to split > the code samples out into Go files, and include them with the "code" > template function. See the other articles for inspiration. > > On 19 Dec 2012 17:42, <dvyukov@google.com> wrote: > > > > Done. PTAL. > > > > > > > > > https://codereview.appspot.com/6948043/diff/14001/doc/articles/race_detector.... > > File doc/articles/race_detector.html (right): > > > > > https://codereview.appspot.com/6948043/diff/14001/doc/articles/race_detector.... > > doc/articles/race_detector.html:99: <li>log_path (default stderr): The > > race detector writes its report to a file named > > On 2012/12/19 02:57:26, adg wrote: > >> > >> <code>log_path</code> > >> <code>stderr</code> > >> etc > > > > > > Please check that I correctly guessed etc > > > > > https://codereview.appspot.com/6948043/diff/17001/doc/articles/race_detector.... > > File doc/articles/race_detector.html (right): > > > > > https://codereview.appspot.com/6948043/diff/17001/doc/articles/race_detector.... > > doc/articles/race_detector.html:126: you may find more races by running > > a binary built with <code>-race</code> under a realistic > > Added <code> here as well > > > > > https://codereview.appspot.com/6948043/diff/17001/doc/articles/race_detector.... > > doc/articles/race_detector.html:297: atomic.StoreInt64(&w.last, > > time.Now().UnixNano()) > > Replaced & with & > > > > > https://codereview.appspot.com/6948043/diff/17001/doc/articles/race_detector.... > > doc/articles/race_detector.html:304: if atomic.LoadInt64(&w.last) < > > time.Now().Add(-10*time.Second).UnixNano() { > > Replaced & with & > > > > https://codereview.appspot.com/6948043/ >
Sign in to reply to this message.
two more suggestions: 1. please document the race build tag maybe we also need to document that in the go tool docs (perhaps in a separate CL) 2. please list the race detector feature to doc/go1.1.html and link to this article.
Sign in to reply to this message.
It's already documented in the go tool docs, I believe. $ go help build ... -race enable data race detection. Currently supported only on linux/amd64, darwin/amd64 and windows/amd64. ... I'd remove the word "Currently" ;-) On 20 December 2012 21:25, <minux.ma@gmail.com> wrote: > two more suggestions: > 1. please document the race build tag > maybe we also need to document that in the go tool > docs (perhaps in a separate CL) > 2. please list the race detector feature to doc/go1.1.html > and link to this article. > > https://codereview.appspot.**com/6948043/<https://codereview.appspot.com/6948... >
Sign in to reply to this message.
On Friday, December 21, 2012, Andrew Gerrand wrote: > It's already documented in the go tool docs, I believe. > > $ go help build > ... > -race > enable data race detection. > Currently supported only on linux/amd64, > darwin/amd64 and windows/amd64. > ... > maybe my original wording is confusing, i refer to the ability to say this: // +build !race > > > I'd remove the word "Currently" ;-) > > On 20 December 2012 21:25, <minux.ma@gmail.com <javascript:_e({}, 'cvml', > 'minux.ma@gmail.com');>> wrote: > >> two more suggestions: >> 1. please document the race build tag >> maybe we also need to document that in the go tool >> docs (perhaps in a separate CL) >> 2. please list the race detector feature to doc/go1.1.html >> and link to this article. >> >> https://codereview.appspot.**com/6948043/<https://codereview.appspot.com/6948... >> > >
Sign in to reply to this message.
On 2012/12/20 20:24:06, adg1 wrote: > It's already documented in the go tool docs, I believe. > > $ go help build > ... > -race > enable data race detection. > Currently supported only on linux/amd64, > darwin/amd64 and windows/amd64. > ... > > I'd remove the word "Currently" ;-) Done in a separate CL: https://codereview.appspot.com/7006043
Sign in to reply to this message.
On 2012/12/19 07:22:04, adg1 wrote: > You don't have to do it but it might end up nicer. :-) > On 19 Dec 2012 18:21, "Andrew Gerrand" <mailto:adg@golang.org> wrote: > > > Instead of worrying about all the escaping, it might be better to split > > the code samples out into Go files, and include them with the "code" > > template function. See the other articles for inspiration. If you do not feel very strongly about it, I would prefer to commit the first version as-is. Later we can improve it... if needed.
Sign in to reply to this message.
PTAL I've mentioned race build tag and how it can be used. Renamed title "Race Detector" -> "Data Race Detector". Added note to go1.1.html.
Sign in to reply to this message.
https://codereview.appspot.com/6948043/diff/32001/doc/articles/race_detector.... File doc/articles/race_detector.html (right): https://codereview.appspot.com/6948043/diff/32001/doc/articles/race_detector.... doc/articles/race_detector.html:129: // +build !race leave a blank line here or the build tag won't be effective.
Sign in to reply to this message.
PTAL https://codereview.appspot.com/6948043/diff/32001/doc/articles/race_detector.... File doc/articles/race_detector.html (right): https://codereview.appspot.com/6948043/diff/32001/doc/articles/race_detector.... doc/articles/race_detector.html:129: // +build !race On 2012/12/21 18:26:14, minux wrote: > leave a blank line here or the build tag won't be effective. Done.
Sign in to reply to this message.
LGTM
Sign in to reply to this message.
*** Submitted as https://code.google.com/p/go/source/detail?r=f03e22ca5fea *** doc: add race detector manual R=minux.ma, franciscossouza, rsc, adg, adg CC=golang-dev https://codereview.appspot.com/6948043
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
