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 pacing fails to account for fragmentation #34048

Open
mknyszek opened this issue Sep 3, 2019 · 4 comments

Comments

@mknyszek
Copy link
Contributor

commented Sep 3, 2019

Currently the scavenger's goal is set to 1.1 * next_gc, and this goal is frequently compared against heap_sys - heap_released. Unfortunately this is somewhat of an apples-to-oranges comparison because next_gc is in terms of bytes of objects whereas the latter is in terms of bytes of pages. While pages contain objects, there could be some degree of fragmentation.

If this fragmentation is greater than 10% (i.e. exceeds the 1.1 factor above) it's possible that the scavenger might always think is has work to do, and it could end up over-scavenging significantly, leading to every new page-level allocation causing a page fault, only to be scavenged again immediately. In fact, we've seen exactly that with some internal code.

The clearest fix to me is to change the pacing to account for this fragmentation. Based on @aclements' advice, it probably makes more sense to just make heap_inuse at the end of the last GC (i.e. at mark termination) the basis for our goal, which is more of an apples-to-apples comparison. However, this loses the property of tracking next_gc, because we want to take advantage of knowing how much the heap will grow or shrink. In order to make this happen, we could take the ratio between the previous heap goal and the current heap goal, and multiply that by heap_inuse to obtain a goal. Of course, this makes the assumption that fragmentation will be relatively steady such that heap_inuse tracks the heap goal, but this assumes a steady-state degree of fragmentation which is a generally reasonable assumption to make.

@mknyszek mknyszek added this to the Go1.14 milestone Sep 3, 2019

@mknyszek mknyszek self-assigned this Sep 3, 2019

@gopherbot

This comment has been minimized.

Copy link

commented Sep 3, 2019

Change https://golang.org/cl/193040 mentions this issue: runtime: redefine scavenge goal in terms of heap_inuse

@mknyszek

This comment has been minimized.

Copy link
Contributor Author

commented Sep 6, 2019

Since this has the potential to cause a severe performance loss, I think we should backport this to Go 1.13.

@gopherbot Please open a backport issue for 1.13.

@gopherbot

This comment has been minimized.

Copy link

commented Sep 6, 2019

Backport issue(s) opened: #34149 (for 1.13).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases.

@mknyszek

This comment has been minimized.

Copy link
Contributor Author

commented Sep 6, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.