Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(528)

Issue 4641066: code review 4641066: sync: add fast path to Once (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
12 years, 9 months ago by dvyukov
Modified:
12 years, 9 months ago
Reviewers:
CC:
bradfitz, rsc, golang-dev
Visibility:
Public.

Description

sync: add fast path to Once The implementation does not grab the lock, if Once is already initalized. Benchmark results on HP Z600 (2 x Xeon E5620, 8 HT cores, 2.40GHz) are as follows: benchmark old ns/op new ns/op delta sync_test.BenchmarkOnce 187.00 14.00 -92.51% sync_test.BenchmarkOnce-2 909.00 21.40 -97.65% sync_test.BenchmarkOnce-4 3684.00 20.90 -99.43% sync_test.BenchmarkOnce-8 5987.00 23.00 -99.62% sync_test.BenchmarkOnce-16 5051.00 21.60 -99.57%

Patch Set 1 #

Patch Set 2 : diff -r 6f1145ee588d https://go.googlecode.com/hg/ #

Patch Set 3 : diff -r 6f1145ee588d https://go.googlecode.com/hg/ #

Patch Set 4 : diff -r 9e53a1312e25 https://go.googlecode.com/hg/ #

Total comments: 1

Patch Set 5 : diff -r 9e53a1312e25 https://go.googlecode.com/hg/ #

Unified diffs Side-by-side diffs Delta from patch set Stats (+11 lines, -3 lines) Patch
M src/pkg/sync/once.go View 1 2 3 2 chunks +11 lines, -3 lines 0 comments Download

Messages

Total messages: 14
dvyukov
Hello golang-dev@googlegroups.com (cc: dvyukov@google.com, golang-dev@googlegroups.com), I'd like you to review this change to https://go.googlecode.com/hg/
12 years, 9 months ago (2011-06-22 16:58:49 UTC) #1
bradfitz
What are the units on your graph? And the legend has 3 colors (two of ...
12 years, 9 months ago (2011-06-22 17:14:19 UTC) #2
dvyukov
On 2011/06/22 17:14:19, bradfitz wrote: > What are the units on your graph? Number of ...
12 years, 9 months ago (2011-06-22 17:20:08 UTC) #3
rsc
These performance improvements are great but I'd like to be able to check them in ...
12 years, 9 months ago (2011-06-22 17:24:15 UTC) #4
dvyukov
On 2011/06/22 17:24:15, rsc wrote: > These performance improvements are great but I'd > like ...
12 years, 9 months ago (2011-06-22 17:26:16 UTC) #5
dvyukov
Hello golang-dev@googlegroups.com, bradfitz@golang.org, rsc@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
12 years, 9 months ago (2011-06-28 10:15:03 UTC) #6
rsc
The graph is great, but people may have the repository locally and not have access ...
12 years, 9 months ago (2011-06-28 12:11:28 UTC) #7
rsc
The graph is great, but people may have the repository locally and not have access ...
12 years, 9 months ago (2011-06-28 12:11:47 UTC) #8
dvyukov
On 2011/06/28 12:11:47, rsc wrote: > The graph is great, but people may have the ...
12 years, 9 months ago (2011-06-28 12:49:53 UTC) #9
rsc
> On HP Z600 (2 x Xeon E5620, 8 HT cores, 2.40GHz): > ... Looks ...
12 years, 9 months ago (2011-06-28 13:05:35 UTC) #10
dvyukov
Hello bradfitz@golang.org, rsc@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
12 years, 9 months ago (2011-06-28 13:17:17 UTC) #11
rsc
*** Submitted as http://code.google.com/p/go/source/detail?r=607e0f74161f *** sync: add fast path to Once The implementation does not ...
12 years, 9 months ago (2011-06-28 13:43:04 UTC) #12
rsc
LGTM
12 years, 9 months ago (2011-06-28 13:43:11 UTC) #13
dvyukov
12 years, 9 months ago (2011-06-28 14:30:24 UTC) #14
On 2011/06/28 12:11:28, rsc wrote:
> The graph is great, but people may have the repository
> locally and not have access to the graph.  Could you also
> run the benchmark with -cpu=1,2,4,8, both old and new,
> and add the benchcmp output into the CL description with
> hg change?
> 
> In the CL description, sync: add fast path to Once
> 
> http://codereview.appspot.com/4641066/diff/10001/src/pkg/sync/once.go
> File src/pkg/sync/once.go (right):
> 
>
http://codereview.appspot.com/4641066/diff/10001/src/pkg/sync/once.go#newcode41
> src/pkg/sync/once.go:41: atomic.CompareAndSwapInt32(&o.done, 0, 1)
> Can this be
> o.done = 1
> ?
> 
> If not, why not?

No, it can't.
It must be store-release (atomic_store(memory_order_release)) to prevent user
initialization code (f()) from sinking below store to o.done. Similarly, load of
o.done on fast path must be load-acquire (that's why it's not plain load).
It may work on IA-32/Intel64 with plain memory accesses (at least until the
compiler is sufficiently aggressive), but it won't work no ARM.
In either case it's better to use special constructs for synchronization rather
than plain memory accesses due to several reasons (correctness aside):
1. It makes code *MUCH* better self-documented.
2. It makes synchronization-related code search much simpler.
3. It helps tools to not produce false positive race reports.
4. It helps tools to better test the code.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b