Commit graph

203 commits

Author SHA1 Message Date
Michael Anthony Knyszek
4e3d58009a runtime: reset scavenge address in scavengeAll
Currently scavengeAll (which is called by debug.FreeOSMemory) doesn't
reset the scavenge address before scavenging, meaning it could miss
large portions of the heap. Fix this by reseting the address before
scavenging, which will ensure it is able to walk over the entire heap.

Fixes #35858.

Change-Id: I4a7408050b8e134318ff94428f98cb96a1795aa9
Reviewed-on: https://go-review.googlesource.com/c/go/+/208960
Reviewed-by: Cherry Zhang <cherryyz@google.com>
Run-TryBot: Cherry Zhang <cherryyz@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
2019-11-27 15:06:55 +00:00
Ville Skyttä
440f7d6404 all: fix a bunch of misspellings
Change-Id: I5b909df0fd048cd66c5a27fca1b06466d3bcaac7
GitHub-Last-Rev: 778c5d2131
GitHub-Pull-Request: golang/go#35624
Reviewed-on: https://go-review.googlesource.com/c/go/+/207421
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
2019-11-15 21:04:43 +00:00
Michael Anthony Knyszek
a2cd2bd55d runtime: add per-p page allocation cache
This change adds a per-p free page cache which the page allocator may
allocate out of without a lock. The change also introduces a completely
lockless page allocator fast path.

Although the cache contains at most 64 pages (and usually less), the
vast majority (85%+) of page allocations are exactly 1 page in size.

Updates #35112.

Change-Id: I170bf0a9375873e7e3230845eb1df7e5cf741b78
Reviewed-on: https://go-review.googlesource.com/c/go/+/195701
Run-TryBot: Michael Knyszek <mknyszek@google.com>
Reviewed-by: Austin Clements <austin@google.com>
2019-11-08 18:00:54 +00:00
Michael Anthony Knyszek
4517c02f28 runtime: add per-p mspan cache
This change adds a per-p mspan object cache similar to the sudog cache.
Unfortunately this cache can't quite operate like the sudog cache, since
it is used in contexts where write barriers are disallowed (i.e.
allocation codepaths), so rather than managing an array and a slice,
it's just an array and a length. A little bit more unsafe, but avoids
any write barriers.

The purpose of this change is to reduce the number of operations which
require the heap lock in allocation, paving the way for a lockless fast
path.

Updates #35112.

Change-Id: I32cfdcd8528fb7be985640e4f3a13cb98ffb7865
Reviewed-on: https://go-review.googlesource.com/c/go/+/196642
Run-TryBot: Michael Knyszek <mknyszek@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Austin Clements <austin@google.com>
2019-11-08 17:01:32 +00:00
Michael Anthony Knyszek
a762221bea runtime: rearrange mheap_.alloc* into allocSpan
This change combines the functionality of allocSpanLocked, allocManual,
and alloc_m into a new method called allocSpan. While these methods'
abstraction boundaries are OK when the heap lock is held throughout,
they start to break down when we want finer-grained locking in the page
allocator.

allocSpan does just that, and only locks the heap when it absolutely has
to. Piggy-backing off of work in previous CLs to make more of span
initialization lockless, this change makes span initialization entirely
lockless as part of the reorganization.

Ultimately this change will enable us to add a lockless fast path to
allocSpan.

Updates #35112.

Change-Id: I99875939d75fb4e958a67ac99e4a7cda44f06864
Reviewed-on: https://go-review.googlesource.com/c/go/+/196641
Run-TryBot: Michael Knyszek <mknyszek@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Austin Clements <austin@google.com>
2019-11-08 17:01:18 +00:00
Michael Anthony Knyszek
dac936a4ab runtime: make more page sweeper operations atomic
This change makes it so that allocation and free related page sweeper
metadata operations (e.g. pageInUse and pagesInUse) are atomic rather
than protected by the heap lock. This will help in reducing the length
of the critical path with the heap lock held in future changes.

Updates #35112.

Change-Id: Ie82bff024204dd17c4c671af63350a7a41add354
Reviewed-on: https://go-review.googlesource.com/c/go/+/196640
Run-TryBot: Michael Knyszek <mknyszek@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Austin Clements <austin@google.com>
2019-11-08 17:00:57 +00:00
Michael Anthony Knyszek
7f574e476a runtime: remove unnecessary large parameter to mheap_.alloc
mheap_.alloc currently accepts both a spanClass and a "large" parameter
indicating whether the allocation is large. These are redundant, since
spanClass.sizeclass() == 0 is an equivalent way to determine this and is
already used in mheap_.alloc. There are no places in the runtime where
the size class could be non-zero and large == true.

Updates #35112.

Change-Id: Ie66facf8f0faca6f4cd3d20a8ac4bc259e11823d
Reviewed-on: https://go-review.googlesource.com/c/go/+/196639
Run-TryBot: Michael Knyszek <mknyszek@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Austin Clements <austin@google.com>
2019-11-08 16:44:33 +00:00
Michael Anthony Knyszek
ffb5646fe0 runtime: define maximum supported physical page and huge page sizes
This change defines a maximum supported physical and huge page size in
the runtime based on the new page allocator's implementation, and uses
them where appropriate.

Furthemore, if the system exceeds the maximum supported huge page
size, we simply ignore it silently.

It also fixes a huge-page-related test which is only triggered by a
condition which is definitely wrong.

Finally, it adds a few TODOs related to code clean-up and supporting
larger huge page sizes.

Updates #35112.
Fixes #35431.

Change-Id: Ie4348afb6bf047cce2c1433576d1514720d8230f
Reviewed-on: https://go-review.googlesource.com/c/go/+/205937
Run-TryBot: Michael Knyszek <mknyszek@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
2019-11-08 16:35:48 +00:00
Michael Anthony Knyszek
ae4534e659 runtime: ensure heap memstats are updated atomically
For the most part, heap memstats are already updated atomically when
passed down to OS-level memory functions (e.g. sysMap). Elsewhere,
however, they're updated with the heap lock.

