sync: merge defers in once.doSlow
Closed this issue · 4 comments
What version of Go are you using (go version)?
1.14.0
Does this issue reproduce with the latest release?
Yes
What operating system and processor architecture are you using (go env)?
windows Amd64
In function doSlow as follow:
func (o *Once) doSlow(f func()) {
o.m.Lock()
defer o.m.Unlock()
if o.done == 0 {
defer atomic.StoreUint32(&o.done, 1)
f()
}
}
make better as follow:
func (o *Once) doSlow(f func()) {
o.m.Lock()
defer func() {
if o.done == 0 {
o.done = 1
}
o.m.Unlock()
}()
if o.done == 0 {
f()
}
}
because:
1, merge the two "defer"
2, do not need the atomic.StoreUint32, the "done = 1" is in the lock scope of mutex
make better performance
This isn't correct. There needs to be an atomic store of o.done, because o.done is read inside (*Once).Do. The danger here is that the first thread calls Do, runs f, and then sets o.done=1. Another thread comes along, sees o.done set to 1 (in (*Once).Do, without grabbing the o.lock), and returns immediately. In that case, the second thread is not guaranteed to see all the side effects of f according to the memory model. We need an atomic store and an atomic load to establish a happens-before relationship.
The other optimization here, merging the defers, might make sense. Benchmark it and find out.
I'm not sure this matters in practice. doSlow should normally be executed O(1) times in the lifetime of a sync.Once.
Sure, you could potentially rewrite doSlow as
func (o *Once) doSlow(f func()) {
o.m.Lock()
if o.done != 0 {
o.m.Unlock()
return
}
defer func() {
atomic.StoreUint32(&o.done, 1)
o.m.Unlock()
}()
f()
}but I wouldn't hold my breath on being able to measure the difference.
Especially with the faster defers in 1.14, this seems unlikely to make a difference. But, sure, write a benchmark and see.
Timed out in state WaitingForInfo. Closing.
(I am just a bot, though. Please speak up if this is a mistake or you have the requested information.)