cmd/compile: reduce lock/scheduler contention

During the parallel section of compilation, we limit the amount of
parallelism to prevent scheduler churn. We do this with a worker
scheduler, but it does not have insight on when a compilation is blocked
due to lock contention.

The symbol table was protected with a lock. While most lookups were
quick lock->read->unlock affairs, sometimes there would be
initialization logic involved. This caused every lookup to stall,
waiting for init. Since our worker scheduler couldn't see this, it would
not launch new goroutine to "cover" the gap.

Fix by splitting the symbol lock into 2 cases, initialization and
lookup. If symbols need initialization simultaneously, they will wait
for each other, but the common case of looking up a symbol will be
handled by a syncmap. In practice, I have yet to see this lock being
blocked on.

Additionally, get rid of the scheduler goroutine and have each
compilation goroutine grab work from a central queue. When multiple
compilations finished at the same time, the work scheduler would
sometime not get run immediately. This ended up starving the system of
work.

These 2 changes together cuts -1.37% off the build time of typescriptgo
on systems with a lot of cores (specifically, the c3h88 perf builder).

Updates #73044.

Change-Id: I6d4b3be56fd00a4fdd4df132bcbd52e4b2a3e91f
Reviewed-on: https://go-review.googlesource.com/c/go/+/724623
Reviewed-by: Keith Randall <khr@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Keith Randall <khr@golang.org>
Auto-Submit: Keith Randall <khr@golang.org>

src/cmd/compile/internal/gc/compile.go

@@ -142,73 +142,47 @@ func compileFunctions(profile *pgoir.Profile) {
 		// Compile the longest functions first,
 		// since they're most likely to be the slowest.
 		// This helps avoid stragglers.
+		// Since we remove from the end of the slice queue,
+		// that means shortest to longest.
 		slices.SortFunc(compilequeue, func(a, b *ir.Func) int {
-			return cmp.Compare(len(b.Body), len(a.Body))
+			return cmp.Compare(len(a.Body), len(b.Body))
 		})
 	}
 
-	// By default, we perform work right away on the current goroutine
-	// as the solo worker.
-	queue := func(work func(int)) {
-		work(0)
-	}
+	var mu sync.Mutex
+	var wg sync.WaitGroup
+	mu.Lock()
 
