time: stop requiring Timer/Ticker.Stop for prompt GC

[rsc]

After almost a decade, I have finally implemented #8898, special-casing the channels used for timers (or maybe the timers used for channels) so that the timers are not in the timer heap when there are no channel operations pending on them. This means that the channels and timers can be GC'ed once they are no longer referenced, without having to wait for a timer to expire or explicitly Stop the timer. (By timer here I mean the data structure used by time.After, time.NewTimer, and time.NewTicker.)

This raises a question: do we want to guarantee that all future versions of Go will provide this behavior, so that code can rely on not needing to call the Stop methods? I think we probably should make that guarantee.

I propose we land my CL and document that After can be used without concern for GC and that NewTimer and NewTicker can be used without concern for deferring Stop just for GC. At the moment the doc comment for After describes the problem and basically says "don't use this function a lot". If we accept this proposal, we can remove that text. NewTimer and NewTicker do not mention needing to call Stop, although it is implied by After's doc comment. If we accept this proposal, we can document that Stop is not necessary for GC, so that people can stop calling Stop.

[gopherbot]

Change https://go.dev/cl/512355 mentions this issue: time: garbage collect unstopped Tickers and Timers

[bcmills]

This seems like it would require other Go implementations (such as gccgo and TinyGo) to implement those same special cases. Are we sure that's viable for them?

[bcmills]

That said, I think we basically have to do this to avoid the portability pitfall I mentioned in #8898 (comment). I don't want to be in a long-term state where it's ok to omit the Stop only if you know that your code will never be compiled with gccgo or TinyGo.

[rsc]

I can't see why it wouldn't be a viable change for gccgo or TinyGo. The change is not terribly complex: it just requires recognizing timers supporting channels and turning them on/off around blocking operations on those channels. See https://go.dev/cl/512355. Also, we have never held Go back before for secondary implementations to catch up, and I am reluctant to start doing that now.

In the second comment, I am not sure what "this" is in "basically have to do this".

[bcmills]

In the second comment, I am not sure what "this" is in "basically have to do this".

This proposal. That is: since the mainline implementation no longer requires a Stop, we need to require secondary implementations to provide the same behavior in order to avoid arguments about whether a given Stop is necessary (for portability) or redundant in code outside of the standard library.

[rsc]

I see. I completely agree that if the CL is submitted then we have to accept the proposal. This proposal is really "should we submit the CL?"

[rsc]

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

[ChrisHines]

@rsc If I understand the implementation correctly a tight for-select loop including a timer channel will cause frequent resorting of timer heaps. Is that a performance concern? Has it been measured?

[rsc]

@rsc If I understand the implementation correctly a tight for-select loop including a timer channel will cause frequent resorting of timer heaps. Is that a performance concern? Has it been measured?

The timer heap manipulation only happens when the other channels are not ready. So how tight can the loop really be if it's constantly finding no channels ready and blocking on each iteration? I haven't figured out a good way to measure that.

If you had a loop with a 1ns timer, that would get handled by a special polling check at the start of the channel operation and wouldn't involve the timer heap at all, so it should run faster than before.

There may be a slower path here but I can't figure out how to provoke it if so.

[rsc]

https://go.dev/cl/512355 is the code if anyone wants to try any benchmark attempts.

[ChrisHines]

The timer heap manipulation only happens when the other channels are not ready.

That's a key piece of the implementation I hadn't realized. Thanks for the explanation.

[bcmills]

The timer heap manipulation only happens when the other channels are not ready.

Does that optimization end up biasing select statements involving timer channels? (If so, is that degree of bias going to be a problem?)

[ianlancetaylor]

It shouldn't bias it. If the timer channel is ready to send a value, then queues one up, and participates in the select statement as a ready channel. If the timer channel is not ready, then it doesn't. We only start worrying about the timer heap when no channels are ready.

[aclements]

I spoke with the compiler/runtime team and nobody had objections to requiring the timer implementation to allow prompt GC.

@aykevl and @deadprogram, I wanted to get your take on this proposal since TinyGo is the main other Go runtime and would have to implement this optimization.

[rsc]

I downloaded the TinyGo distribution and looked to see how time.After was implemented. The answer appears to be that it is not implemented at all. It looks like if you use TinyGo you can have time.Sleep.

% grep -Rn time.After .
./src/net/conn_test.go:68:	timer := time.AfterFunc(time.Minute, func() {
./src/net/pipe.go:56:		d.timer = time.AfterFunc(dur, func() {
./src/reflect/all_test.go:1511:		{time.After(1), false},
./src/reflect/all_test.go:2020:			time.AfterFunc(pause, helper)
./src/reflect/all_test.go:2272:	timeout := time.After(5 * time.Second)
./src/reflect/all_test.go:6202:		case <-time.After(5 * time.Second):
% grep -Rn 'func After' .
% 
% grep -Rn 'time\.Sleep' .
./src/net/conn_test.go:270:		time.Sleep(100 * time.Millisecond)
./src/net/conn_test.go:339:		time.Sleep(100 * time.Millisecond)
./src/reflect/all_test.go:2117:		time.Sleep(1 * time.Second)
./src/runtime/scheduler_none.go:5://go:linkname sleep time.Sleep
./src/runtime/scheduler_any.go:9://go:linkname sleep time.Sleep
./src/examples/blinkm/blinkm.go:37:		time.Sleep(100 * time.Millisecond)
./src/examples/pininterrupt/pininterrupt.go:50:		time.Sleep(time.Hour)
./src/examples/blinky2/blinky2.go:24:		time.Sleep(time.Millisecond * 1000)
./src/examples/blinky2/blinky2.go:28:		time.Sleep(time.Millisecond * 1000)
./src/examples/blinky2/blinky2.go:38:		time.Sleep(time.Millisecond * 420)
...
% 

[aclements]

If I'm not mistaken, TinyGo just uses the time package from the main Go std. I believe this is their implementation of the runtime functions that underlie the time package: https://github.com/tinygo-org/tinygo/blob/v0.28.1/src/runtime/time.go and https://github.com/tinygo-org/tinygo/blob/v0.28.1/src/runtime/scheduler.go#L119

[thepudds]

CC @dgryski (who has been doing some work on TinyGo)

[dgryski]

TinyGo does use the standard library (as much as we can), and we do support time.After and friends (again, as much as we can within the confines of a non-preemptive scheduler). I'm fine to add this to our backlog. There are already a number of runtime optimizations that Go has that TinyGo doesn't have yet, so this will just be one more to document.

[rsc]

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

[bcmills]

Would this also address #37196, or is that mostly orthogonal?

[rsc]

I don't think it helps #37196 because in the worst case there is a long-running select already waiting on the channel, in which case the timer is in the heap and the behavior matches the current behavior.

[rsc]

On the other hand the implementation changes for this issue definitely overlap with the already-accepted #37196, so I will take a look at whether I can do that while everything is paged in.

[rsc]

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

[aykevl]

Finally got to this issue (holiday and all that).

So right now we use the standard library time package and implement the runtime side of it in our custom runtime. That implementation isn't great: it basically calls the provided function from inside the scheduler so any blocking call is probably going to mess things up. Consider it a work in progress.

Regarding the actual issue: it appears to require a bunch of special casing for timer channels. Maybe we'll implement that some day, but I can't promise that because it grows the runtime size even when timers aren't used and I very much would like to keep the runtime size as low as reasonably possible (while implementing the Go spec, of course).

(At the same time, the current design of the TinyGo runtime should garbage collect deadlocked goroutines as a side effect of how the scheduler works. I didn't actually test this though. AFAIK the main Go runtime doesn't do this. Just to show a different design results in different things being trivial or difficult).