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: sysUsed often called on non-scavenged memory #36603

Open
mknyszek opened this issue Jan 16, 2020 · 4 comments
Open

runtime: sysUsed often called on non-scavenged memory #36603

mknyszek opened this issue Jan 16, 2020 · 4 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsFix The path to resolution is known, but the work has not been done. Performance
Milestone

Comments

@mknyszek
Copy link
Contributor

In working on #36507, I noticed that one source of regression on Darwin was that sysUsed, as of Go 1.14, is now called on the whole memory allocation, even if just one page of that is actually scavenged. This behavior was already present in Go 1.11, went away in Go 1.12, and is now back.

On systems where sysUsed is a no-op and we rely on the first access "unscavenging" the page, this has no effect, because we're already only unscavenging exactly what's needed. On systems like Darwin and Windows where the "recommit" step is explicit (i.e. sysUsed is not a no-op) this can have a non-trivial impact on performance, since the kernel probably walks over the unscavenged pages for nothing.

This contributed very slightly to the performance regression in #36218, but not enough to cause it to block the 1.14 release.

The fix here is straight-forward: we just need to lower the call of sysUsed in the page allocator where we have complete information over exactly which pages are scavenged. If a given allocation has >50% of its pages scavenged then we can probably just sysUsed the whole thing to save a bit on syscall overheads. This change also makes sense because we already do sysUnused at a fairly low-level part of the page allocator, so bringing sysUsed down there makes sense.

@mknyszek mknyszek added Performance NeedsFix The path to resolution is known, but the work has not been done. labels Jan 16, 2020
@mknyszek mknyszek added this to the Go1.15 milestone Jan 16, 2020
@mknyszek mknyszek self-assigned this Jan 16, 2020
@odeke-em
Copy link
Member

Hello @mknyszek, thank you for reporting this and for the details! Given that we haven't covered much ground for Go1.15 on this issue, is there something else we can do or shall we move it to Go1.16 and then backport fixes to Go1.14 and Go1.15? Thank you.

@mknyszek
Copy link
Contributor Author

This isn't essential. Let's move it to 1.16. No need to backport, either, the performance regression here is very slight in practice (even in microbenchmarks that specifically seem to trigger bad behavior here), and other improvements probably totally overcame it in practice. Worth fixing, but not with any urgency.

@odeke-em
Copy link
Member

Thank you Michael! Moving it to Go1.16.

@odeke-em odeke-em modified the milestones: Go1.15, Go1.16 May 31, 2020
@odeke-em
Copy link
Member

Howdy @mknyszek! Shall we punt to Go1.17 or Backlog? Thank you, and happy holidays!

@odeke-em odeke-em modified the milestones: Go1.16, Go1.17 Feb 4, 2021
@mknyszek mknyszek modified the milestones: Go1.17, Backlog Apr 14, 2021
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jul 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsFix The path to resolution is known, but the work has not been done. Performance
Projects
Status: Triage Backlog
Development

No branches or pull requests

3 participants