-	if nWorkers := base.Flag.LowerC; nWorkers > 1 {
-		// For concurrent builds, we allow the work queue
-		// to grow arbitrarily large, but only nWorkers work items
-		// can be running concurrently.
-		workq := make(chan func(int))
-		done := make(chan int)
+	for workerId := range base.Flag.LowerC {
+		// TODO: replace with wg.Go when the oldest bootstrap has it.
+		// With the current policy, that'd be go1.27.
+		wg.Add(1)
 		go func() {
-			ids := make([]int, nWorkers)
-			for i := range ids {
-				ids[i] = i
-			}
-			var pending []func(int)
+			defer wg.Done()
+			var closures []*ir.Func
 			for {
-				select {
-				case work := <-workq:
-					pending = append(pending, work)
-				case id := <-done:
-					ids = append(ids, id)
-				}
-				for len(pending) > 0 && len(ids) > 0 {
-					work := pending[len(pending)-1]
-					id := ids[len(ids)-1]
-					pending = pending[:len(pending)-1]
-					ids = ids[:len(ids)-1]
-					go func() {
-						work(id)
-						done <- id
-					}()
+				mu.Lock()
+				compilequeue = append(compilequeue, closures...)
+				remaining := len(compilequeue)
+				if remaining == 0 {
+					mu.Unlock()
+					return
 				}
+				fn := compilequeue[len(compilequeue)-1]
+				compilequeue = compilequeue[:len(compilequeue)-1]
+				mu.Unlock()
+				ssagen.Compile(fn, workerId, profile)
+				closures = fn.Closures
 			}
 		}()
-		queue = func(work func(int)) {
-			workq <- work
-		}
-	}
-
-	var wg sync.WaitGroup
-	var compile func([]*ir.Func)
-	compile = func(fns []*ir.Func) {
-		wg.Add(len(fns))
-		for _, fn := range fns {
-			fn := fn
-			queue(func(worker int) {
-				ssagen.Compile(fn, worker, profile)
-				compile(fn.Closures)
-				wg.Done()
-			})
-		}
 	}
 
 	types.CalcSizeDisabled = true // not safe to calculate sizes concurrently
 	base.Ctxt.InParallel = true
 
-	compile(compilequeue)
-	compilequeue = nil
+	mu.Unlock()
 	wg.Wait()
+	compilequeue = nil
 
 	base.Ctxt.InParallel = false
 	types.CalcSizeDisabled = false

src/cmd/internal/obj/line_test.go

@@ -12,7 +12,6 @@ import (
 
 func TestGetFileSymbolAndLine(t *testing.T) {
 	ctxt := new(Link)
-	ctxt.hash = make(map[string]*LSym)
 	ctxt.statichash = make(map[string]*LSym)
 
 	afile := src.NewFileBase("a.go", "a.go")

src/cmd/internal/obj/link.go

@@ -1169,21 +1169,22 @@ type Link struct {
 	Flag_maymorestack    string // If not "", call this function before stack checks
 	Bso                  *bufio.Writer
 	Pathname             string
-	Pkgpath              string           // the current package's import path
-	hashmu               sync.Mutex       // protects hash, funchash
-	hash                 map[string]*LSym // name -> sym mapping
-	funchash             map[string]*LSym // name -> sym mapping for ABIInternal syms
-	statichash           map[string]*LSym // name -> sym mapping for static syms
-	PosTable             src.PosTable
-	InlTree              InlTree // global inlining tree used by gc/inl.go
-	DwFixups             *DwarfFixupTable
-	DwTextCount          int
-	Imports              []goobj.ImportedPkg
-	DiagFunc             func(string, ...any)
-	DiagFlush            func()
-	DebugInfo            func(ctxt *Link, fn *LSym, info *LSym, curfn Func) ([]dwarf.Scope, dwarf.InlCalls)
-	GenAbstractFunc      func(fn *LSym)
-	Errors               int
+	Pkgpath              string // the current package's import path
+
+	hashmu          sync.Mutex       // protects hash, funchash
+	hash            sync.Map         // name(string) -> sym(*syncOnce) mapping
+	funchash        sync.Map         // name(string) -> sym(*syncOnce) mapping for ABIInternal syms
+	statichash      map[string]*LSym // name -> sym mapping for static syms
+	PosTable        src.PosTable
+	InlTree         InlTree // global inlining tree used by gc/inl.go
+	DwFixups        *DwarfFixupTable
+	DwTextCount     int
+	Imports         []goobj.ImportedPkg
+	DiagFunc        func(string, ...any)
+	DiagFlush       func()
+	DebugInfo       func(ctxt *Link, fn *LSym, info *LSym, curfn Func) ([]dwarf.Scope, dwarf.InlCalls)
+	GenAbstractFunc func(fn *LSym)
+	Errors          int
 
 	InParallel    bool // parallel backend phase in effect
 	UseBASEntries bool // use Base Address Selection Entries in location lists and PC ranges
@@ -1217,6 +1218,13 @@ type Link struct {
 	Fingerprint goobj.FingerprintType // fingerprint of symbol indices, to catch index mismatch
 }
 
+// symOnce is a "marker" value for our sync.Maps. We insert it on lookup and
+// use the atomic value to check whether initialization has been completed.
+type symOnce struct {
+	sym    LSym
+	inited atomic.Bool
+}
+
 // Assert to vet's printf checker that Link.DiagFunc is a printf-like.
 func _(ctxt *Link) {
 	ctxt.DiagFunc = func(format string, args ...any) {

src/cmd/internal/obj/sym.go

@@ -42,12 +42,11 @@ import (
 	"log"
 	"math"
 	"sort"
+	"sync"
 )
 
 func Linknew(arch *LinkArch) *Link {
 	ctxt := new(Link)
-	ctxt.hash = make(map[string]*LSym)
-	ctxt.funchash = make(map[string]*LSym)
 	ctxt.statichash = make(map[string]*LSym)
 	ctxt.Arch = arch
 	ctxt.Pathname = objabi.WorkingDir()
@@ -90,28 +89,34 @@ func (ctxt *Link) LookupABI(name string, abi ABI) *LSym {
 // If it does not exist, it creates it and
 // passes it to init for one-time initialization.
 func (ctxt *Link) LookupABIInit(name string, abi ABI, init func(s *LSym)) *LSym {
-	var hash map[string]*LSym
+	var hash *sync.Map
 	switch abi {
 	case ABI0:
-		hash = ctxt.hash
+		hash = &ctxt.hash
 	case ABIInternal:
-		hash = ctxt.funchash
+		hash = &ctxt.funchash
 	default:
 		panic("unknown ABI")
 	}
 
-	ctxt.hashmu.Lock()
-	s := hash[name]
-	if s == nil {
-		s = &LSym{Name: name}
-		s.SetABI(abi)
-		hash[name] = s
-		if init != nil {
-			init(s)
+	c, _ := hash.Load(name)
+	if c == nil {
+		once := &symOnce{
+			sym: LSym{Name: name},
 		}
+		once.sym.SetABI(abi)
+		c, _ = hash.LoadOrStore(name, once)
 	}
-	ctxt.hashmu.Unlock()
-	return s
+	once := c.(*symOnce)
+	if init != nil && !once.inited.Load() {
+		ctxt.hashmu.Lock()
+		if !once.inited.Load() {
+			init(&once.sym)
+			once.inited.Store(true)
+		}
+		ctxt.hashmu.Unlock()
+	}
+	return &once.sym
 }
 
 // Lookup looks up the symbol with name name.
@@ -124,17 +129,31 @@ func (ctxt *Link) Lookup(name string) *LSym {
 // If it does not exist, it creates it and
 // passes it to init for one-time initialization.
 func (ctxt *Link) LookupInit(name string, init func(s *LSym)) *LSym {
-	ctxt.hashmu.Lock()
-	s := ctxt.hash[name]
-	if s == nil {
-		s = &LSym{Name: name}
-		ctxt.hash[name] = s
-		if init != nil {
-			init(s)
+	c, _ := ctxt.hash.Load(name)
+	if c == nil {
+		once := &symOnce{
+			sym: LSym{Name: name},
 		}
+		c, _ = ctxt.hash.LoadOrStore(name, once)
 	}
-	ctxt.hashmu.Unlock()
-	return s
+	once := c.(*symOnce)
+	if init != nil && !once.inited.Load() {
+		// TODO(dmo): some of our init functions modify other fields
+		// in the symbol table. They are only implicitly protected since
+		// we serialize all inits under the hashmu lock.
+		// Consider auditing the functions and have them lock their
+		// concurrent access values explicitly. This would make it possible
+		// to have more than one than one init going at a time (although this might
+		// be a theoretical concern, I have yet to catch this lock actually being waited
+		// on).
+		ctxt.hashmu.Lock()
+		if !once.inited.Load() {
+			init(&once.sym)
+			once.inited.Store(true)
+		}
+		ctxt.hashmu.Unlock()
+	}
+	return &once.sym
 }
 
 func (ctxt *Link) rodataKind() (suffix string, typ objabi.SymKind) {