Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

runtime: scavenger not as effective as in previous releases #35788

Closed
mknyszek opened this issue Nov 22, 2019 · 10 comments
Closed

runtime: scavenger not as effective as in previous releases #35788

mknyszek opened this issue Nov 22, 2019 · 10 comments
Assignees
Labels
Milestone

Comments

@mknyszek
Copy link
Contributor

@mknyszek mknyszek commented Nov 22, 2019

Recent page allocator changes (#35112) forced changes to scavenger as well. AFAICT these changes came with two problems.

Firstly, the scavenger maintains an address which it uses to mark which part of the address space has already been searched. Unfortunately the updates of this value are racy, and it's difficult to determine what kind of check makes sense to prevent these races. Today these races mean that the scavenger can miss updates and end up not working for a whole GC cycle (since it only runs down the heap once), or it could end up iterating over address space that a partially concurrent scavenge (e.g. from heap-growth) had already looked at. The affect on application performance is a higher average RSS than in Go 1.13.

Secondly, the scavenger is awoken on each "pacing" update, but today that only means an update to the goal. Because the scavenger is now self-paced, this wake-up is mostly errant, and is generally not a good indicator that there's new work to be done. What this means for application performance is that it might be scavenging memory further down the heap than it should (violating some principles of the original scavenge design) and thus causing undue page faults, resulting in worse CPU performance than in Go 1.13.

I suggest that we fix these for Go 1.14 (fixes are already implemented), but they need not block the beta.

@mknyszek mknyszek added this to the Go1.14 milestone Nov 22, 2019
@mknyszek mknyszek self-assigned this Nov 22, 2019
@gopherbot
Copy link

@gopherbot gopherbot commented Nov 22, 2019

Change https://golang.org/cl/207998 mentions this issue: runtime: wake scavenger and update address on sweep done

@gopherbot
Copy link

@gopherbot gopherbot commented Nov 22, 2019

Change https://golang.org/cl/208378 mentions this issue: runtime: remove scavAddr in favor of address ranges

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Dec 5, 2019

@mknyszek What is left to do here for 1.14?

@mknyszek
Copy link
Contributor Author

@mknyszek mknyszek commented Dec 5, 2019

All the patches for this are out for review and I'm iterating on them.

@mknyszek
Copy link
Contributor Author

@mknyszek mknyszek commented Dec 5, 2019

Sorry, that sounds like they haven't been reviewed yet. By "iterating on them" I mean that they're being iterated on via review passes.

@gopherbot
Copy link

@gopherbot gopherbot commented Dec 11, 2019

Change https://golang.org/cl/210877 mentions this issue: runtime: add scavenge waker goroutine

@aclements
Copy link
Member

@aclements aclements commented Dec 19, 2019

We've decided this seems a bit too high-risk for 1.14 without enough reward. In practice, these races seem quite rare, and if they do happen, they're likely to only delay scavenging by one GC cycle. Bumping to 1.15. If we get evidence that this is causing real memory pressure problems in 1.14, we have the patches ready.

@gopherbot
Copy link

@gopherbot gopherbot commented Feb 10, 2020

Change https://golang.org/cl/218997 mentions this issue: runtime: avoid re-scanning scavenged and untouched memory

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 28, 2020

Change https://golang.org/cl/230718 mentions this issue: runtime: use linearAddress in more parts of the runtime

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 28, 2020

Change https://golang.org/cl/230717 mentions this issue: runtime: make addrRange[s] operate on linearized addresses

gopherbot pushed a commit that referenced this issue Apr 30, 2020
This change modifies the semantics of waking the scavenger: rather than
wake on any update to pacing, wake when we know we will have work to do,
that is, when the sweeper is done. The current scavenger runs over the
address space just once per GC cycle, and we want to maximize the chance
that the scavenger observes the most attractive scavengable memory in
that pass (i.e. free memory with the highest address), so the timing is
important. By having the scavenger awaken and reset its search space
when the sweeper is done, we increase the chance that the scavenger will
observe the most attractive scavengable memory, because no more memory
will be freed that GC cycle (so the highest scavengable address should
now be available).

Furthermore, in applications that go idle, this means the background
scavenger will be awoken even if another GC doesn't happen, which isn't
true today.

However, we're unable to wake the scavenger directly from within the
sweeper; waking the scavenger involves modifying timers and readying
goroutines, the latter of which may trigger an allocation today (and the
sweeper may run during allocation!). Instead, we do the following:

1. Set a flag which is checked by sysmon. sysmon will clear the flag and
   wake the scavenger.
2. Wake the scavenger unconditionally at sweep termination.

The idea behind this policy is that it gets us close enough to the state
above without having to deal with the complexity of waking the scavenger
in deep parts of the runtime. If the application goes idle and sweeping
finishes (so we don't reach sweep termination), then sysmon will wake
the scavenger. sysmon has a worst-case 20 ms delay in responding to this
signal, which is probably fine if the application is completely idle
anyway, but if the application is actively allocating, then the
proportional sweeper should help ensure that sweeping ends very close to
sweep termination, so sweep termination is a perfectly reasonable time
to wake up the scavenger.

Updates #35788.

Change-Id: I84289b37816a7d595d803c72a71b7f5c59d47e6b
Reviewed-on: https://go-review.googlesource.com/c/go/+/207998
Run-TryBot: Michael Knyszek <mknyszek@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Austin Clements <austin@google.com>
@gopherbot gopherbot closed this in 55ec518 May 8, 2020
gopherbot pushed a commit that referenced this issue May 8, 2020
Currently the scavenger will reset to the top of the heap every GC. This
means if it scavenges a bunch of memory which doesn't get used again,
it's going to keep re-scanning that memory on subsequent cycles. This
problem is especially bad when it comes to heap spikes: suppose an
application's heap spikes to 2x its steady-state size. The scavenger
will run over the top half of that heap even if the heap shrinks, for
the rest of the application's lifetime.

To fix this, we maintain two numbers: a "free" high watermark, which
represents the highest address freed to the page allocator in that
cycle, and a "scavenged" low watermark, which represents how low of an
address the scavenger got to when scavenging. If the "free" watermark
exceeds the "scavenged" watermark, then we pick the "free" watermark as
the new "top of the heap" for the scavenger when starting the next
scavenger cycle. Otherwise, we have the scavenger pick up where it left
off.

With this mechanism, we only ever re-scan scavenged memory if a random
page gets freed very high up in the heap address space while most of the
action is happening in the lower parts. This case should be exceedingly
unlikely because the page reclaimer walks over the heap from low address
to high addresses, and we use a first-fit address-ordered allocation
policy.

Updates #35788.

Change-Id: Id335603b526ce3a0eb79ef286d1a4e876abc9cab
Reviewed-on: https://go-review.googlesource.com/c/go/+/218997
Run-TryBot: Michael Knyszek <mknyszek@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Austin Clements <austin@google.com>
Reviewed-by: David Chase <drchase@google.com>
gopherbot pushed a commit that referenced this issue May 8, 2020
Currently addrRange and addrRanges operate on real addresses. That is,
the addresses they manipulate don't include arenaBaseOffset. When added
to an address, arenaBaseOffset makes the address space appear contiguous
on platforms where the address space is segmented. While this is
generally OK because even those platforms which have a segmented address
space usually don't give addresses in a different segment, today it
causes a mismatch between the scavenger and the rest of the page
allocator. The scavenger scavenges from the highest addresses first, but
only via real address, whereas the page allocator allocates memory in
offset address order.

So this change makes addrRange and addrRanges, i.e. what the scavenger
operates on, use offset addresses. However, lots of the page allocator
relies on an addrRange containing real addresses.

To make this transition less error-prone, this change introduces a new
type, offAddr, whose purpose is to make offset addresses a distinct
type, so any attempt to trivially mix real and offset addresses will
trigger a compilation error.

This change doesn't attempt to use offAddr in all of the runtime; a
follow-up change will look for and catch remaining uses of an offset
address which doesn't use the type.

Updates #35788.

Change-Id: I991d891ac8ace8339ca180daafdf6b261a4d43d1
Reviewed-on: https://go-review.googlesource.com/c/go/+/230717
Run-TryBot: Michael Knyszek <mknyszek@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Austin Clements <austin@google.com>
gopherbot pushed a commit that referenced this issue May 8, 2020
This change uses the new offAddr type in more parts of the runtime where
we've been implicitly switching from the default address space to a
contiguous view. The purpose of offAddr is to represent addresses in the
contiguous view of the address space, and to make direct computations
between real addresses and offset addresses impossible. This change thus
improves readability in the runtime.

Updates #35788.

Change-Id: I4e1c5fed3ed68aa12f49a42b82eb3f46aba82fc1
Reviewed-on: https://go-review.googlesource.com/c/go/+/230718
Run-TryBot: Michael Knyszek <mknyszek@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Austin Clements <austin@google.com>
xujianhai666 added a commit to xujianhai666/go-1 that referenced this issue May 21, 2020
This change modifies the semantics of waking the scavenger: rather than
wake on any update to pacing, wake when we know we will have work to do,
that is, when the sweeper is done. The current scavenger runs over the
address space just once per GC cycle, and we want to maximize the chance
that the scavenger observes the most attractive scavengable memory in
that pass (i.e. free memory with the highest address), so the timing is
important. By having the scavenger awaken and reset its search space
when the sweeper is done, we increase the chance that the scavenger will
observe the most attractive scavengable memory, because no more memory
will be freed that GC cycle (so the highest scavengable address should
now be available).

Furthermore, in applications that go idle, this means the background
scavenger will be awoken even if another GC doesn't happen, which isn't
true today.

However, we're unable to wake the scavenger directly from within the
sweeper; waking the scavenger involves modifying timers and readying
goroutines, the latter of which may trigger an allocation today (and the
sweeper may run during allocation!). Instead, we do the following:

1. Set a flag which is checked by sysmon. sysmon will clear the flag and
   wake the scavenger.
2. Wake the scavenger unconditionally at sweep termination.

The idea behind this policy is that it gets us close enough to the state
above without having to deal with the complexity of waking the scavenger
in deep parts of the runtime. If the application goes idle and sweeping
finishes (so we don't reach sweep termination), then sysmon will wake
the scavenger. sysmon has a worst-case 20 ms delay in responding to this
signal, which is probably fine if the application is completely idle
anyway, but if the application is actively allocating, then the
proportional sweeper should help ensure that sweeping ends very close to
sweep termination, so sweep termination is a perfectly reasonable time
to wake up the scavenger.

Updates golang#35788.

Change-Id: I84289b37816a7d595d803c72a71b7f5c59d47e6b
Reviewed-on: https://go-review.googlesource.com/c/go/+/207998
Run-TryBot: Michael Knyszek <mknyszek@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Austin Clements <austin@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.