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: make runtime.GC, debug.FreeOSMemory concurrent #18216
Labels
Milestone
Comments
CL https://golang.org/cl/37516 mentions this issue. |
CL https://golang.org/cl/37518 mentions this issue. |
CL https://golang.org/cl/37520 mentions this issue. |
CL https://golang.org/cl/37519 mentions this issue. |
CL https://golang.org/cl/37716 mentions this issue. |
CL https://golang.org/cl/38951 mentions this issue. |
gopherbot
pushed a commit
that referenced
this issue
Mar 31, 2017
Currently the GC triggering condition is an awkward combination of the gcMode (whether or not it's gcBackgroundMode) and a boolean "forceTrigger" flag. Replace this with a new gcTrigger type that represents the range of transition predicates we need. This has several advantages: 1. We can remove the awkward logic that affects the trigger behavior based on the gcMode. Now gcMode purely controls whether to run a STW GC or not and the gcTrigger controls whether this is a forced GC that cannot be consolidated with other GC cycles. 2. We can lift the time-based triggering logic in sysmon to just another type of GC trigger and move the logic to the trigger test. 3. This sets us up to have a cycle count-based trigger, which we'll use to make runtime.GC trigger concurrent GC with the desired consolidation properties. For #18216. Change-Id: If9cd49349579a548800f5022ae47b8128004bbfc Reviewed-on: https://go-review.googlesource.com/37516 Run-TryBot: Austin Clements <austin@google.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Rick Hudson <rlh@golang.org>
gopherbot
pushed a commit
that referenced
this issue
Mar 31, 2017
Currently freeOSMemory calls gcStart directly, but we really just want it to behave like runtime.GC() and then perform a scavenge, so make it call runtime.GC() rather than gcStart. For #18216. Change-Id: I548ec007afc788e87d383532a443a10d92105937 Reviewed-on: https://go-review.googlesource.com/37518 Run-TryBot: Austin Clements <austin@google.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Rick Hudson <rlh@golang.org>
gopherbot
pushed a commit
that referenced
this issue
Mar 31, 2017
Currently gcMode != gcBackgroundMode implies this was a user-forced GC cycle. This is no longer going to be true when we make runtime.GC() trigger a concurrent GC, so replace this with an explicit work.userForced bit. For #18216. Change-Id: If7d71bbca78b5f0b35641b070f9d457f5c9a52bd Reviewed-on: https://go-review.googlesource.com/37519 Run-TryBot: Austin Clements <austin@google.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Rick Hudson <rlh@golang.org>
gopherbot
pushed a commit
that referenced
this issue
Mar 31, 2017
Forced GCs don't provide good information about how to adjust the GC trigger. Currently we avoid adjusting the trigger on forced GC because forced GC is also STW and we don't adjust the trigger on STW GC. However, this will become a problem when forced GC is concurrent. Fix this by skipping trigger adjustment if the GC was user-forced. For #18216. Change-Id: I03dfdad12ecd3cfeca4573140a0768abb29aac5e Reviewed-on: https://go-review.googlesource.com/38951 Run-TryBot: Austin Clements <austin@google.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Rick Hudson <rlh@golang.org>
gopherbot
pushed a commit
that referenced
this issue
Mar 31, 2017
sweepone returns ^uintptr(0) when there are no more spans to *start* sweeping, but there may be spans being swept concurrently at the time and there's currently no efficient way to tell when the sweeper is done sweeping all the spans. We'll need this for concurrent runtime.GC(), so add a count of the number of active sweepone calls to make it possible to block until sweeping is truly done. This is also useful for more accurately printing the gcpacertrace, since that should be printed after all of the sweeping stats are in (currently we can print it slightly too early). For #18216. Change-Id: I06e6240c9e7b40aca6fd7b788bb6962107c10a0f Reviewed-on: https://go-review.googlesource.com/37716 Run-TryBot: Austin Clements <austin@google.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Rick Hudson <rlh@golang.org>
lparth
pushed a commit
to lparth/go
that referenced
this issue
Apr 13, 2017
Currently the GC triggering condition is an awkward combination of the gcMode (whether or not it's gcBackgroundMode) and a boolean "forceTrigger" flag. Replace this with a new gcTrigger type that represents the range of transition predicates we need. This has several advantages: 1. We can remove the awkward logic that affects the trigger behavior based on the gcMode. Now gcMode purely controls whether to run a STW GC or not and the gcTrigger controls whether this is a forced GC that cannot be consolidated with other GC cycles. 2. We can lift the time-based triggering logic in sysmon to just another type of GC trigger and move the logic to the trigger test. 3. This sets us up to have a cycle count-based trigger, which we'll use to make runtime.GC trigger concurrent GC with the desired consolidation properties. For golang#18216. Change-Id: If9cd49349579a548800f5022ae47b8128004bbfc Reviewed-on: https://go-review.googlesource.com/37516 Run-TryBot: Austin Clements <austin@google.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Rick Hudson <rlh@golang.org>
lparth
pushed a commit
to lparth/go
that referenced
this issue
Apr 13, 2017
Currently freeOSMemory calls gcStart directly, but we really just want it to behave like runtime.GC() and then perform a scavenge, so make it call runtime.GC() rather than gcStart. For golang#18216. Change-Id: I548ec007afc788e87d383532a443a10d92105937 Reviewed-on: https://go-review.googlesource.com/37518 Run-TryBot: Austin Clements <austin@google.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Rick Hudson <rlh@golang.org>
lparth
pushed a commit
to lparth/go
that referenced
this issue
Apr 13, 2017
Currently gcMode != gcBackgroundMode implies this was a user-forced GC cycle. This is no longer going to be true when we make runtime.GC() trigger a concurrent GC, so replace this with an explicit work.userForced bit. For golang#18216. Change-Id: If7d71bbca78b5f0b35641b070f9d457f5c9a52bd Reviewed-on: https://go-review.googlesource.com/37519 Run-TryBot: Austin Clements <austin@google.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Rick Hudson <rlh@golang.org>
lparth
pushed a commit
to lparth/go
that referenced
this issue
Apr 13, 2017
Forced GCs don't provide good information about how to adjust the GC trigger. Currently we avoid adjusting the trigger on forced GC because forced GC is also STW and we don't adjust the trigger on STW GC. However, this will become a problem when forced GC is concurrent. Fix this by skipping trigger adjustment if the GC was user-forced. For golang#18216. Change-Id: I03dfdad12ecd3cfeca4573140a0768abb29aac5e Reviewed-on: https://go-review.googlesource.com/38951 Run-TryBot: Austin Clements <austin@google.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Rick Hudson <rlh@golang.org>
lparth
pushed a commit
to lparth/go
that referenced
this issue
Apr 13, 2017
sweepone returns ^uintptr(0) when there are no more spans to *start* sweeping, but there may be spans being swept concurrently at the time and there's currently no efficient way to tell when the sweeper is done sweeping all the spans. We'll need this for concurrent runtime.GC(), so add a count of the number of active sweepone calls to make it possible to block until sweeping is truly done. This is also useful for more accurately printing the gcpacertrace, since that should be printed after all of the sweeping stats are in (currently we can print it slightly too early). For golang#18216. Change-Id: I06e6240c9e7b40aca6fd7b788bb6962107c10a0f Reviewed-on: https://go-review.googlesource.com/37716 Run-TryBot: Austin Clements <austin@google.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Rick Hudson <rlh@golang.org>
lparth
pushed a commit
to lparth/go
that referenced
this issue
Apr 13, 2017
Currently runtime.GC() triggers a STW GC. For common uses in tests and benchmarks, it doesn't matter whether it's STW or concurrent, but for uses in servers for things like collecting heap profiles and controlling memory footprint, this pause can be a bit problem for latency. This changes runtime.GC() to trigger a concurrent GC. In order to remain as close as possible to its current meaning, we define it to always perform a full mark/sweep GC cycle before returning (even if that means it has to finish up a cycle we're in the middle of first) and to publish the heap profile as of the triggered mark termination. While it must perform a full cycle, simultaneous runtime.GC() calls can be consolidated into a single full cycle. Fixes golang#18216. Change-Id: I9088cc5deef4ab6bcf0245ed1982a852a01c44b5 Reviewed-on: https://go-review.googlesource.com/37520 Run-TryBot: Austin Clements <austin@google.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Rick Hudson <rlh@golang.org>
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
We've seen servers get utterly wedged because code incorrectly calls runtime.GC or debug.FreeOSMemory. Especially if multiple goroutines all decide this is necessary at the same time and stack up.
The implementations should both (1) use a concurrent GC, blocking of course until the GC is done, and (2) unblock all calls that arrived _before the GC started, so that if you call runtime.GC from 100 goroutines all at once, then you get something like two GCs (whatever starts first, and then the other 99 or so show up during the first GC, which kicks off one more).
The text was updated successfully, but these errors were encountered: