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: simplify cons/mark smoothing and use the correct trigger point in the cons/mark calculation #53892

Open
mknyszek opened this issue Jul 14, 2022 · 2 comments
Assignees
Labels
compiler/runtime NeedsFix
Milestone

Comments

@mknyszek
Copy link
Contributor

mknyszek commented Jul 14, 2022

In Go 1.19 I tried to switch to using a more correct trigger point in the cons/mark allocation. However, because of how the incorrect trigger point (the precomputed one, gcControllerState.trigger(), as opposed to the actual one, gcControllerState.heapLive at the point the GC is triggered) interacts with the PI controller used to smooth the cons/mark measurements, it turns out to generate a worse result overall.

More context may be found here: #53738 (comment).

I think that a smoothing function so sensitive to past history is a bit of a mistake, and this sort of thing could easily happen again. As a result, I propose fixing all of the above in the next release by:

  • Switching to a simpler smoothing function, like the moving average of cons/mark measurements over the last 2 cycles, then
  • Using the real trigger point as opposed to the precomputed trigger.

In benchmarks, I've observed that the combination of these two:

  • Provides a more stable estimate of the actual cons/mark ratio, making the pacer easier to understand,
  • Is much more accurate at hitting the heap goal, so the GC respects GOGC much more often and reliably, and,
  • Results in a more stable rate of GC assists in the steady-state overall.

Update: It's worth noting that this is all about transient effects in the pacer. In theory if I let the benchmark from #53738 (comment) run much longer this would all wash away, but the transients do also matter, and I think we should switch to something more simple and stable, if less accurate in the long run (the moving average will never find the true steady-state, but I'm starting to think that's OK).

@mknyszek mknyszek self-assigned this Jul 14, 2022
@gopherbot gopherbot added the compiler/runtime label Jul 14, 2022
@mknyszek mknyszek added this to the Go1.20 milestone Jul 14, 2022
@mknyszek mknyszek added NeedsFix and removed compiler/runtime labels Jul 14, 2022
@gopherbot
Copy link

gopherbot commented Jul 14, 2022

Change https://go.dev/cl/417558 mentions this issue: runtime: smooth cons/mark with a moving average and use actual trigger

@mknyszek
Copy link
Contributor Author

mknyszek commented Jul 14, 2022

It's worth noting that this is all about transient effects in the pacer. In theory if I let the benchmark from #53738 (comment) run much longer this would all wash away, but the transients do also matter, and I think we should switch to something more stable.

@mknyszek mknyszek added the compiler/runtime label Aug 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/runtime NeedsFix
Projects
Status: Todo
Development

No branches or pull requests

2 participants