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 freezes up in Go 1.14 in Windows due to coarse time granularity [1.14 backport] #38856

Closed
gopherbot opened this issue May 4, 2020 · 6 comments
Assignees
Milestone

Comments

@gopherbot
Copy link

@gopherbot gopherbot commented May 4, 2020

@networkimprov requested issue #38617 to be considered for backport to the next 1.14 minor release.

@gopherbot please backport to 1.14; this is a regression.

@dmitshur
Copy link
Member

@dmitshur dmitshur commented May 6, 2020

@mknyszek noted in #38617 (comment) that there is a workaround available:

So, this is a real bug. I'm not sure what the fix is, and it may be worth back-porting to 1.14? There is a workaround, which is to call debug.FreeOSMemory occasionally.

It's not a great experience, but it is worth considering.

/cc @mknyszek @aclements Now that a fix is available, do you have thoughts on how safe it would be to backport this to 1.14?

@aclements
Copy link
Member

@aclements aclements commented May 7, 2020

It's a pretty simple fix. It seems worth back-porting to me. @mknyszek ?

@mknyszek
Copy link
Contributor

@mknyszek mknyszek commented May 7, 2020

Backporting is fine with me, the change is not very risky.

@mknyszek mknyszek self-assigned this May 7, 2020
@gopherbot
Copy link
Author

@gopherbot gopherbot commented May 7, 2020

Change https://golang.org/cl/232743 mentions this issue: [release-branch.go1.14] runtime: make the scavenger's pacing logic more defensive

@dmitshur
Copy link
Member

@dmitshur dmitshur commented May 7, 2020

Approved per discussion in a release meeting. This is a regression affecting a first class port, the workaround isn't very accessible, and the fix is not high-risk.

@gopherbot
Copy link
Author

@gopherbot gopherbot commented May 7, 2020

Closed by merging b7eca1c to release-branch.go1.14.

@gopherbot gopherbot closed this May 7, 2020
gopherbot pushed a commit that referenced this issue May 7, 2020
…re defensive

This change adds two bits of logic to the scavenger's pacing. Firstly,
it checks to make sure we scavenged at least one physical page, if we
released a non-zero amount of memory. If we try to release less than one
physical page, most systems will release the whole page, which could
lead to memory corruption down the road, and this is a signal we're in
this situation.

Secondly, the scavenger's pacing logic now checks to see if the time a
scavenging operation takes is measured to be exactly zero or negative.
The exact zero case can happen if time update granularity is too large
to effectively capture the time the scavenging operation took, like on
Windows where the OS timer frequency is generally 1ms. The negative case
should not happen, but we're being defensive (against kernel bugs, bugs
in the runtime, etc.). If either of these cases happen, we fall back to
Go 1.13 behavior: assume the scavenge operation took around 10µs per
physical page. We ignore huge pages in this case because we're in
unknown territory, so we choose to be conservative about pacing (huge
pages could only increase the rate of scavenging).

Currently, the scavenger is broken on Windows because the granularity of
time measurement is around 1 ms, which is too coarse to measure how fast
we're scavenging, so we often end up with a scavenging time of zero,
followed by NaNs and garbage values in the pacing logic, which usually
leads to the scavenger sleeping forever.

For #38617.
Fixes #38856.

Change-Id: Iaaa2a4cbb21338e1258d010f7362ed58b7db1af7
Reviewed-on: https://go-review.googlesource.com/c/go/+/229997
Run-TryBot: Michael Knyszek <mknyszek@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: David Chase <drchase@google.com>
Reviewed-by: Austin Clements <austin@google.com>
(cherry picked from commit c791537)
Reviewed-on: https://go-review.googlesource.com/c/go/+/232743
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.