In the original BoringCrypto port, ecdsa and rsa's public and private keys added a 'boring unsafe.Pointer' field to cache the BoringCrypto form of the key. This led to problems with code that “knew” the layout of those structs and in particular that they had no unexported fields. In response, as an awful kludge, I changed the compiler to pretend that field did not exist when laying out reflect data. Because we want to merge BoringCrypto in the main tree, we need a different solution. Using boring.Cache is that solution. For #51940. Change-Id: Ideb2b40b599a1dc223082eda35a5ea9abcc01e30 Reviewed-on: https://go-review.googlesource.com/c/go/+/395883 Run-TryBot: Russ Cox <rsc@golang.org> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Roland Shoemaker <roland@golang.org>
In the original BoringCrypto port, ecdsa and rsa's public and private keys added a 'boring unsafe.Pointer' field to cache the BoringCrypto form of the key. This led to problems with code that “knew” the layout of those structs and in particular that they had no unexported fields. In response, as an awful kludge, I changed the compiler to pretend that field did not exist when laying out reflect data. Because we want to merge BoringCrypto in the main tree, we need a different solution. The different solution is this CL's boring.Cache, which is a concurrent, GC-aware map from unsafe.Pointer to unsafe.Pointer (if generics were farther along we could use them nicely here, but I am afraid of breaking tools that aren't ready to see generics in the standard library yet). More complex approaches are possible, but a simple, fixed-size hash table is easy to make concurrent and should be fine. For #51940. Change-Id: I44062a8defbd87b705a787cffc64c6a9d0132785 Reviewed-on: https://go-review.googlesource.com/c/go/+/395882 Run-TryBot: Russ Cox <rsc@golang.org> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Cherry Mui <cherryyz@google.com>
Change-Id: Ic5f71c04f08c03319c043f35be501875adb0a3b0
This test tends to be flaky on 32-bit systems. There's not enough bits in the hash output, so we expect a nontrivial number of collisions, and it is often quite a bit higher than expected. Fixes #43130 Change-Id: If35413b7c45eed778a08b834dacf98009ceca840 Reviewed-on: https://go-review.googlesource.com/c/go/+/402456 Run-TryBot: Keith Randall <khr@golang.org> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Ian Lance Taylor <iant@google.com> Reviewed-by: Keith Randall <khr@google.com>
This change refactors the scavenger into a type whose methods represent the actual function and scheduling of the scavenger. It also stubs out access to global state in order to make it testable. This change thus also adds a test for the scavenger. In writing this test, I discovered the lack of a behavior I expected: if the pageAlloc.scavenge returns < the bytes requested scavenged, that means the heap is exhausted. This has been true this whole time, but was not documented or explicitly relied upon. This change rectifies that. In theory this means the scavenger could spin in run() indefinitely (as happened in the test) if shouldStop never told it to stop. In practice, shouldStop fires long before the heap is exhausted, but for future changes it may be important. At the very least it's good to be intentional about these things. While we're here, I also moved the call to stopTimer out of wake and into sleep. There's no reason to add more operations to a context that's already precarious (running without a P on sysmon). Change-Id: Ib31b86379fd9df84f25ae282734437afc540da5c Reviewed-on: https://go-review.googlesource.com/c/go/+/384734 Reviewed-by: Michael Pratt <mpratt@google.com> Run-TryBot: Michael Knyszek <mknyszek@google.com> TryBot-Result: Gopher Robot <gobot@golang.org>
This change completes the proposal laid out in #44163. With #44313 resolved, we now ensure that stopped Ms are able to wake up and become dedicated GC workers. As a result, idle GC workers are in theory no longer required to be a proxy for scheduling dedicated mark workers. And, with at least one dedicated mark worker running (which is non-preemptible) we ensure the GC makes progress in all circumstances when at least one is running. Currently we ensure at least one idle mark worker is available at all times because it's possible before #44313 that a dedicated worker doesn't ever get scheduled, leading to a deadlock if user goroutines block on a GC completing. But now that extra idle mark worker should be unnecessary to ensure GC progress when at least one dedicated mark worker is going to be scheduled. Fixes #44163. Change-Id: I62889ef2db4e69d44da883e8e6eebcfe5398c86d Reviewed-on: https://go-review.googlesource.com/c/go/+/395634 Reviewed-by: Michael Pratt <mpratt@google.com> Run-TryBot: Michael Knyszek <mknyszek@google.com> TryBot-Result: Gopher Robot <gobot@golang.org>
This change moves several scheduling decisions made by schedule into findrunnable. The main motivation behind this change is the fact that stopped Ms can't become dedicated or fractional GC workers. The main reason for this is that when a stopped M wakes up, it stays in findrunnable until it finds work, which means it will never consider GC work. On that note, it'll also never consider becoming the trace reader, either. Another way of looking at it is that this change tries to make findrunnable aware of more sources of work than it was before. With this change, any M in findrunnable should be capable of becoming a GC worker, resolving #44313. While we're here, let's also make more sources of work, such as the trace reader, visible to handoffp, which should really be checking all sources of work. With that, we also now correctly handle the case where StopTrace is called from the last live M that is also locked (#39004). stoplockedm calls handoffp to start a new M and handle the work it cannot, and once we include the trace reader in that, we ensure that the trace reader gets scheduled. This change attempts to preserve the exact same ordering of work checking to reduce its impact. One consequence of this change is that upon entering schedule, some sources of work won't be checked twice (i.e. the local and global runqs, and timers) as they do now, which in some sense gives them a lower priority than they had before. Fixes #39004. Fixes #44313. Change-Id: I5d8b7f63839db8d9a3e47cdda604baac1fe615ce Reviewed-on: https://go-review.googlesource.com/c/go/+/393880 Reviewed-by: Michael Pratt <mpratt@google.com> Run-TryBot: Michael Knyszek <mknyszek@google.com> TryBot-Result: Gopher Robot <gobot@golang.org>
This change reduces the maximum number of idle mark workers during periodic (currently every 2 minutes) GC cycles to 1. Idle mark workers soak up all available and unused Ps, up to GOMAXPROCS. While this provides some throughput and latency benefit in general, it can cause what appear to be massive CPU utilization spikes in otherwise idle applications. This is mostly an issue for *very* idle applications, ones idle enough to trigger periodic GC cycles. This spike also tends to interact poorly with auto-scaling systems, as the system might assume the load average is very low and suddenly see a massive burst in activity. The result of this change is not to bring down this 100% (of GOMAXPROCS) CPU utilization spike to 0%, but rather min(25% + 1/GOMAXPROCS*100%, 100%) Idle mark workers also do incur a small latency penalty as they must be descheduled for other work that might pop up. Luckily the runtime is pretty good about getting idle mark workers off of Ps, so in general the latency benefit from shorter GC cycles outweighs this cost. But, the cost is still non-zero and may be more significant in idle applications that aren't invoking assists and write barriers quite as often. We can't completely eliminate idle mark workers because they're currently necessary for GC progress in some circumstances. Namely, they're critical for progress when all we have is fractional workers. If a fractional worker meets its quota, and all user goroutines are blocked directly or indirectly on a GC cycle (via runtime.GOMAXPROCS, or runtime.GC), the program may deadlock without GC workers, since the fractional worker will go to sleep with nothing to wake it. Fixes #37116. For #44163. Change-Id: Ib74793bb6b88d1765c52d445831310b0d11ef423 Reviewed-on: https://go-review.googlesource.com/c/go/+/393394 Reviewed-by: Michael Pratt <mpratt@google.com> Run-TryBot: Michael Knyszek <mknyszek@google.com> TryBot-Result: Gopher Robot <gobot@golang.org>
OpenBSD has a coarse sleep granularity that rounds up to 10 ms increments. This can cause significant STW delays, among other issues. As far as I can tell, there's only 1 tightly timed sleep without an explicit wakeup for which this actually matters. Fixes #52475. Change-Id: Ic69fc11096ddbbafd79b2dcdf3f912fde242db24 Reviewed-on: https://go-review.googlesource.com/c/go/+/401638 Reviewed-by: Matthew Dempsky <mdempsky@google.com> Reviewed-by: Michael Pratt <mpratt@google.com> Run-TryBot: Michael Knyszek <mknyszek@google.com> TryBot-Result: Gopher Robot <gobot@golang.org>
Right now we export alloc count metrics via the runtime/metrics package and mark them as monotonic, but that's not actually true. As an optimization, the runtime assumes a span is always fully allocated before being uncached, and updates the accounting as such. In the rare case that it's wrong, the span has enough information to back out what did not get allocated. This change uses 16 bits of padding in the mspan to house another field that represents the amount of mspan slots filled just as the mspan is cached. This is information is enough to get an exact count, allowing us to make the metrics truly monotonic. Change-Id: Iaff3ca43f8745dc1bbb0232372423e014b89b920 Reviewed-on: https://go-review.googlesource.com/c/go/+/377516 Reviewed-by: Michael Pratt <mpratt@google.com> Run-TryBot: Michael Knyszek <mknyszek@google.com> TryBot-Result: Gopher Robot <gobot@golang.org>