| Age | Commit message (Collapse) | Author |
|---|
|
This CL makes two changes to reduce the predictability
with which bubbled timers fire.
When asynctimerchan=0 (the default), regular timers with an associated
channel are only added to a timer heap when some channel operation
is blocked on that channel. This allows us to garbage collect
unreferenced, unstopped timers. Timers in a synctest bubble, in
contrast, are always added to the bubble's timer heap.
This CL changes bubbled timers with a channel to be handled the
same as unbubbled ones, adding them to the bubble's timer heap only
when some channel operation is blocked on the timer's channel.
This permits unstopped bubbled timers to be garbage collected,
but more importantly it makes all timers past their deadline
behave identically, regardless of whether they are in a bubble.
This CL also changes timer scheduling to execute bubbled timers
immediately when possible rather than adding them to a heap.
Timers in a bubble's heap are executed when the bubble is idle.
Executing timers immediately avoids creating a predictable
order of execution.
For #73850
Fixes #73934
Change-Id: If82e441546408f780f6af6fb7f6e416d3160295d
Reviewed-on: https://go-review.googlesource.com/c/go/+/678075
Auto-Submit: Damien Neil <dneil@google.com>
Reviewed-by: Michael Pratt <mpratt@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
|
|
In synctest bubbles, fire timers scheduled for the same instant
in a randomized order.
Pending timers are added to a heap ordered by the timer's wakeup time.
Add a per-timer random value, set when the timer is added to a heap,
to break ties between timers scheduled for the same instant.
Only inject this randomness in synctest bubbles. We could do so
for all timers at the cost of one cheaprand call per timer,
but given that it's effectively impossible to create two timers
scheduled for the same instant outside of a fake-time environment,
don't bother.
Fixes #73876
For #73850
Change-Id: Ie96c86a816f548d4c31e4e014bf9293639155bd4
Reviewed-on: https://go-review.googlesource.com/c/go/+/677276
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Damien Neil <dneil@google.com>
Reviewed-by: Michael Pratt <mpratt@google.com>
|
|
We've settled on calling the group of goroutines started by
synctest.Run a "bubble". At the time the runtime implementation
was written, I was still calling this a "group". Update the code
to match the current terminology.
Change-Id: I31b757f31d804b5d5f9564c182627030a9532f4a
Reviewed-on: https://go-review.googlesource.com/c/go/+/670135
Reviewed-by: Michael Pratt <mpratt@google.com>
Auto-Submit: Damien Neil <dneil@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
|
|
When an AfterFunc executes in a synctest bubble, there is a series of
happens-before relationships:
1. The AfterFunc is created.
2. The AfterFunc goroutine executes.
3. The AfterFunc goroutine returns.
4. A subsequent synctest.Wait call returns.
We were failing to correctly establish the happens-before relationship
between the AfterFunc goroutine and the AfterFunc itself being created.
When an AfterFunc executes, the G running the timer temporarily switches
to the timer heap's racectx. It then calls time.goFunc, which starts a
new goroutine to execute the timer. time.goFunc relies on the new goroutine
inheriting the racectx of the G running the timer.
Normal, non-synctest timers, execute with m.curg == nil, which causes
new goroutines to inherit the g0 racectx. We were running synctest
timers with m.curg set (to the G executing synctest.Run), so the new
AfterFunc goroutine was created using m.curg's racectx. This resulted
in us not properly establishing the happens-before relationship between
AfterFunc being called and the AfterFunc goroutine starting.
Fix this by setting m.curg to nil while executing timers.
As one additional fix, when waking a blocked bubble, wake the root
goroutine rather than a goroutine blocked in Wait if there is a
timer that can fire.
Fixes #72750
Change-Id: I2b2d6b0f17f64649409adc93c2603f720494af89
Reviewed-on: https://go-review.googlesource.com/c/go/+/658595
Auto-Submit: Damien Neil <dneil@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Michael Pratt <mpratt@google.com>
|
|
Don't include a monotonic time in time.Times created inside
a bubble, to avoid the confusion of different Times using
different monotonic clock epochs.
For #67434
goos: darwin
goarch: arm64
pkg: time
cpu: Apple M1 Pro
│ /tmp/bench.0 │ /tmp/bench.1 │
│ sec/op │ sec/op vs base │
Since-10 18.42n ± 2% 18.68n ± 1% ~ (p=0.101 n=10)
Until-10 18.28n ± 2% 18.46n ± 2% +0.98% (p=0.009 n=10)
geomean 18.35n 18.57n +1.20%
Change-Id: Iaf1b80d0a4df52139c5b80d4bde4410ef8a49f2f
Reviewed-on: https://go-review.googlesource.com/c/go/+/657415
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Damien Neil <dneil@google.com>
Reviewed-by: Michael Pratt <mpratt@google.com>
|
|
When reading from time.Timer.C for an expired timer using
a fake clock (in a synctest bubble), the timer will not
be in a heap. Avoid a spurious panic claiming the timer
moved between synctest bubbles.
Drop the panic when a bubbled goroutine reads from a
non-bubbled timer channel: We allow bubbled goroutines
to access non-bubbled channels in general.
Fixes #70741
Change-Id: I27005e46f4d0067cc6846d234d22766d2e05d163
Reviewed-on: https://go-review.googlesource.com/c/go/+/634955
Auto-Submit: Damien Neil <dneil@google.com>
Reviewed-by: Michael Pratt <mpratt@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
|
|
Add an internal (for now) implementation of testing/synctest.
The synctest.Run function executes a tree of goroutines in an
isolated environment using a fake clock. The synctest.Wait function
allows a test to wait for all other goroutines within the test
to reach a blocking point.
For #67434
For #69687
Change-Id: Icb39e54c54cece96517e58ef9cfb18bf68506cfc
Reviewed-on: https://go-review.googlesource.com/c/go/+/591997
Reviewed-by: Michael Pratt <mpratt@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
|
|
This change switches isSending to be an atomic.Int32 instead of an
atomic.Uint8. The Int32 version is managed as a counter, which is
something that we couldn't do with Uint8 without adding a new intrinsic
which may not be available on all architectures.
That is, instead of only being able to support 8 concurrent timer
firings on the same timer because we only have 8 independent bits to set
for each concurrent timer firing, we can now have 2^31-1 concurrent
timer firings before running into any issues. Like the fact that each
bit-set was matched with a clear, here we match increments with
decrements to indicate that we're in the "sending on a channel" critical
section in the timer code, so we can report the correct result back on
Stop or Reset.
We choose an Int32 instead of a Uint32 because it's easier to check for
obviously bad values (negative values are always bad) and 2^31-1
concurrent timer firings should be enough for anyone.
Previously, we avoided anything bigger than a Uint8 because we could
pack it into some padding in the runtime.timer struct. But it turns out
that the type that actually matters, runtime.timeTimer, is exactly 96
bytes in size. This means its in the next size class up in the 112 byte
size class because of an allocation header. We thus have some free space
to work with. This change increases the size of this struct from 96
bytes to 104 bytes.
(I'm not sure if runtime.timer is often allocated directly, but if it
is, we get lucky in the same way too. It's exactly 80 bytes in size,
which means its in the 96-byte size class, leaving us with some space to
work with.)
Fixes #69969.
Related to #69880 and #69312.
Change-Id: I9fd59cb6a69365c62971d1f225490a65c58f3e77
Cq-Include-Trybots: luci.golang.try:gotip-linux-amd64-longtest
Reviewed-on: https://go-review.googlesource.com/c/go/+/621616
Reviewed-by: Ian Lance Taylor <iant@google.com>
Auto-Submit: Michael Knyszek <mknyszek@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
|
|
The Ticker Stop and Reset methods don't report a value,
so we don't need to track whether they are interrupting a send.
This includes a test that used to fail about 2% of the time on
my laptop when run under x/tools/cmd/stress.
Change-Id: Ic6d14b344594149dd3c24b37bbe4e42e83f9a9ad
Reviewed-on: https://go-review.googlesource.com/c/go/+/620136
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Auto-Submit: Michael Knyszek <mknyszek@google.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
|
|
I've done some more testing of the new isSending field.
I'm not able to get more than 2 bits set. That said,
with this change it's significantly less likely to have even
2 bits set. The idea here is to clear the bit before possibly
locking the channel we are sending the value on, thus avoiding
some delay and some serialization.
For #69312
Change-Id: I8b5f167f162bbcbcbf7ea47305967f349b62b0f4
Reviewed-on: https://go-review.googlesource.com/c/go/+/617497
Reviewed-by: Ian Lance Taylor <iant@google.com>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
Commit-Queue: Ian Lance Taylor <iant@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>
|
|
The timer code is careful to ensure that if stop/reset is called
while a timer is being run, we cancel the run. However, the code
failed to ensure that in that case stop/reset returned true,
meaning that the timer had been stopped. In the racing case
stop/reset could see that t.when had been set to zero,
and return false, even though the timer had not and never would fire.
Fix this by tracking whether a timer run is in progress,
and using that to reliably detect that the run was cancelled,
meaning that stop/reset should return true.
Fixes #69312
Change-Id: I78e870063eb96650638f12c056e32c931417c84a
Reviewed-on: https://go-review.googlesource.com/c/go/+/611496
Reviewed-by: David Chase <drchase@google.com>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Ian Lance Taylor <iant@golang.org>
|
|
Change-Id: Id0fb180a2d7910cdff7f4ab7154d9ceeb2f1cb71
GitHub-Last-Rev: 03658dd9747684f69ea238a8d1d7f7d15446099e
GitHub-Pull-Request: golang/go#68709
Reviewed-on: https://go-review.googlesource.com/c/go/+/602675
Reviewed-by: Ian Lance Taylor <iant@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>
|
|
Cleanup and friction reduction
For #65355.
Change-Id: Ia14c9dc584a529a35b97801dd3e95b9acc99a511
Reviewed-on: https://go-review.googlesource.com/c/go/+/600436
Reviewed-by: Keith Randall <khr@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Keith Randall <khr@golang.org>
|
|
No effect on benchmarks, but the code is clearer.
goos: linux
goarch: amd64
pkg: time
cpu: AMD Ryzen 9 7950X 16-Core Processor
│ s7base.txt │ s7.txt │
│ sec/op │ sec/op vs base │
AdjustTimers10000-32 195.9µ ± 13% 198.1µ ± 2% ~ (p=0.436 n=10)
AdjustTimers10000SingleThread-32 1.573m ± 13% 1.566m ± 10% ~ (p=0.739 n=10)
AdjustTimers10000NoReset-32 170.6µ ± 1% 170.4µ ± 1% ~ (p=0.912 n=10)
AdjustTimers10000NoSleep-32 183.9µ ± 2% 181.4µ ± 2% -1.39% (p=0.045 n=10)
AdjustTimers10000NoResetNoSleep-32 151.3µ ± 1% 150.0µ ± 1% -0.90% (p=0.007 n=10)
goos: darwin
goarch: arm64
pkg: time
cpu: Apple M3 Pro
│ m3base.txt │ m3.txt │
│ sec/op │ sec/op vs base │
AdjustTimers10000-12 234.2µ ± 1% 234.5µ ± 1% ~ (p=0.796 n=10)
AdjustTimers10000SingleThread-12 1.191m ± 1% 1.272m ± 1% +6.81% (p=0.000 n=10)
AdjustTimers10000NoReset-12 239.6µ ± 2% 236.3µ ± 9% ~ (p=0.971 n=10)
AdjustTimers10000NoSleep-12 223.3µ ± 2% 221.4µ ± 3% ~ (p=0.579 n=10)
AdjustTimers10000NoResetNoSleep-12 209.2µ ± 2% 209.0µ ± 4% ~ (p=0.796 n=10)
Change-Id: Id48aa893235d652814b7fa4605037f09b0b4d73b
Reviewed-on: https://go-review.googlesource.com/c/go/+/574897
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
|
|
This CL changes the timer heap from a slice of timers to a slice
of {timer, when} pairs, moving timer.whenHeap into the heap
backing store itself. This makes the ownership clearer (t.whenHeap
was always protected by the heap lock, not the timer's lock)
and also avoids an indirection when doing heap operations,
which look at the when field quite a lot.
goos: linux
goarch: amd64
pkg: time
cpu: AMD Ryzen 9 7950X 16-Core Processor
│ s7base.txt │ s7.txt │
│ sec/op │ sec/op vs base │
AdjustTimers10000-32 244.1µ ± 5% 195.9µ ± 13% -19.76% (p=0.001 n=10)
AdjustTimers10000SingleThread-32 1.674m ± 6% 1.573m ± 13% -6.03% (p=0.001 n=10)
AdjustTimers10000NoReset-32 194.0µ ± 2% 170.6µ ± 1% -12.06% (p=0.000 n=10)
AdjustTimers10000NoSleep-32 223.0µ ± 7% 183.9µ ± 2% -17.54% (p=0.000 n=10)
AdjustTimers10000NoResetNoSleep-32 181.1µ ± 1% 151.3µ ± 1% -16.45% (p=0.000 n=10)
goos: darwin
goarch: arm64
pkg: time
cpu: Apple M3 Pro
│ m3base.txt │ m3.txt │
│ sec/op │ sec/op vs base │
AdjustTimers10000-12 266.6µ ± 2% 234.2µ ± 1% -12.16% (p=0.000 n=10)
AdjustTimers10000SingleThread-12 1.194m ± 0% 1.191m ± 1% -0.33% (p=0.029 n=10)
AdjustTimers10000NoReset-12 260.9µ ± 2% 239.6µ ± 2% -8.15% (p=0.000 n=10)
AdjustTimers10000NoSleep-12 247.8µ ± 2% 223.3µ ± 2% -9.90% (p=0.000 n=10)
AdjustTimers10000NoResetNoSleep-12 231.7µ ± 2% 209.2µ ± 2% -9.68% (p=0.000 n=10)
Change-Id: I546f077068476d7a7b855889419b292525fb2bc5
Reviewed-on: https://go-review.googlesource.com/c/go/+/574896
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
|
|
There is a short window when timers.adjust could miss a
timer update. Close that window. Does not change benchmark.
goos: linux
goarch: amd64
pkg: time
cpu: AMD Ryzen 9 7950X 16-Core Processor
│ s7base.txt │ s7.txt │
│ sec/op │ sec/op vs base │
AdjustTimers10000-32 239.9µ ± 5% 237.6µ ± 8% ~ (p=0.631 n=10)
AdjustTimers10000SingleThread-32 1.686m ± 8% 1.710m ± 5% ~ (p=0.481 n=10)
AdjustTimers10000NoReset-32 194.1µ ± 1% 190.8µ ± 2% -1.69% (p=0.023 n=10)
AdjustTimers10000NoSleep-32 226.2µ ± 3% 222.9µ ± 3% ~ (p=0.143 n=10)
AdjustTimers10000NoResetNoSleep-32 182.9µ ± 1% 180.9µ ± 2% ~ (p=0.165 n=10)
goos: darwin
goarch: arm64
pkg: time
cpu: Apple M3 Pro
│ m3base.txt │ m3.txt │
│ sec/op │ sec/op vs base │
AdjustTimers10000-12 269.3µ ± 2% 267.0µ ± 2% ~ (p=0.529 n=10)
AdjustTimers10000SingleThread-12 1.176m ± 1% 1.213m ± 1% +3.15% (p=0.000 n=10)
AdjustTimers10000NoReset-12 262.6µ ± 2% 261.4µ ± 3% ~ (p=0.123 n=10)
AdjustTimers10000NoSleep-12 247.8µ ± 1% 246.5µ ± 1% ~ (p=0.393 n=10)
AdjustTimers10000NoResetNoSleep-12 231.0µ ± 1% 232.3µ ± 1% ~ (p=0.684 n=10)
Change-Id: Ifdfcdd5a25046027912a8b306644bde7ec2d3214
Reviewed-on: https://go-review.googlesource.com/c/go/+/574741
Auto-Submit: Russ Cox <rsc@golang.org>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
|
|
This new logic more closely mimics what we did before my CL stack.
I had reasoned that certainly
ts.adjust(now, force=true)
ts.run(now)
would be faster than
ts.adjust(now, force=false)
ts.run(now)
ts.adjust(now, force=true)
But certainty is just an emotion, and that turns out not to be the case.
I don't really understand why the second sequence is faster,
but it definitely is, so put it back.
goos: linux
goarch: amd64
pkg: time
cpu: AMD Ryzen 9 7950X 16-Core Processor
│ s7base.txt │ s7.txt │
│ sec/op │ sec/op vs base │
AdjustTimers10000-32 263.3µ ± 4% 239.9µ ± 5% -8.87% (p=0.000 n=10)
AdjustTimers10000SingleThread-32 1.742m ± 3% 1.686m ± 8% ~ (p=0.105 n=10)
AdjustTimers10000NoReset-32 192.2µ ± 2% 194.1µ ± 1% +1.00% (p=0.009 n=10)
AdjustTimers10000NoSleep-32 237.0µ ± 2% 226.2µ ± 3% -4.55% (p=0.001 n=10)
AdjustTimers10000NoResetNoSleep-32 185.2µ ± 1% 182.9µ ± 1% -1.23% (p=0.003 n=10)
goos: darwin
goarch: arm64
pkg: time
cpu: Apple M3 Pro
│ m3base.txt │ m3.txt │
│ sec/op │ sec/op vs base │
AdjustTimers10000-12 272.6µ ± 3% 269.3µ ± 2% ~ (p=0.063 n=10)
AdjustTimers10000SingleThread-12 1.126m ± 1% 1.176m ± 1% +4.42% (p=0.000 n=10)
AdjustTimers10000NoReset-12 255.1µ ± 2% 262.6µ ± 2% +2.96% (p=0.000 n=10)
AdjustTimers10000NoSleep-12 250.2µ ± 2% 247.8µ ± 1% ~ (p=0.063 n=10)
AdjustTimers10000NoResetNoSleep-12 230.3µ ± 1% 231.0µ ± 1% ~ (p=0.280 n=10)
Change-Id: I67b5765f97dfca0142ee38e15a9904b520f51e83
Reviewed-on: https://go-review.googlesource.com/c/go/+/574740
Auto-Submit: Russ Cox <rsc@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
|
|
goos: linux
goarch: amd64
pkg: time
cpu: AMD Ryzen 9 7950X 16-Core Processor
│ s7base.txt │ s7.txt │
│ sec/op │ sec/op vs base │
AdjustTimers10000-32 291.4µ ± 5% 263.3µ ± 4% -9.64% (p=0.000 n=10)
AdjustTimers10000SingleThread-32 1.728m ± 5% 1.742m ± 3% ~ (p=0.796 n=10)
AdjustTimers10000NoReset-32 253.1µ ± 6% 192.2µ ± 2% -24.07% (p=0.000 n=10)
AdjustTimers10000NoSleep-32 277.3µ ± 3% 237.0µ ± 2% -14.54% (p=0.000 n=10)
AdjustTimers10000NoResetNoSleep-32 241.4µ ± 2% 185.2µ ± 1% -23.30% (p=0.000 n=10)
goos: darwin
goarch: arm64
pkg: time
cpu: Apple M3 Pro
│ m3base.txt │ m3.txt │
│ sec/op │ sec/op vs base │
AdjustTimers10000-12 288.1µ ± 1% 272.6µ ± 3% -5.38% (p=0.000 n=10)
AdjustTimers10000SingleThread-12 1.195m ± 1% 1.126m ± 1% -5.74% (p=0.000 n=10)
AdjustTimers10000NoReset-12 280.8µ ± 1% 255.1µ ± 2% -9.14% (p=0.000 n=10)
AdjustTimers10000NoSleep-12 292.5µ ± 1% 250.2µ ± 2% -14.47% (p=0.000 n=10)
AdjustTimers10000NoResetNoSleep-12 279.8µ ± 2% 230.3µ ± 1% -17.69% (p=0.000 n=10)
Change-Id: I36edb40ee2cd11ab44d20bff045fa77609dca648
Reviewed-on: https://go-review.googlesource.com/c/go/+/574739
Reviewed-by: Ian Lance Taylor <iant@google.com>
Auto-Submit: Russ Cox <rsc@golang.org>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
|
|
goos: linux
goarch: amd64
pkg: time
cpu: AMD Ryzen 9 7950X 16-Core Processor
│ s7base.txt │ s7.txt │
│ sec/op │ sec/op vs base │
AdjustTimers10000-32 315.1µ ± 4% 291.4µ ± 5% -7.52% (p=0.001 n=10)
AdjustTimers10000SingleThread-32 1.728m ± 6% 1.728m ± 5% ~ (p=0.971 n=10)
AdjustTimers10000NoReset-32 263.0µ ± 3% 253.1µ ± 6% -3.75% (p=0.023 n=10)
AdjustTimers10000NoSleep-32 306.8µ ± 5% 277.3µ ± 3% -9.62% (p=0.000 n=10)
AdjustTimers10000NoResetNoSleep-32 245.3µ ± 3% 241.4µ ± 2% ~ (p=0.529 n=10)
goos: darwin
goarch: arm64
pkg: time
cpu: Apple M3 Pro
│ m3base.txt │ m3.txt │
│ sec/op │ sec/op vs base │
AdjustTimers10000-12 385.5µ ± 3% 288.1µ ± 1% -25.26% (p=0.000 n=10)
AdjustTimers10000SingleThread-12 1.213m ± 1% 1.195m ± 1% -1.45% (p=0.001 n=10)
AdjustTimers10000NoReset-12 346.4µ ± 5% 280.8µ ± 1% -18.94% (p=0.000 n=10)
AdjustTimers10000NoSleep-12 362.6µ ± 1% 292.5µ ± 1% -19.33% (p=0.000 n=10)
AdjustTimers10000NoResetNoSleep-12 317.3µ ± 8% 279.8µ ± 2% -11.79% (p=0.000 n=10)
Change-Id: Id3e10abfb1024f8e7883c8c366d941e4f1c2a894
Reviewed-on: https://go-review.googlesource.com/c/go/+/574738
Reviewed-by: Ian Lance Taylor <iant@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Russ Cox <rsc@golang.org>
|
|
For #65355
Change-Id: I65dd090fb99de9b231af2112c5ccb0eb635db2be
Reviewed-on: https://go-review.googlesource.com/c/go/+/560155
Reviewed-by: David Chase <drchase@google.com>
Reviewed-by: Michael Pratt <mpratt@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Ibrahim Bazoka <ibrahimbazoka729@gmail.com>
Auto-Submit: Emmanuel Odeke <emmanuel@orijtech.com>
|
|
Affected benchmark results, including new benchmark (some of these may
just be noise, of course):
AdjustTimers10000-12 797.7µ ± 2% 709.6µ ± 2% -11.04% (p=0.000 n=10)
TickerResetNaive-12 62.69n ± 1% 63.56n ± 1% +1.40% (p=0.018 n=10)
NowUnixMicro-12 29.95n ± 1% 30.25n ± 4% +1.00% (p=0.024 n=10)
ParseDuration-12 81.88n ± 0% 81.45n ± 0% -0.51% (p=0.006 n=10)
UnmarshalText-12 186.9n ± 1% 185.2n ± 1% -0.88% (p=0.006 n=10)
geomean 151.8n 151.2n -0.40%
Change-Id: I3ef8356249c5d703b314498e34ee8095093671c8
Reviewed-on: https://go-review.googlesource.com/c/go/+/573455
Reviewed-by: Austin Clements <austin@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Commit-Queue: Ian Lance Taylor <iant@google.com>
|
|
A proposal discussion in mid-2020 on #37196 decided to change
time.Timer and time.Ticker so that their Stop and Reset methods
guarantee that no old value (corresponding to the previous configuration
of the Timer or Ticker) will be received after the method returns.
The trivial way to do this is to make the Timer/Ticker channels
unbuffered, create a goroutine per Timer/Ticker feeding the channel,
and then coordinate with that goroutine during Stop/Reset.
Since Stop/Reset coordinate with the goroutine and the channel
is unbuffered, there is no possibility of a stale value being sent
after Stop/Reset returns.
Of course, we do not want an extra goroutine per Timer/Ticker,
but that's still a good semantic model: behave like the channels
are unbuffered and fed by a coordinating goroutine.
The actual implementation is more effort but behaves like the model.
Specifically, the timer channel has a 1-element buffer like it always has,
but len(t.C) and cap(t.C) are special-cased to return 0 anyway, so user
code cannot see what's in the buffer except with a receive.
Stop/Reset lock out any stale sends and then clear any pending send
from the buffer.
Some programs will change behavior. For example:
package main
import "time"
func main() {
t := time.NewTimer(2 * time.Second)
time.Sleep(3 * time.Second)
if t.Reset(2*time.Second) != false {
panic("expected timer to have fired")
}
<-t.C
<-t.C
}
This program (from #11513) sleeps 3s after setting a 2s timer,
resets the timer, and expects Reset to return false: the Reset is too
late and the send has already occurred. It then expects to receive
two values: the one from before the Reset, and the one from after
the Reset.
With an unbuffered timer channel, it should be clear that no value
can be sent during the time.Sleep, so the time.Reset returns true,
indicating that the Reset stopped the timer from going off.
Then there is only one value to receive from t.C: the one from after the Reset.
In 2015, I used the above example as an argument against this change.
Note that a correct version of the program would be:
func main() {
t := time.NewTimer(2 * time.Second)
time.Sleep(3 * time.Second)
if !t.Reset(2*time.Second) {
<-t.C
}
<-t.C
}
This works with either semantics, by heeding t.Reset's result.
The change should not affect correct programs.
However, one way that the change would be visible is when programs
use len(t.C) (instead of a non-blocking receive) to poll whether the timer
has triggered already. We might legitimately worry about breaking such
programs.
In 2020, discussing #37196, Bryan Mills and I surveyed programs using
len on timer channels. These are exceedingly rare to start with; nearly all
the uses are buggy; and all the buggy programs would be fixed by the new
semantics. The details are at [1].
To further reduce the impact of this change, this CL adds a temporary
GODEBUG setting, which we didn't know about yet in 2015 and 2020.
Specifically, asynctimerchan=1 disables the change and is the default
for main programs in modules that use a Go version before 1.23.
We hope to be able to retire this setting after the minimum 2-year window.
Setting asynctimerchan=1 also disables the garbage collection change
from CL 568341, although users shouldn't need to know that since
it is not a semantically visible change (unless we have bugs!).
As an undocumented bonus that we do not officially support,
asynctimerchan=2 disables the channel buffer change but keeps
the garbage collection change. This may help while we are
shaking out bugs in either of them.
Fixes #37196.
[1] https://github.com/golang/go/issues/37196#issuecomment-641698749
Change-Id: I8925d3fb2b86b2ae87fd2acd055011cbf7bd5916
Reviewed-on: https://go-review.googlesource.com/c/go/+/568341
Reviewed-by: Austin Clements <austin@google.com>
Auto-Submit: Russ Cox <rsc@golang.org>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
|
|
Classic operating system kernel mistake: if you start using
per-CPU data without disabling interrupts on the CPU,
and then an interrupt reschedules the process onto a different
CPU, now you're using the wrong CPU's per-CPU data.
The same thing happens in Go if you use per-M or per-P
data structures while not holding a lock nor using acquirem.
In the original timer.modify before CL 564977, I had been
very careful about this during the "unlock t; lock ts" dance,
only calling releasem after ts was locked. That made sure
we used the right ts. The refactoring of that code into its
own helper function in CL 564977 missed that nuance.
The code
ts := &getg().m.p.p.ptr().timers
ts.lock()
was now executing without holding any locks nor acquirem.
If the goroutine changed its M or P between deciding which
ts to use and actually locking that ts, the code would proceed
to add the timer t to some other P's timers. If the P was idle
by then, the scheduler could have already checked it for timers
and not notice the newly added timer when deciding when the
next timer should trigger.
The solution is to do what the old code correctly did, namely
acquirem before deciding which ts to use, rather than assume
getg().m.p won't change before ts.lock can complete.
This CL does that.
Before CL 564977,
stress ./time.test -test.run='ZeroTimer/impl=(func|cache)' -test.timeout=3m -test.count=20
ran without failure for over an hour on my laptop.
Starting in CL 564977, it consistently failed within a few minutes.
After this CL, it now runs without failure for over an hour again.
Fixes #66006.
Change-Id: Ib9e7ccaa0f22a326ce3fdef2b9a92f7f0bdafcbf
Reviewed-on: https://go-review.googlesource.com/c/go/+/571196
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Russ Cox <rsc@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
|
|
From the beginning of Go, the time package has had a gotcha:
if you use a select on <-time.After(1*time.Minute), even if the select
finishes immediately because some other case is ready, the underlying
timer from time.After keeps running until the minute is over. This
pins the timer in the timer heap, which keeps it from being garbage
collected and in extreme cases also slows down timer operations.
The lack of garbage collection is the more important problem.
The docs for After warn against this scenario and suggest using
NewTimer with a call to Stop after the select instead, purely to work
around this garbage collection problem.
Oddly, the docs for NewTimer and NewTicker do not mention this
problem, but they have the same issue: they cannot be collected until
either they are Stopped or, in the case of Timer, the timer expires.
(Tickers repeat, so they never expire.) People have built up a shared
knowledge that timers and tickers need to defer t.Stop even though the
docs do not mention this (it is somewhat implied by the After docs).
This CL fixes the garbage collection problem, so that a timer that is
unreferenced can be GC'ed immediately, even if it is still running.
The approach is to only insert the timer into the heap when some
channel operation is blocked on it; the last channel operation to stop
using the timer takes it back out of the heap. When a timer's channel
is no longer referenced, there are no channel operations blocked on
it, so it's not in the heap, so it can be GC'ed immediately.
This CL adds an undocumented GODEBUG asynctimerchan=1
that will disable the change. The documentation happens in
the CL 568341.
Fixes #8898.
Fixes #61542.
Change-Id: Ieb303b6de1fb3527d3256135151a9e983f3c27e6
Reviewed-on: https://go-review.googlesource.com/c/go/+/512355
Reviewed-by: Austin Clements <austin@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Russ Cox <rsc@golang.org>
|
|
Toggled by a compile-time const, so there should be no
runtime footprint in ordinary builds.
Change-Id: I7751847524f4fda3853388d3e5a18188bd737c27
Reviewed-on: https://go-review.googlesource.com/c/go/+/570336
Auto-Submit: Russ Cox <rsc@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Austin Clements <austin@google.com>
|
|
The timers had evolved to the point where the state was stored as follows:
if timer in heap:
state has timerHeaped set
if heap timer is stale:
heap deadline in t.when
real deadline in t.nextWhen
state has timerNextWhen set
else:
real deadline in t.when
t.nextWhen unset
else:
real deadline in t.when
t.nextWhen unset
That made it hard to find the real deadline and just hard to think about everything.
The new state is:
real deadline in t.when (always)
if timer in heap:
state has timerHeaped set
heap deadline in t.whenHeap
if heap timer is stale:
state has timerModified set
Separately, the 'state' word itself was being used as a lock
and state bits because the code started with CAS loops,
which we abstracted into the lock/unlock methods step by step.
At this point, we can switch to a real lock, making sure to
publish the one boolean needed by timers fast paths
at each unlock.
All this simplifies various logic considerably.
Change-Id: I35766204f7a26d999206bd56cc0db60ad1b17cbe
Reviewed-on: https://go-review.googlesource.com/c/go/+/570335
Auto-Submit: Russ Cox <rsc@golang.org>
Reviewed-by: Austin Clements <austin@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Reviewed-by: Michael Pratt <mpratt@google.com>
|
|
If user code has two timers t1 and t2 and does *t1 = *t2
(or *t1 = Timer{}), it creeps me out that we would be
corrupting the runtime data structures inlined in the
Timer struct. Replace that field with a pointer to the
runtime data structure instead, so that the corruption
cannot happen, even in a badly behaved program.
In fact, remove the struct definition entirely and linkname
a constructor instead. Now the runtime can evolve the struct
however it likes without needing to keep package time in sync.
Also move the workaround logic for #21874 out of
runtime and into package time.
Change-Id: Ia30f7802ee7b3a11f5d8a78dd30fd9c8633dc787
Reviewed-on: https://go-review.googlesource.com/c/go/+/568339
Reviewed-by: Ian Lance Taylor <iant@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
|
|
Ticker.Reset was added in CL 217362 in 2020.
It added the runtime helper modTimer, which is
analogous to startTimer and resetTimer but for tickers.
Unlike those, it does not contain a racerelease, which
means that code synchronizing by starting a ticker
will be diagnosed with a spurious race.
Add racerelease to modTimer and add tests of all
three racereleases (in startTimer, resetTimer, and modTimer).
Also do not call time.resetTimer from elsewhere in runtime,
since that function is only for package time. Use t.reset instead.
For #33184.
Change-Id: Ie40c1ad24911f21e81b1d3cc608cf086ff2bc83d
Reviewed-on: https://go-review.googlesource.com/c/go/+/568340
Auto-Submit: Russ Cox <rsc@golang.org>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Michael Pratt <mpratt@google.com>
|
|
allp < timers has not been necessary since CL 258303.
sched < timers was implied by allp < timers, and that
was still necessary, but only when the world is stopped.
Rewrite the code to avoid that lock since the world is stopped.
Now timers and timer are independent of the scheduler,
so they could call into the scheduler (for example to ready
a goroutine) if we wanted them to.
Change-Id: I12a93013c98e51c9e2f2148175b02afce8384a59
Reviewed-on: https://go-review.googlesource.com/c/go/+/568337
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Michael Pratt <mpratt@google.com>
Auto-Submit: Russ Cox <rsc@golang.org>
|
|
Before CL 564118, there were two ways to add a new timer:
addtimer or modtimer. Much code was duplicated between them
and it was always valid to call modtimer instead of addtimer
(but not vice versa), so that CL changed all addtimer call sites
to use modtimer and deleted addtimer.
One thing that was unique to addtimer, however, was that it
called cleantimers (now named ts.cleanHead) after locking the
timers, while modtimer did not. This was the only difference
in the duplicated code, and I missed it. Restore the call to
ts.cleanHead when adding a new timer.
Also fix double-unlock in cleanHead.
Change-Id: I26cc50d650f31f977c0c31195cd013244883dba9
Reviewed-on: https://go-review.googlesource.com/c/go/+/568338
Reviewed-by: Ian Lance Taylor <iant@google.com>
Auto-Submit: Russ Cox <rsc@golang.org>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Michael Pratt <mpratt@google.com>
|
|
The comment in updateTimerPMask is wrong. It says:
// Looks like there are no timers, however another P
// may be adding one at this very moment.
// Take the lock to synchronize.
This was my incorrect simplification of the original comment
from CL 264477 when I was renaming all the things it mentioned:
// Looks like there are no timers, however another P may transiently
// decrement numTimers when handling a timerModified timer in
// checkTimers. We must take timersLock to serialize with these changes.
updateTimerPMask is being called by pidleput, so the P in question
is not in use. And other P's cannot add to this P.
As the original comment more precisely noted, the problem was
that other P's might be calling timers.check, which updates ts.len
occasionally while ts is locked, and one of those updates might
"leak" an ephemeral len==0 even when the heap is not going to
be empty when the P is finally unlocked. The lock/unlock in
updateTimerPMask synchronizes to avoid that. But this defeats
most of the purpose of using ts.len in the first place.
Instead of requiring that synchronization, we can arrange that
ts.len only ever shows a "publishable" length, meaning the len(ts.heap)
we leave behind during ts.unlock.
Having done that, updateTimerPMask can be inlined into pidleput.
The big comment on updateTimerPMask explaining how timerpMask
works is better placed as the doc comment for timerpMask itself,
so move it there.
Change-Id: I5442c9bb7f1473b5fd37c43165429d087012e73f
Reviewed-on: https://go-review.googlesource.com/c/go/+/568336
Reviewed-by: Michael Pratt <mpratt@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Russ Cox <rsc@golang.org>
|
|
No semantic changes here.
Cleaning up for next change.
Change-Id: I9706009739677ff9eb893bcc007d805f7877511e
Reviewed-on: https://go-review.googlesource.com/c/go/+/568335
Reviewed-by: Austin Clements <austin@google.com>
Auto-Submit: Russ Cox <rsc@golang.org>
Reviewed-by: Michael Pratt <mpratt@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
|
|
The timer zombie count was fundamentally racy and worked around
in CL 569995. We worked around that by ignoring underflow.
The fundamnental race was because t.ts was set before t was
inserted into ts. CL 564997 corrected that fundamental problem,
so now we can account for zombies completely accurately,
never seeing values less than zero. Do that.
Change-Id: Idfbccc6662af5935f29f2a06a35e8ea93929bed7
Reviewed-on: https://go-review.googlesource.com/c/go/+/569996
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Austin Clements <austin@google.com>
Auto-Submit: Russ Cox <rsc@golang.org>
|
|
timers.wakeTime, which is called concurrently by P's trying to decide
how long they should sleep, can return inaccurate values while
timers.adjust is running. (Before the refactoring, this was still true
but the code did not have good names and was spread across more
files, making the race harder to see.)
The runtime thread sleeping code is complex enough that I am not
confident that the inaccuracy can cause delayed timer wakeups,
but I am also not confident that it can't, nor that it won't in the future.
There are two parts to the fix:
1. A simple logic change in timers.adjust.
2. The introduction of t.maybeAdd to avoid having a t that is
marked as belonging to a specific timers ts but not present
in ts.heap. That was okay before when everything was racy
but needs to be eliminated to make timers.adjust fully consistent.
The cost of the change is an extra CAS-lock operation on a timer add
(close to free since the CAS-lock was just unlocked) and a change
in the static lock ranking to allow malloc while holding a timer lock.
Change-Id: I1249e6e24ae9ef74a69837f453e15b513f0d75c0
Reviewed-on: https://go-review.googlesource.com/c/go/+/564977
Reviewed-by: Austin Clements <austin@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Russ Cox <rsc@golang.org>
|
|
Writes to timers.len are protected by the timers.lock.
There is no need to use an Add instead of a Store,
and the code is clearer (and perhaps slightly faster)
using the Store.
Change-Id: Icc6caef1b7405adec55c9b55b999b71de7d97484
Reviewed-on: https://go-review.googlesource.com/c/go/+/564976
Auto-Submit: Russ Cox <rsc@golang.org>
Reviewed-by: Austin Clements <austin@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
|
|
These names were copied over from the p field names,
but now that they are part of the timers type they can use
shorter names that make the relationship clearer.
timer0When -> minWhen
timerModifiedEarliest -> minNextWhen
This code change is only the renaming.
Change-Id: I1c0adc0b3a1289d35639619d5c945585b2d81a9f
Reviewed-on: https://go-review.googlesource.com/c/go/+/564975
Auto-Submit: Russ Cox <rsc@golang.org>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Austin Clements <austin@google.com>
|
|
The current implementation sets t.ts before adding t to ts;
that can cause inconsistencies with temporarily negative
ts.zombies values. Handle them gracefully, since we only
care about detecting very positive values.
Pending CL 564977 removes the race that sets t.ts early,
and then CL 569996 builds on top of that to make the count precise.
This CL just gets examples like the new test working sooner.
Change-Id: Ibe1aecc2554f83436f761f48e4050bd962982e4f
Reviewed-on: https://go-review.googlesource.com/c/go/+/569995
Reviewed-by: Austin Clements <austin@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
|
|
Continuing conversion from C to Go, introduce type timers
encapsulating all timer heap state, with methods for operations.
This should at least be easier to think about, instead of having
these fields strewn through the P struct. It should also be easier
to test.
I am skeptical about the pair of atomic int64 deadlines:
I think there are missed wakeups lurking.
Having the code in an abstracted API should make it easier
to reason through and fix if needed.
[This is one CL in a refactoring stack making very small changes
in each step, so that any subtle bugs that we miss can be more
easily pinpointed to a small change.]
Change-Id: If5ea3e0b946ca14076f44c85cbb4feb9eddb4f95
Reviewed-on: https://go-review.googlesource.com/c/go/+/564132
Reviewed-by: Austin Clements <austin@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Russ Cox <rsc@golang.org>
|
|
Continuing conversion from C to Go, change timer API to use methods.
[This is one CL in a refactoring stack making very small changes
in each step, so that any subtle bugs that we miss can be more
easily pinpointed to a small change.]
Change-Id: I4cb88a366993a77aa4fad739793a7db7213cc38c
Reviewed-on: https://go-review.googlesource.com/c/go/+/564131
Reviewed-by: Ian Lance Taylor <iant@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
|
|
The status enumeration is simple enough now that we can
view it as a bit set instead. Switch to a bit set, freeing up
the remaining bits for use in followup work to allow
garbage-collecting timers.
[This is one CL in a refactoring stack making very small changes
in each step, so that any subtle bugs that we miss can be more
easily pinpointed to a small change.]
Change-Id: I5f331fe3db1b5cb52f8571091f97f8ba029f3ac9
Reviewed-on: https://go-review.googlesource.com/c/go/+/564130
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
|
|
Continue using timer.lock to simplify timer operations.
[This is one CL in a refactoring stack making very small changes
in each step, so that any subtle bugs that we miss can be more
easily pinpointed to a small change.]
Change-Id: Iaf371315308425d132217eacb20b1e120a6833c5
Reviewed-on: https://go-review.googlesource.com/c/go/+/564127
Reviewed-by: Ian Lance Taylor <iant@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
|
|
Continue using timer.lock to simplify timer operations.
[This is one CL in a refactoring stack making very small changes
in each step, so that any subtle bugs that we miss can be more
easily pinpointed to a small change.]
Change-Id: I2298cede902cbf0aea268c54d741190007a733c8
Reviewed-on: https://go-review.googlesource.com/c/go/+/564128
Reviewed-by: Ian Lance Taylor <iant@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
|
|
Continue using timer.lock to simplify timer operations.
[This is one CL in a refactoring stack making very small changes
in each step, so that any subtle bugs that we miss can be more
easily pinpointed to a small change.]
Change-Id: I504335a010d6eb4d7d627145b64a896582158406
Reviewed-on: https://go-review.googlesource.com/c/go/+/564129
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
|
|
Continue using timer.lock to simplify timer operations.
[This is one CL in a refactoring stack making very small changes
in each step, so that any subtle bugs that we miss can be more
easily pinpointed to a small change.]
Change-Id: Ic12fd2630e8ac23cddd00fa7e3240a1ac19da596
Reviewed-on: https://go-review.googlesource.com/c/go/+/564126
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
|
|
Continue using timer.lock to simplify timer operations.
Note the removal of a previous potential deadlock.
(Explained at new line 325, there was a lock inversion
between individual timer locks and the 'timers' lock.)
[This is one CL in a refactoring stack making very small changes
in each step, so that any subtle bugs that we miss can be more
easily pinpointed to a small change.]
Change-Id: I8c9be00d13c6acd171a8aa2882a4fc844498f754
Reviewed-on: https://go-review.googlesource.com/c/go/+/564125
Reviewed-by: Ian Lance Taylor <iant@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
|
|
No deadlocks yet!
Change-Id: I87fb3742a386d682fbcc8cb98e98771b54bc3fec
Reviewed-on: https://go-review.googlesource.com/c/go/+/564133
Reviewed-by: Austin Clements <austin@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
|
|
The state set is now simplified enough that all the CAS loops
are starting to look the same: they are just spin locks.
So introduce an actual timer.lock method and use it in deltimer.
[This is one CL in a refactoring stack making very small changes
in each step, so that any subtle bugs that we miss can be more
easily pinpointed to a small change.]
Change-Id: Ifd7f20eeede5c764ef10ecba64855c29a5ddbe39
Reviewed-on: https://go-review.googlesource.com/c/go/+/564124
Reviewed-by: Ian Lance Taylor <iant@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
|
|
When we make a change to a timer, we have to note the
desired change to t.when and then wait for the timer heap
owner to apply the change. There are two possible changes:
delete or set a new t.when. Most of the code for processing
these changes is the same, so we can simplify the code by
making both have the same state: timerDeleted is now
timerModified with t.nextwhen == 0.
This is part of a larger simplification of the state set.
[This is one CL in a refactoring stack making very small changes
in each step, so that any subtle bugs that we miss can be more
easily pinpointed to a small change.]
Change-Id: I1a2a12f8250bcd40f7b08b83f22c3a82b124eda6
Reviewed-on: https://go-review.googlesource.com/c/go/+/564123
Reviewed-by: Ian Lance Taylor <iant@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
|
|
For historical reasons, we have to treat a zero timer as
the same as an initialized timer that was stopped (removed).
The two states are already treated mostly identically.
Merge them.
This is part of a larger simplification of the state set.
[This is one CL in a refactoring stack making very small changes
in each step, so that any subtle bugs that we miss can be more
easily pinpointed to a small change.]
Change-Id: I9c3aeb8f92bafb18c47489c1ec20a7b87ac5cd9c
Reviewed-on: https://go-review.googlesource.com/c/go/+/564122
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
|
|
timerMoving is just a kind of "locked for modification",
so merge it into timerModifying.
This is part of a larger simplification of the state set.
[This is one CL in a refactoring stack making very small changes
in each step, so that any subtle bugs that we miss can be more
easily pinpointed to a small change.]
Change-Id: I5379122f96d9921ecda7a6a37cabd6c6b4d529a4
Reviewed-on: https://go-review.googlesource.com/c/go/+/564121
Reviewed-by: Ian Lance Taylor <iant@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
|
