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: high-percentile latency of memory allocations has regressed significantly [1.13 backport] #34556

Closed
mknyszek opened this issue Sep 26, 2019 · 4 comments
Labels
CherryPickApproved Used during the release process for point releases FrozenDueToAge Performance
Milestone

Comments

@mknyszek
Copy link
Contributor

For some reason gopherbot wouldn't open a backport issue for me. This is a backport of #31678.

The fixes for this (once and for all) have landed in master. https://go-review.googlesource.com/c/go/+/183857/9

The importance of this bug for a 1.13 backport is the fact that it's one of the pieces to the fix for #32828 for Go 1.13, which is a major regression in Kubernetes which is blocking them from landing.

There are two problems with Kubernetes: inline scavenging on the allocation path can really spike allocation tail latencies (this issue) and it could spiral into over-scavenging if fragmentation is not accounted for (#34149).

The fixes for both issues must be landed together to completely fix the issue.

@mknyszek mknyszek added Performance CherryPickCandidate Used during the release process for point releases labels Sep 26, 2019
@mknyszek mknyszek added this to the Go1.13.2 milestone Sep 26, 2019
@mknyszek mknyszek self-assigned this Sep 26, 2019
@aclements
Copy link
Member

I agree that we should backport this fix. This and #34149 are blocking k8s from upgrading to Go 1.13.

To be clear, the following two CLs need to be backported for this:
https://go-review.googlesource.com/c/go/+/189957/4
https://go-review.googlesource.com/c/go/+/183857/9

CL 183857 is nearly identical to a change we backported to Go 1.12. CL 189957 is necessary for CL 183857 to be most effective.

@andybons andybons added CherryPickApproved Used during the release process for point releases and removed CherryPickCandidate Used during the release process for point releases labels Oct 2, 2019
@gopherbot
Copy link

Change https://golang.org/cl/198485 mentions this issue: [release-branch.go1.13] runtime: grow the heap incrementally

@gopherbot
Copy link

Change https://golang.org/cl/198486 mentions this issue: [release-branch.go1.13] runtime: scavenge on growth instead of inline with allocation

gopherbot pushed a commit that referenced this issue Oct 4, 2019
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.

Updates #34556

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>
(cherry picked from commit f18109d)
Reviewed-on: https://go-review.googlesource.com/c/go/+/198485
Run-TryBot: Andrew Bonventre <andybons@golang.org>
Reviewed-by: Austin Clements <austin@google.com>
@gopherbot
Copy link

Closed by merging 951dbb1 to release-branch.go1.13.

gopherbot pushed a commit that referenced this issue Oct 4, 2019
… 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 #34556

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>
(cherry picked from commit eb96f8a)
Reviewed-on: https://go-review.googlesource.com/c/go/+/198486
Reviewed-by: Austin Clements <austin@google.com>
Run-TryBot: Andrew Bonventre <andybons@golang.org>
@katiehockman katiehockman modified the milestones: Go1.13.2, Go1.13.3 Oct 17, 2019
@golang golang locked and limited conversation to collaborators Oct 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CherryPickApproved Used during the release process for point releases FrozenDueToAge Performance
Projects
None yet
Development

No branches or pull requests

5 participants