In order to facilitate holding the heap lock for less time during
allocation paths, this change more consistently makes the update of
these statistics atomic by calling mSysStat{Inc,Dec} appropriately
instead of simply adding or subtracting. It also ensures these values
are loaded atomically.

Furthermore, an undocumented but safe update condition for these
memstats is during STW, at which point using atomics is unnecessary.
This change also documents this condition in mstats.go.

Updates #35112.

Change-Id: I87d0b6c27b98c88099acd2563ea23f8da1239b66
Reviewed-on: https://go-review.googlesource.com/c/go/+/196638
Run-TryBot: Michael Knyszek <mknyszek@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Austin Clements <austin@google.com>
2019-11-08 16:21:04 +00:00
Michael Anthony Knyszek
814c5058bb runtime: remove useless heap_objects accounting
This change removes useless additional heap_objects accounting for large
objects. heap_objects is computed from scratch at ReadMemStats time
(which stops the world) by using nlargealloc and nlargefree, so mutating
heap_objects turns out to be pointless.

As a result, the "large" parameter on "mheap_.freeSpan" is no longer
necessary and so this change cleans that up too.

Change-Id: I7d6b486d9b57c018e3db46221d81b55fe4c1b021
Reviewed-on: https://go-review.googlesource.com/c/go/+/196637
Run-TryBot: Michael Knyszek <mknyszek@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Austin Clements <austin@google.com>
2019-11-08 16:20:27 +00:00
Michael Anthony Knyszek
4208dbef16 runtime: make allocNeedsZero lock-free
In preparation for a lockless fast path in the page allocator, this
change makes it so that checking if an allocation needs to be zeroed may
be done atomically.

Unfortunately, this means there is a CAS-loop to ensure monotonicity of
the zeroedBase value in heapArena. This CAS-loop exits if an allocator
acquiring memory further on in the arena wins or if it succeeds. The
CAS-loop should have a relatively small amount of contention because of
this monotonicity, though it would be ideal if we could just have
CAS-ers with the greatest value always win. The CAS-loop is unnecessary
in the steady-state, but should bring some start-up performance gains as
it's likely cheaper than the additional zeroing required, especially for
large allocations.

For very large allocations that span arenas, the CAS-loop should be
completely uncontended for most of the arenas it touches, it may only
encounter contention on the first and last arena.

Updates #35112.

Change-Id: If3d19198b33f1b1387b71e1ce5902d39a5c0f98e
Reviewed-on: https://go-review.googlesource.com/c/go/+/203859
Run-TryBot: Michael Knyszek <mknyszek@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Austin Clements <austin@google.com>
2019-11-08 16:20:17 +00:00
Michael Anthony Knyszek
33dfd3529b runtime: remove old page allocator
This change removes the old page allocator from the runtime.

Updates #35112.

Change-Id: Ib20e1c030f869b6318cd6f4288a9befdbae1b771
Reviewed-on: https://go-review.googlesource.com/c/go/+/195700
Run-TryBot: Michael Knyszek <mknyszek@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Austin Clements <austin@google.com>
2019-11-08 00:07:43 +00:00
Michael Anthony Knyszek
a120cc8b36 runtime: compute whether a span needs zeroing in the new page allocator
This change adds the allocNeedZero method to mheap which uses the new
heapArena field zeroedBase to determine whether a new allocation needs
zeroing. The purpose of this work is to avoid zeroing memory that is
fresh from the OS in the context of the new allocator, where we no
longer have the concept of a free span to track this information.

The new field in heapArena, zeroedBase, is small, which runs counter to
the advice in the doc comment for heapArena. Since heapArenas are
already not a multiple of the system page size, this advice seems stale,
and we're OK with using an extra physical page for a heapArena. So, this
change also deletes the comment with that advice.

Updates #35112.

Change-Id: I688cd9fd3c57a98a6d43c45cf699543ce16697e2
Reviewed-on: https://go-review.googlesource.com/c/go/+/203858
Run-TryBot: Michael Knyszek <mknyszek@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Austin Clements <austin@google.com>
2019-11-07 20:52:05 +00:00
Michael Anthony Knyszek
689f6f77f0 runtime: integrate new page allocator into runtime
This change integrates all the bits and pieces of the new page allocator
into the runtime, behind a global constant.

Updates #35112.

Change-Id: I6696bde7bab098a498ab37ed2a2caad2a05d30ec
Reviewed-on: https://go-review.googlesource.com/c/go/+/201764
Run-TryBot: Michael Knyszek <mknyszek@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Austin Clements <austin@google.com>
2019-11-07 20:14:02 +00:00
Michael Anthony Knyszek
21445b091e runtime: make the scavenger self-paced
Currently the runtime background scavenger is paced externally,
controlled by a collection of variables which together describe a line
that we'd like to stay under.

However, the line to stay under is computed as a function of the number
of free and unscavenged huge pages in the heap at the end of the last
GC. Aside from this number being inaccurate (which is still acceptable),
the scavenging system also makes an order-of-magnitude assumption as to
how expensive scavenging a single page actually is.

This change simplifies the scavenger in preparation for making it
operate on bitmaps. It makes it so that the scavenger paces itself, by
measuring the amount of time it takes to scavenge a single page. The
scavenging methods on mheap already avoid breaking huge pages, so if we
scavenge a real huge page, then we'll have paced correctly, otherwise
we'll sleep for longer to avoid using more than scavengePercent wall
clock time.

Unfortunately, all this involves measuring time, which is quite tricky.
Currently we don't directly account for long process sleeps or OS-level
context switches (which is quite difficult to do in general), but we do
account for Go scheduler overhead and variations in it by maintaining an
EWMA of the ratio of time spent scavenging to the time spent sleeping.
This ratio, as well as the sleep time, are bounded in order to deal with
the aforementioned OS-related anomalies.

