util/time: update comment on Timer to reflect proper usage

[ajwerner]

The comment pre-dated the addition of the sync.Pool.

[cockroach-teamcity]

This change is 

[nvanbenschoten]

I'm worried that we don't actually obey this rule and that we have bugs lingering in this area:

➜  cockroach git:(master) git grep -E 'var \w+ timeutil.Timer'
pkg/cmd/github-pull-request-make/testdata/27595.diff:+  var t timeutil.Timer
pkg/cmd/roachtest/allocator.go: var statsTimer timeutil.Timer
pkg/gossip/gossip.go:           var bootstrapTimer timeutil.Timer
pkg/rpc/context.go:     var heartbeatTimer timeutil.Timer
pkg/server/debug/logspy.go:     var flushTimer timeutil.Timer
pkg/server/node.go:             var timer timeutil.Timer
pkg/server/updates.go:          var timer timeutil.Timer
pkg/sql/conn_executor.go:               var timer timeutil.Timer
pkg/sql/distsqlrun/outbox.go:   var flushTimer timeutil.Timer
pkg/storage/closedts/provider/provider.go:      var t timeutil.Timer
pkg/storage/compactor/compactor.go:                     var timer timeutil.Timer
pkg/storage/raft_transport.go:  var raftIdleTimer timeutil.Timer
pkg/storage/txnwait/txnqueue.go:        var pusheeTxnTimer timeutil.Timer
pkg/util/timeutil/timer.go://  var timer timeutil.Timer

A solution that would allow these stack allocated timers is to add a poolable bool field to timeutil.Timer that we set to true in NewTimer. We could then only release the timer into the pool in Timer.Stop if it is on the heap. What do you think?

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @nvanbenschoten)

[ajwerner]

I don't think that's a bad idea but it may not be necessary. Upon further reflection, I'm not sure these are actually correctness issues and probably I should make the comment less harsh. The docs on sync.Pool imply that it's valid to add values which were not obtained through Get, if the pool is empty Get just calls New() and doesn't interact with the pool at all. As long as the value is never used after Stop it's okay.

I guess the main thing to note is that those timers will always be moved to the heap. I'll update the comment to be less scary.

[nvanbenschoten]

I guess the main thing to note is that those timers will always be moved to the heap.

Is the pooling in Stop the only thing that forces them on to the heap? It would be a shame if the act of trying to avoid allocations is actually causing them.

[ajwerner]

I guess the main thing to note is that those timers will always be moved to the heap.

Is the pooling in Stop the only thing that forces them on to the heap? It would be a shame if the act of trying to avoid allocations is actually causing them.

Fair. I think then I should make the comment less harsh so as to not imply it's a correctness issue. Then in another commit I'll change the var _ timeutil.Timer to call NewTimer() and thus utilize the pool.

[ajwerner]

But yes, that Put in Stop is the cause of the escaping:

$ go build -gcflags=-m ./pkg/cmd/roachtest/ 2>&1 | grep statsTime                                                                             
pkg/cmd/roachtest/allocator.go:221:18: statsTimer escapes to heap
pkg/cmd/roachtest/allocator.go:220:6: moved to heap: statsTimer

$ git diff
diff --git a/pkg/util/timeutil/timer.go b/pkg/util/timeutil/timer.go
index b10179fd7c..4421c2f10e 100644
--- a/pkg/util/timeutil/timer.go
+++ b/pkg/util/timeutil/timer.go
@@ -104,7 +104,7 @@ func (t *Timer) Stop() bool {
                        timeTimerPool.Put(t.timer)
                }
        }
        *t = Timer{}
-       timerPool.Put(t)
+       //timerPool.Put(t)
        return res
 }
$ go build -gcflags=-m ./pkg/cmd/roachtest/ 2>&1 | grep statsTime                                                                             
pkg/cmd/roachtest/allocator.go:221:18: waitForRebalance statsTimer does not escape

[ajwerner]

This is RFAL.

I added a linter that probably wasn't necessary and in some cases we'll pull a timer used for the life of a process out of a sync pool but that's fine.

Reviewable status: complete! 0 of 0 LGTMs obtained

[nvanbenschoten]

Reviewed 1 of 1 files at r1, 4 of 4 files at r2, 15 of 15 files at r3.
Reviewable status: complete! 1 of 0 LGTMs obtained

[ajwerner]

I legitimately have no idea why I didn’t merge this at the time

[andreimatei]

but this doesn't help with the bug #61373 is fixing (Reset and Stop after Stop), does it?

Reviewable status: complete! 1 of 0 LGTMs obtained

[ajwerner]

cc @yuzefovich want to get this merged? It doesn't solve the reuse problem, but maybe it makes it more likely folks will read the constructor doc comments? I guess the real question is why do we need the sync.Pool on the outer timer struct -- it's internally a pointer and bool that seems like in the common case shouldn't escape. Maybe there's a place where that allocation mattered, and a more tightly scoped sync pool should be put there.