From 4a8373c553e8a3b15177f74ac12240bbe320a7ca Mon Sep 17 00:00:00 2001 From: Austin Clements Date: Tue, 1 Aug 2023 13:54:32 -0400 Subject: [PATCH] runtime: move pcvalue cache to M Currently, the pcvalue cache is stack allocated for each operation that needs to look up a lot of pcvalues. It's not always clear where to put it, a lot of the time we just pass a nil cache, it doesn't get reused across operations, and we put a surprising amount of effort into threading these caches around. This CL moves it to the M, where it can be long-lived and used by all pcvalue lookups, and we don't have to carefully thread it across operations. This is a re-roll of CL 515276 with a fix for reentrant use of the pcvalue cache from the signal handler. Change-Id: Id94c0c0fb3004d1fda1b196790eebd949c621f28 Reviewed-on: https://go-review.googlesource.com/c/go/+/520063 TryBot-Result: Gopher Robot Run-TryBot: Austin Clements Auto-Submit: Austin Clements Reviewed-by: Michael Knyszek --- src/runtime/runtime2.go | 3 ++ src/runtime/symtab.go | 84 ++++++++++++++++++++++++++--------------- 2 files changed, 56 insertions(+), 31 deletions(-) diff --git a/src/runtime/runtime2.go b/src/runtime/runtime2.go index 885b493bad..c3a3679302 100644 --- a/src/runtime/runtime2.go +++ b/src/runtime/runtime2.go @@ -618,6 +618,9 @@ type m struct { // Whether this is a pending preemption signal on this M. signalPending atomic.Uint32 + // pcvalue lookup cache + pcvalueCache pcvalueCache + dlogPerM mOS diff --git a/src/runtime/symtab.go b/src/runtime/symtab.go index d828c37a75..d8ee8ac70b 100644 --- a/src/runtime/symtab.go +++ b/src/runtime/symtab.go @@ -823,6 +823,7 @@ func (s srcFunc) name() string { type pcvalueCache struct { entries [2][8]pcvalueCacheEnt + inUse int } type pcvalueCacheEnt struct { @@ -843,7 +844,7 @@ func pcvalueCacheKey(targetpc uintptr) uintptr { } // Returns the PCData value, and the PC where this value starts. -func pcvalue(f funcInfo, off uint32, targetpc uintptr, cache *pcvalueCache, strict bool) (int32, uintptr) { +func pcvalue(f funcInfo, off uint32, targetpc uintptr, _ *pcvalueCache, strict bool) (int32, uintptr) { // If true, when we get a cache hit, still look up the data and make sure it // matches the cached contents. const debugCheckCache = false @@ -853,31 +854,46 @@ func pcvalue(f funcInfo, off uint32, targetpc uintptr, cache *pcvalueCache, stri } // Check the cache. This speeds up walks of deep stacks, which - // tend to have the same recursive functions over and over. - // - // This cache is small enough that full associativity is - // cheaper than doing the hashing for a less associative - // cache. + // tend to have the same recursive functions over and over, + // or repetitive stacks between goroutines. var checkVal int32 var checkPC uintptr - if cache != nil { - x := pcvalueCacheKey(targetpc) - for i := range cache.entries[x] { - // We check off first because we're more - // likely to have multiple entries with - // different offsets for the same targetpc - // than the other way around, so we'll usually - // fail in the first clause. - ent := &cache.entries[x][i] - if ent.off == off && ent.targetpc == targetpc { - if debugCheckCache { - checkVal, checkPC = ent.val, ent.valPC - break - } else { - return ent.val, ent.valPC + ck := pcvalueCacheKey(targetpc) + { + mp := acquirem() + cache := &mp.pcvalueCache + // The cache can be used by the signal handler on this M. Avoid + // re-entrant use of the cache. The signal handler can also write inUse, + // but will always restore its value, so we can use a regular increment + // even if we get signaled in the middle of it. + cache.inUse++ + if cache.inUse == 1 { + for i := range cache.entries[ck] { + // We check off first because we're more + // likely to have multiple entries with + // different offsets for the same targetpc + // than the other way around, so we'll usually + // fail in the first clause. + ent := &cache.entries[ck][i] + if ent.off == off && ent.targetpc == targetpc { + val, pc := ent.val, ent.valPC + if debugCheckCache { + checkVal, checkPC = ent.val, ent.valPC + break + } else { + cache.inUse-- + releasem(mp) + return val, pc + } } } + } else if debugCheckCache && (cache.inUse < 1 || cache.inUse > 2) { + // Catch accounting errors or deeply reentrant use. In principle + // "inUse" should never exceed 2. + throw("cache.inUse out of range") } + cache.inUse-- + releasem(mp) } if !f.valid() { @@ -910,17 +926,23 @@ func pcvalue(f funcInfo, off uint32, targetpc uintptr, cache *pcvalueCache, stri print("runtime: table value ", val, "@", prevpc, " != cache value ", checkVal, "@", checkPC, " at PC ", targetpc, " off ", off, "\n") throw("bad pcvalue cache") } - } else if cache != nil { - x := pcvalueCacheKey(targetpc) - e := &cache.entries[x] - ci := fastrandn(uint32(len(cache.entries[x]))) - e[ci] = e[0] - e[0] = pcvalueCacheEnt{ - targetpc: targetpc, - off: off, - val: val, - valPC: prevpc, + } else { + mp := acquirem() + cache := &mp.pcvalueCache + cache.inUse++ + if cache.inUse == 1 { + e := &cache.entries[ck] + ci := fastrandn(uint32(len(cache.entries[ck]))) + e[ci] = e[0] + e[0] = pcvalueCacheEnt{ + targetpc: targetpc, + off: off, + val: val, + valPC: prevpc, + } } + cache.inUse-- + releasem(mp) } return val, prevpc -- 2.50.0