From cc6923e839ff10581223a600fca805203d56acd5 Mon Sep 17 00:00:00 2001 From: Daniel Morsing Date: Mon, 1 Dec 2025 09:40:16 +0000 Subject: [PATCH] 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 Reviewed-by: Dmitri Shuralyov LUCI-TryBot-Result: Go LUCI Reviewed-by: Keith Randall Auto-Submit: Keith Randall --- src/cmd/compile/internal/gc/compile.go | 76 +++++++++----------------- src/cmd/internal/obj/line_test.go | 1 - src/cmd/internal/obj/link.go | 38 ++++++++----- src/cmd/internal/obj/sym.go | 67 +++++++++++++++-------- 4 files changed, 91 insertions(+), 91 deletions(-) diff --git a/src/cmd/compile/internal/gc/compile.go b/src/cmd/compile/internal/gc/compile.go index 1eb4b8cc37..ab9dac2f70 100644 --- a/src/cmd/compile/internal/gc/compile.go +++ b/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 diff --git a/src/cmd/internal/obj/line_test.go b/src/cmd/internal/obj/line_test.go index de7ef1a22e..2cf3d3088b 100644 --- a/src/cmd/internal/obj/line_test.go +++ b/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") diff --git a/src/cmd/internal/obj/link.go b/src/cmd/internal/obj/link.go index c70c1d9438..05ffba11ea 100644 --- a/src/cmd/internal/obj/link.go +++ b/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) { diff --git a/src/cmd/internal/obj/sym.go b/src/cmd/internal/obj/sym.go index 08c50ec72b..acd9d9cf60 100644 --- a/src/cmd/internal/obj/sym.go +++ b/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) { -- 2.52.0