Updates #35112.

Change-Id: Ieca8b088fdfca2bebb06bcde25ef14a42fd5216b
Reviewed-on: https://go-review.googlesource.com/c/go/+/201763
Run-TryBot: Michael Knyszek <mknyszek@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Austin Clements <austin@google.com>
2019-11-07 20:12:18 +00:00
Michael Anthony Knyszek
383b447e0d runtime: clean up power-of-two rounding code with align functions
This change renames the "round" function to the more appropriately named
"alignUp" which rounds an integer up to the next multiple of a power of
two.

This change also adds the alignDown function, which is almost like
alignUp but rounds down to the previous multiple of a power of two.

With these two functions, we also go and replace manual rounding code
with it where we can.

Change-Id: Ie1487366280484dcb2662972b01b4f7135f72fec
Reviewed-on: https://go-review.googlesource.com/c/go/+/190618
Reviewed-by: Austin Clements <austin@google.com>
Reviewed-by: Keith Randall <khr@golang.org>
2019-11-04 23:41:34 +00:00
Austin Clements
7de15e362b runtime: atomically set span state and use as publication barrier
When everything is working correctly, any pointer the garbage
collector encounters can only point into a fully initialized heap
span, since the span must have been initialized before that pointer
could escape the heap allocator and become visible to the GC.

However, in various cases, we try to be defensive against bad
pointers. In findObject, this is just a sanity check: we never expect
to find a bad pointer, but programming errors can lead to them. In
spanOfHeap, we don't necessarily trust the pointer and we're trying to
check if it really does point to the heap, though it should always
point to something. Conservative scanning takes this to a new level,
since it can only guess that a word may be a pointer and verify this.

In all of these cases, we have a problem that the span lookup and
check can race with span initialization, since the span becomes
visible to lookups before it's fully initialized.

Furthermore, we're about to start initializing the span without the
heap lock held, which is going to introduce races where accesses were
previously protected by the heap lock.

To address this, this CL makes accesses to mspan.state atomic, and
ensures that the span is fully initialized before setting the state to
mSpanInUse. All loads are now atomic, and in any case where we don't
trust the pointer, it first atomically loads the span state and checks
that it's mSpanInUse, after which it will have synchronized with span
initialization and can safely check the other span fields.

For #10958, #24543, but a good fix in general.

Change-Id: I518b7c63555b02064b98aa5f802c92b758fef853
Reviewed-on: https://go-review.googlesource.com/c/go/+/203286
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
2019-10-31 17:09:50 +00:00
Austin Clements
a9b37ae026 runtime: fully initialize span in alloc_m
Currently, several important fields of a heap span are set by
heapBits.initSpan, which happens after the span has already been
published and returned from the locked region of alloc_m. In
particular, allocBits is set very late, which makes mspan.isFree
unsafe even if you were to lock the heap because it tries to access
allocBits.

This CL fixes this by populating these fields in alloc_m. The next CL
builds on this to only publish the span once it is fully initialized.
Together, they'll make it safe to check allocBits even if there is a
race with alloc_m.

For #10958, #24543, but a good fix in general.

Change-Id: I7fde90023af0f497e826b637efa4d19c32840c08
Reviewed-on: https://go-review.googlesource.com/c/go/+/203285
Run-TryBot: Austin Clements <austin@google.com>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
2019-10-31 17:09:48 +00:00
Michael Anthony Knyszek
62e4156552 runtime: fix lock acquire cycles related to scavenge.lock
There are currently two edges in the lock cycle graph caused by
scavenge.lock: with sched.lock and mheap_.lock. These edges appear
because of the call to ready() and stack growths respectively.
Furthermore, there's already an invariant in the code wherein
mheap_.lock must be acquired before scavenge.lock, hence the cycle.

The fix to this is to bring scavenge.lock higher in the lock cycle
graph, such that sched.lock and mheap_.lock are only acquired once
scavenge.lock is already held.

To faciliate this change, we move scavenger waking outside of
gcSetTriggerRatio such that it doesn't have to happen with the heap
locked. Furthermore, we check scavenge generation numbers with the heap
locked by using gopark instead of goparkunlock, and specify a function
which aborts the park should there be any skew in generation count.

Fixes #34047.

Change-Id: I3519119214bac66375e2b1262b36ce376c820d12
Reviewed-on: https://go-review.googlesource.com/c/go/+/191977
Run-TryBot: Michael Knyszek <mknyszek@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
2019-09-26 15:44:49 +00:00
Michael Anthony Knyszek
eb96f8a574 runtime: scavenge on growth instead of inline with allocation
Inline scavenging causes significant performance regressions in tail
latency for k8s and has relatively little benefit for RSS footprint.

We disabled inline scavenging in Go 1.12.5 (CL 174102) as well, but
we thought other changes in Go 1.13 had mitigated the issues with
inline scavenging. Apparently we were wrong.

This CL switches back to only doing foreground scavenging on heap
growth, rather than doing it when allocation tries to allocate from
scavenged space.

Fixes #32828.

Change-Id: I1f5df44046091f0b4f89fec73c2cde98bf9448cb
Reviewed-on: https://go-review.googlesource.com/c/go/+/183857
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
2019-09-25 22:20:36 +00:00
Austin Clements
f18109d7e3 runtime: grow the heap incrementally
Currently, we map and grow the heap a whole arena (64MB) at a time.
Unfortunately, in order to fix #32828, we need to switch from
scavenging inline with allocation back to scavenging on heap growth,
but heap-growth scavenging happens in large jumps because we grow the
heap in large jumps.

