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 <gobot@golang.org>
Run-TryBot: Austin Clements <austin@google.com>
Auto-Submit: Austin Clements <austin@google.com>
Reviewed-by: Michael Knyszek <mknyszek@google.com>

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