In order to prepare for better heap-growth scavenging, this CL
separates mapping more space for the heap from actually "growing" it
(tracking the new space with spans). Instead, growing the heap keeps
track of the "current arena" it's growing into. It track that with new
spans as needed, and only maps more arena space when the current arena
is inadequate. The effect to the user is the same, but this will let
us scavenge on much smaller increments of heap growth.

There are two slightly subtleties to this change:

1. If an allocation requires mapping a new arena and that new arena
   isn't contiguous with the current arena, we don't want to lose the
   unused space in the current arena, so we have to immediately track
   that with a span.

2. The mapped space must be accounted as released and idle, even
   though it isn't actually tracked in a span.

For #32828, since this makes heap-growth scavenging far more
effective, especially at small heap sizes. For example, this change is
necessary for TestPhysicalMemoryUtilization to pass once we remove
inline scavenging.

Change-Id: I300e74a0534062467e4ce91cdc3508e5ef9aa73a
Reviewed-on: https://go-review.googlesource.com/c/go/+/189957
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
2019-09-25 22:17:21 +00:00
Michael Knyszek
8c3040d768 runtime: call sysHugePage less often
Currently when we coalesce memory we make a sysHugePage call
(MADV_HUGEPAGE) to ensure freed and coalesced huge pages are treated as
such so the scavenger's assumptions about performance are more in line
with reality.

Unfortunately we do it way too often because we do it if there was any
change to the huge page count for the span we're coalescing into, not
taking into account that it could coalesce with its neighbors and not
actually create a new huge page.

This change makes it so that it only calls sysHugePage if the original
huge page counts between the span to be coalesced into and its neighbors
do not add up (i.e. a new huge page was created due to alignment). Calls
to sysHugePage will now happen much less frequently, as intended.

Updates #32828.

Change-Id: Ia175919cb79b730a658250425f97189e27d7fda3
Reviewed-on: https://go-review.googlesource.com/c/go/+/186926
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Austin Clements <austin@google.com>
2019-07-30 18:53:01 +00:00
Michael Anthony Knyszek
a41ebe6e25 runtime: add physHugePageShift
This change adds physHugePageShift which is defined such that
1 << physHugePageShift == physHugePageSize. The purpose of this variable
is to avoid doing expensive divisions in key functions, such as
(*mspan).hugePages.

This change also does a sweep of any place we might do a division or mod
operation with physHugePageSize and turns it into bit shifts and other
bitwise operations.

Finally, this change adds a check to mallocinit which ensures that
physHugePageSize is always a power of two. osinit might choose to ignore
non-powers-of-two for the value and replace it with zero, but mallocinit
will fail if it's not a power of two (or zero). It also derives
physHugePageShift from physHugePageSize.

This change helps improve the performance of most applications because
of how often (*mspan).hugePages is called.

Updates #32828.

Change-Id: I1a6db113d52d563f59ae8fd4f0e130858859e68f
Reviewed-on: https://go-review.googlesource.com/c/go/+/186598
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Austin Clements <austin@google.com>
2019-07-30 18:44:52 +00:00
Michael Anthony Knyszek
7ed7669c0d runtime: ensure mheap lock stack growth invariant is maintained
Currently there's an invariant in the runtime wherein the heap lock
can only be acquired on the system stack, otherwise a self-deadlock
could occur if the stack grows while the lock is held.

This invariant is upheld and documented in a number of situations (e.g.
allocManual, freeManual) but there are other places where the invariant
is either not maintained at all which risks self-deadlock (e.g.
setGCPercent, gcResetMarkState, allocmcache) or is maintained but
undocumented (e.g. gcSweep, readGCStats_m).

This change adds go:systemstack to any function that acquires the heap
lock or adds a systemstack(func() { ... }) around the critical section,
where appropriate. It also documents the invariant on (*mheap).lock
directly and updates repetitive documentation to refer to that comment.

Fixes #32105.

Change-Id: I702b1290709c118b837389c78efde25c51a2cafb
Reviewed-on: https://go-review.googlesource.com/c/go/+/177857
Run-TryBot: Michael Knyszek <mknyszek@google.com>
Reviewed-by: Austin Clements <austin@google.com>
2019-05-24 15:34:57 +00:00
Michael Anthony Knyszek
4e7bef84c1 runtime: mark newly-mapped memory as scavenged
On most platforms newly-mapped memory is untouched, meaning the pages
backing the region haven't been faulted in yet. However, we mark this
memory as unscavenged which means the background scavenger
aggressively "returns" this memory to the OS if the heap is small.

The only platform where newly-mapped memory is actually unscavenged (and
counts toward the application's RSS) is on Windows, since
(*mheap).sysAlloc commits the reservation. Instead of making a special
case for Windows, I change the requirements a bit for a sysReserve'd
region. It must now be both sysMap'd and sysUsed'd, with sysMap being a
no-op on Windows. Comments about memory allocation have been updated to
include a more up-to-date mental model of which states a region of memory
may be in (at a very low level) and how to transition between these
states.

Now this means we can correctly mark newly-mapped heap memory as
scavenged on every platform, reducing the load on the background
scavenger early on in the application for small heaps. As a result,
heap-growth scavenging is no longer necessary, since any actual RSS
growth will be accounted for on the allocation codepath.

Finally, this change also cleans up grow a little bit to avoid
pretending that it's freeing an in-use span and just does the necessary
operations directly.

Fixes #32012.
Fixes #31966.
Updates #26473.

Change-Id: Ie06061eb638162e0560cdeb0b8993d94cfb4d290
Reviewed-on: https://go-review.googlesource.com/c/go/+/177097
Run-TryBot: Michael Knyszek <mknyszek@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Austin Clements <austin@google.com>
2019-05-16 22:00:47 +00:00
Michael Anthony Knyszek
ff70494b75 runtime: split spans when scavenging if it's more than we need
This change makes it so that during scavenging we split spans when the
span we have next for scavenging is larger than the amount of work we
have left to do.

The purpose of this change is to improve the worst-case behavior of the
scavenger: currently, if the scavenger only has a little bit of work to
do but sees a very large free span, it will scavenge the whole thing,
spending a lot of time to get way ahead of the scavenge pacing for no
reason.

With this change the scavenger should follow the pacing more closely,
but may still over-scavenge by up to a physical huge page since the
splitting behavior avoids breaking up huge pages in free spans.

This change is also the culmination of the scavenging improvements, so
we also include benchmark results for this series (starting from
"runtime: merge all treaps into one implementation" until this patch).

This patch stack results in average and peak RSS reductions (up to 11%
and 7% respectively) for some benchmarks, with mostly minimal
performance degredation (3-4% for some benchmarks, ~0% geomean). Each of
these benchmarks was executed with GODEBUG=madvdontneed=1 on Linux; the
performance degredation is even smaller when MADV_FREE may be used, but
the impact on RSS is much harder to measure. Applications that generally
maintain a steady heap size for the most part show no change in
application performance.

These benchmarks are taken from an experimental benchmarking suite
representing a variety of open-source Go packages, the raw results may
be found here:

https://perf.golang.org/search?q=upload:20190509.1

For #30333.

Change-Id: I618a48534d2d6ce5f656bb66825e3c383ab1ffba
Reviewed-on: https://go-review.googlesource.com/c/go/+/175797
Run-TryBot: Michael Knyszek <mknyszek@google.com>
Reviewed-by: Austin Clements <austin@google.com>
2019-05-09 16:23:09 +00:00
Michael Anthony Knyszek
fe67ea32bf runtime: add background scavenger
This change adds a background scavenging goroutine whose pacing is
determined when the heap goal changes. The scavenger is paced to use
at most 1% of the mutator's time for most systems. Furthermore, the
scavenger's pacing is computed based on the estimated number of
scavengable huge pages to take advantage of optimizations provided by
the OS.

The purpose of this scavenger is to deal with a shrinking heap: if the
heap goal is falling over time, the scavenger should kick in and start
returning free pages from the heap to the OS.

Also, now that we have a pacing system, the credit system used by
scavengeLocked has become redundant. Replace it with a mechanism which
only scavenges on the allocation path if it makes sense to do so with
respect to the new pacing system.

Fixes #30333.

Change-Id: I6203f8dc84affb26c3ab04528889dd9663530edc
Reviewed-on: https://go-review.googlesource.com/c/go/+/142960
Run-TryBot: Michael Knyszek <mknyszek@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Austin Clements <austin@google.com>
2019-05-09 16:21:43 +00:00
Michael Anthony Knyszek
eaa1c87b00 runtime: remove periodic scavenging
This change removes the periodic scavenger which goes over every span
in the heap and scavenges it if it hasn't been used for 5 minutes. It
should no longer be necessary if we have background scavenging
(follow-up).

For #30333.

Change-Id: Ic3a1a4e85409dc25719ba4593a3b60273a4c71e0
Reviewed-on: https://go-review.googlesource.com/c/go/+/143157
Run-TryBot: Michael Knyszek <mknyszek@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Austin Clements <austin@google.com>
2019-05-09 16:20:52 +00:00
Michael Anthony Knyszek
31c4e09915 runtime: ensure free and unscavenged spans may be backed by huge pages
This change adds a new sysHugePage function to provide the equivalent of
Linux's madvise(MADV_HUGEPAGE) support to the runtime. It then uses
sysHugePage to mark a newly-coalesced free span as backable by huge
pages to make the freeHugePages approximation a bit more accurate.

The problem being solved here is that if a large free span is composed
of many small spans which were coalesced together, then there's a chance
that they have had madvise(MADV_NOHUGEPAGE) called on them at some point,
which makes freeHugePages less accurate.

For #30333.

Change-Id: Idd4b02567619fc8d45647d9abd18da42f96f0522
Reviewed-on: https://go-review.googlesource.com/c/go/+/173338
Run-TryBot: Michael Knyszek <mknyszek@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Austin Clements <austin@google.com>
2019-05-06 21:15:01 +00:00
Michael Anthony Knyszek
5c15ed64de runtime: split spans during allocation without treap removal
Now that the treap is first-fit, we can make a nice optimization.
Mainly, since we know that span splitting doesn't modify the relative
position of a span in a treap, we can actually modify a span in-place
on the treap. The only caveat is that we need to update the relevant
metadata.

To enable this optimization, this change introduces a mutate method on
the iterator which takes a callback that is passed the iterator's span.
The method records some properties of the span before it calls into the
callback and then uses those records to see what changed and update
treap metadata appropriately.

Change-Id: I74f7d2ee172800828434ba0194d3d78d3942acf2
Reviewed-on: https://go-review.googlesource.com/c/go/+/174879
Run-TryBot: Michael Knyszek <mknyszek@google.com>
Reviewed-by: Austin Clements <austin@google.com>
2019-05-06 21:14:52 +00:00
Michael Anthony Knyszek
f4a5ae5594 runtime: track the number of free unscavenged huge pages
This change tracks the number of potential free and unscavenged huge
pages which will be used to inform the rate at which scavenging should
occur.

For #30333.

Change-Id: I47663e5ffb64cac44ffa10db158486783f707479
Reviewed-on: https://go-review.googlesource.com/c/go/+/170860
Run-TryBot: Michael Knyszek <mknyszek@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Austin Clements <austin@google.com>
2019-05-06 20:59:20 +00:00
Michael Anthony Knyszek
a62b5723be runtime: scavenge huge spans first
This change adds two new treap iteration types: one for large
unscavenged spans (contain at least one huge page) and one for small
unscavenged spans. This allows us to scavenge the huge spans first by
first iterating over the large ones, then the small ones.

Also, since we now depend on physHugePageSize being a power of two,
ensure that that's the case when it's retrieved from the OS.

For #30333.

Change-Id: I51662740205ad5e4905404a0856f5f2b2d2a5680
Reviewed-on: https://go-review.googlesource.com/c/go/+/174399
Run-TryBot: Michael Knyszek <mknyszek@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Austin Clements <austin@google.com>
2019-05-06 20:57:39 +00:00
Michael Anthony Knyszek
fa8470a8cd runtime: make treap iteration more efficient
This change introduces a treapIterFilter type which represents the
power set of states described by a treapIterType.

This change then adds a treapIterFilter field to each treap node
indicating the types of spans that live in that subtree. The field is
maintained via the same mechanism used to maintain maxPages. This allows
pred, succ, start, and end to be judicious about which subtrees it will
visit, ensuring that iteration avoids traversing irrelevant territory.

Without this change, repeated scavenging attempts can end up being N^2
as the scavenger walks over what it already scavenged before finding new
spans available for scavenging.

Finally, this change also only scavenges a span once it is removed from
the treap. There was always an invariant that spans owned by the treap
may not be mutated in-place, but with this change violating that
invariant can cause issues with scavenging.

For #30333.

Change-Id: I8040b997e21c94a8d3d9c8c6accfe23cebe0c3d3
Reviewed-on: https://go-review.googlesource.com/c/go/+/174878
Run-TryBot: Michael Knyszek <mknyszek@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Austin Clements <austin@google.com>
2019-05-06 20:50:16 +00:00
Michael Anthony Knyszek
9baa4301cf runtime: merge all treaps into one implementation
This change modifies the treap implementation to support holding all
spans in a single treap, instead of keeping them all in separate treaps.

This improves ergonomics for nearly all treap-related callsites.
With that said, iteration is now more expensive, but it never occurs on
the fast path, only on scavenging-related paths.

This change opens up the opportunity for further optimizations, such as
splitting spans without treap removal (taking treap removal off the span
allocator's critical path) as well as improvements to treap iteration
(building linked lists for each iteration type and managing them on
insert/removal, since those operations should be less frequent).

For #30333.

Change-Id: I3dac97afd3682a37fda09ae8656a770e1369d0a9
Reviewed-on: https://go-review.googlesource.com/c/go/+/174398
Run-TryBot: Michael Knyszek <mknyszek@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Austin Clements <austin@google.com>
2019-05-06 20:19:44 +00:00
Iskander Sharipov
12e63226b9 all: remove commented-out print statements
Those print statements are not a good debug helpers
and only clutter the code.

Change-Id: Ifbf450a04e6fa538af68e6352c016728edb4119a
Reviewed-on: https://go-review.googlesource.com/c/go/+/160537
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Josh Bleecher Snyder <josharian@gmail.com>
2019-05-05 08:09:30 +00:00
Michael Anthony Knyszek
40036a99a0 runtime: change the span allocation policy to first-fit
This change modifies the treap implementation to be address-ordered
instead of size-ordered, and further augments it so it may be used for
allocation. It then modifies the find method to implement a first-fit
allocation policy.

This change to the treap implementation consequently makes it so that
spans are scavenged in highest-address-first order without any
additional changes to the scavenging code. Because the treap itself is
now address ordered, and the scavenging code iterates over it in
reverse, the highest address is now chosen instead of the largest span.

This change also renames the now wrongly-named "scavengeLargest" method
on mheap to just "scavengeLocked" and also fixes up logic in that method
which made assumptions about size.

For #30333.

Change-Id: I94b6f3209211cc1bfdc8cdaea04152a232cfbbb4
Reviewed-on: https://go-review.googlesource.com/c/go/+/164101
Run-TryBot: Michael Knyszek <mknyszek@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Austin Clements <austin@google.com>
2019-05-01 14:50:40 +00:00
Michael Anthony Knyszek
7b33b6274f runtime: introduce treapForSpan to reduce code duplication
Currently which treap a span should be inserted into/removed from is
checked by looking at the span's properties. This logic is repeated in
four places. As this logic gets more complex, it makes sense to
de-duplicate this, so introduce treapForSpan instead which captures this
logic by returning the appropriate treap for the span.

For #30333.

Change-Id: I4bd933d93dc50c5fc7c7c7f56ceb95194dcbfbcc
Reviewed-on: https://go-review.googlesource.com/c/go/+/170857
Run-TryBot: Michael Knyszek <mknyszek@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Austin Clements <austin@google.com>
2019-04-10 22:01:12 +00:00
Michael Anthony Knyszek
2ab6d0172e runtime: throw if scavenge necessary during coalescing
Currently when coalescing if two adjacent spans are scavenged, we
subtract their sizes from memstats and re-scavenge the new combined
span. This is wasteful however, since the realignment semantics make
this case of having to re-scavenge impossible.

In realign() inside of coalesce(), there was also a bug: on systems
where physPageSize > pageSize, we wouldn't realign because a condition
had the wrong sign. This wasteful re-scavenging has been masking this
bug this whole time. So, this change fixes that first.

Then this change gets rid of the needsScavenge logic and instead checks
explicitly for the possibility of unscavenged pages near the physical
page boundary. If the possibility exists, it throws. The intent of
throwing here is to catch changes to the runtime which cause this
invariant to no longer hold, at which point it would likely be
appropriate to scavenge the additional pages (and only the additional
pages) at that point.

Change-Id: I185e3d7b53e36e90cf9ace5fa297a9e8008d75f3
Reviewed-on: https://go-review.googlesource.com/c/go/+/158377
Run-TryBot: Michael Knyszek <mknyszek@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Austin Clements <austin@google.com>
2019-04-10 22:00:03 +00:00
Michael Anthony Knyszek
7bb8fc1033 runtime: use iterator instead of raw node for treap find
Right now the mTreap structure exposes the treapNode structure through
only one interface: find. There's no reason (performance or otherwise)
for exposing this, and we get a cleaner abstraction through the
iterators this way. This change also makes it easier to make changes to
the mTreap implementation without violating its interface.

Change-Id: I5ef86b8ac81a47d05d8404df65af9ec5f419dc40
Reviewed-on: https://go-review.googlesource.com/c/go/+/164098
Reviewed-by: Austin Clements <austin@google.com>
2019-04-08 15:41:43 +00:00
Michael Anthony Knyszek
23d4c6cdd6 runtime: merge codepaths in scavengeLargest
This change just makes the code in scavengeLargest easier to reason
about by reducing the number of exit points to the method. It should
still be correct either way because the condition checked at the end
(released > nbytes) will always be false if we return, but this just
makes the code a little easier to maintain.

Change-Id: If60da7696aca3fab3b5ddfc795d600d87c988238
Reviewed-on: https://go-review.googlesource.com/c/go/+/160617
Run-TryBot: Michael Knyszek <mknyszek@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Austin Clements <austin@google.com>
2019-04-08 15:36:03 +00:00
Michael Anthony Knyszek
faf187fb8e runtime: add credit system for scavenging
When scavenging small amounts it's possible we over-scavenge by a
significant margin since we choose to scavenge the largest spans first.
This over-scavenging is never accounted for.

With this change, we add a scavenge credit pool, similar to the reclaim
credit pool. Any time scavenging triggered by RSS growth starts up, it
checks if it can cash in some credit first. If after using all the
credit it still needs to scavenge, then any extra it does it adds back
into the credit pool.

This change mitigates the performance impact of golang.org/cl/159500 on
the Garbage benchmark. On Go1 it suggests some improvements, but most of
that is within the realm of noise (Revcomp seems very sensitive to
GC-related changes, both postively and negatively).

Garbage: https://perf.golang.org/search?q=upload:20190131.5
Go1:     https://perf.golang.org/search?q=upload:20190131.4

Performance change with both changes:

Garbage: https://perf.golang.org/search?q=upload:20190131.7
Go1:     https://perf.golang.org/search?q=upload:20190131.6

Change-Id: I87bd3c183e71656fdafef94714194b9fdbb77aa2
Reviewed-on: https://go-review.googlesource.com/c/160297
Reviewed-by: Austin Clements <austin@google.com>
2019-01-31 16:55:43 +00:00
Michael Anthony Knyszek
8e093e7a1c runtime: scavenge memory upon allocating from scavenged memory
Because scavenged and unscavenged spans no longer coalesce, memory that
is freed no longer has a high likelihood of being re-scavenged. As a
result, if an application is allocating at a fast rate, it may work fast
enough to undo all the scavenging work performed by the runtime's
current scavenging mechanisms. This behavior is exacerbated by the
global best-fit allocation policy the runtime uses, since scavenged
spans are just as likely to be chosen as unscavenged spans on average.

To remedy that, we treat each allocation of scavenged space as a heap
growth, and scavenge other memory to make up for the allocation.

This change makes performance of the runtime slightly worse, as now
we're scavenging more often during allocation. The regression is
particularly obvious with the garbage benchmark (3%) but most of the Go1
benchmarks are within the margin of noise. A follow-up change should
help.

Garbage: https://perf.golang.org/search?q=upload:20190131.3
Go1:     https://perf.golang.org/search?q=upload:20190131.2

Updates #14045.

Change-Id: I44a7e6586eca33b5f97b6d40418db53a8a7ae715
Reviewed-on: https://go-review.googlesource.com/c/159500
Reviewed-by: Austin Clements <austin@google.com>
2019-01-31 16:54:41 +00:00
Michael Anthony Knyszek
6e9f664b9a runtime: don't coalesce scavenged spans with unscavenged spans
As a result of changes earlier in Go 1.12, the scavenger became much
more aggressive. In particular, when scavenged and unscavenged spans
coalesced, they would always become scavenged. This resulted in most
spans becoming scavenged over time. While this is good for keeping the
RSS of the program low, it also causes many more undue page faults and
many more calls to madvise.

For most applications, the impact of this was negligible. But for
applications that repeatedly grow and shrink the heap by large amounts,
the overhead can be significant. The overhead was especially obvious on
older versions of Linux where MADV_FREE isn't available and
MADV_DONTNEED must be used.

This change makes it so that scavenged spans will never coalesce with
unscavenged spans. This  results in fewer page faults overall. Aside
from this, the expected impact of this change is more heap growths on
average, as span allocations will be less likely to be fulfilled. To
mitigate this slightly, this change also coalesces spans eagerly after
scavenging, to at least ensure that all scavenged spans and all
unscavenged spans are coalesced with each other.

Also, this change adds additional logic in the case where two adjacent
spans cannot coalesce. In this case, on platforms where the physical
page size is larger than the runtime's page size, we realign the
boundary between the two adjacent spans to a physical page boundary. The
advantage of this approach is that "unscavengable" spans, that is, spans
which cannot be scavenged because they don't cover at least a single
physical page are grown to a size where they have a higher likelihood of
being discovered by the runtime's scavenging mechanisms when they border
a scavenged span. This helps prevent the runtime from accruing pockets
of "unscavengable" memory in between scavenged spans, preventing them
from coalescing.

We specifically choose to apply this logic to all spans because it
simplifies the code, even though it isn't strictly necessary. The
expectation is that this change will result in a slight loss in
performance on platforms where the physical page size is larger than the
runtime page size.

Update #14045.

Change-Id: I64fd43eac1d6de6f51d7a2ecb72670f10bb12589
Reviewed-on: https://go-review.googlesource.com/c/158078
Run-TryBot: Michael Knyszek <mknyszek@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Austin Clements <austin@google.com>
2019-01-17 17:07:23 +00:00
Michael Anthony Knyszek
2f99e889f0 runtime: de-duplicate coalescing code
Currently the code surrounding coalescing is duplicated between merging
with the span before the span considered for coalescing and merging with
the span after. This change factors out the shared portions of these
codepaths into a local closure which acts as a helper.

Change-Id: I7919fbed3f9a833eafb324a21a4beaa81f2eaa91
Reviewed-on: https://go-review.googlesource.com/c/158077
Run-TryBot: Michael Knyszek <mknyszek@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Austin Clements <austin@google.com>
2019-01-17 17:05:37 +00:00
Michael Anthony Knyszek
79ac638e41 runtime: refactor coalescing into its own method
The coalescing process is complex and in a follow-up change we'll need
to do it in more than one place, so this change factors out the
coalescing code in freeSpanLocked into a method on mheap.

Change-Id: Ia266b6cb1157c1b8d3d8a4287b42fbcc032bbf3a
Reviewed-on: https://go-review.googlesource.com/c/157838
Run-TryBot: Michael Knyszek <mknyszek@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Austin Clements <austin@google.com>
2019-01-17 16:59:49 +00:00
Michael Anthony Knyszek
4b3f04c63b runtime: make mTreap iterator bidirectional
This change makes mTreap's iterator type, treapIter, bidirectional
instead of unidirectional. This change helps support moving the find
operation on a treap to return an iterator instead of a treapNode, in
order to hide the details of the treap when accessing elements.

For #28479.

Change-Id: I5dbea4fd4fb9bede6e81bfd089f2368886f98943
Reviewed-on: https://go-review.googlesource.com/c/156918
Reviewed-by: Austin Clements <austin@google.com>
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
2019-01-10 18:15:48 +00:00
Michael Anthony Knyszek
064842450b runtime: allocate from free and scav fairly
This change modifies the behavior of span allocations to no longer
prefer the free treap over the scavenged treap.

While there is an additional cost to allocating out of the scavenged
treap, the current behavior of preferring the unscavenged spans can
lead to unbounded growth of a program's virtual memory footprint.

In small programs (low # of Ps, low resident set size, low allocation
rate) this behavior isn't really apparent and is difficult to
reproduce.

However, in relatively large, long-running programs we see this
unbounded growth in free spans, and an unbounded amount of heap
growths.

It still remains unclear how this policy change actually ends up
increasing the number of heap growths over time, but switching the
policy back to best-fit does indeed solve the problem.

Change-Id: Ibb88d24f9ef6766baaa7f12b411974cc03341e7b
Reviewed-on: https://go-review.googlesource.com/c/148979
Run-TryBot: Michael Knyszek <mknyszek@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rick Hudson <rlh@golang.org>
Reviewed-by: Austin Clements <austin@google.com>
2018-12-17 23:28:36 +00:00
Michael Anthony Knyszek
3651476075 runtime: add iterator abstraction for mTreap
This change adds the treapIter type which provides an iterator
abstraction for walking over an mTreap. In particular, the mTreap type
now has iter() and rev() for iterating both forwards (smallest to
largest) and backwards (largest to smallest). It also has an erase()
method for erasing elements at the iterator's current position.

For #28479.

While the expectation is that this change will slow down Go programs,
the impact on Go1 and Garbage is negligible.

Go1:     https://perf.golang.org/search?q=upload:20181214.6
Garbage: https://perf.golang.org/search?q=upload:20181214.11

Change-Id: I60dbebbbe73cbbe7b78d45d2093cec12cc0bc649
Reviewed-on: https://go-review.googlesource.com/c/151537
Run-TryBot: Michael Knyszek <mknyszek@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Austin Clements <austin@google.com>
Reviewed-by: Rick Hudson <rlh@golang.org>
2018-12-17 23:28:18 +00:00
Austin Clements
b00a6d8bfe runtime: eliminate mheap.busy* lists
The old whole-page reclaimer was the only thing that used the busy
span lists. Remove them so nothing uses them any more.

Change-Id: I4007dd2be08b9ef41bfdb0c387215c73c392cc4c
Reviewed-on: https://go-review.googlesource.com/c/138960
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rick Hudson <rlh@golang.org>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
2018-11-15 19:27:13 +00:00
Austin Clements
5333550bdc runtime: implement efficient page reclaimer
When we attempt to allocate an N page span (either for a large
allocation or when an mcentral runs dry), we first try to sweep spans
to release N pages. Currently, this can be extremely expensive:
sweeping a span to emptiness is the hardest thing to ask for and the
sweeper generally doesn't know where to even look for potentially
fruitful results. Since this is on the critical path of many
allocations, this is unfortunate.

This CL changes how we reclaim empty spans. Instead of trying lots of
spans and hoping for the best, it uses the newly introduced span marks
to efficiently find empty spans. The span marks (and in-use bits) are
in a dense bitmap, so these spans can be found with an efficient
sequential memory scan. This approach can scan for unmarked spans at
about 300 GB/ms and can free unmarked spans at about 32 MB/ms. We
could probably significantly improve the rate at which is can free
unmarked spans, but that's a separate issue.

Like the current reclaimer, this is still linear in the number of
spans that are swept, but the constant factor is now so vanishingly
small that it doesn't matter.

The benchmark in #18155 demonstrates both significant page reclaiming
delays, and object reclaiming delays. With "-retain-count=20000000
-preallocate=true -loop-count=3", the benchmark demonstrates several
page reclaiming delays on the order of 40ms. After this change, the
page reclaims are insignificant. The longest sweeps are still ~150ms,
but are object reclaiming delays. We'll address those in the next
several CLs.

Updates #18155.

Fixes #21378 by completely replacing the logic that had that bug.

Change-Id: Iad80eec11d7fc262d02c8f0761ac6998425c4064
Reviewed-on: https://go-review.googlesource.com/c/138959
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rick Hudson <rlh@golang.org>
2018-11-15 19:27:11 +00:00