cmd/compile: move Text.From.Sym initialization earlier

The initialization of an ATEXT Prog's From.Sym
can race with the assemblers in a concurrent compiler.
CL 40254 contains an initial, failed attempt to
fix that race.

This CL takes a different approach: Rather than
expose an API to initialize the Prog,
expose an API to initialize the Sym.

The initialization of the Sym can then be
moved earlier in the compiler, avoiding the race.

The growth of gc.Func has negligible
performance impact; see below.

Passes toolstash -cmp.

Updates #15756

name       old alloc/op      new alloc/op      delta
Template        38.8MB ± 0%       38.8MB ± 0%    ~     (p=0.968 n=9+10)
Unicode         29.8MB ± 0%       29.8MB ± 0%    ~     (p=0.684 n=10+10)
GoTypes          113MB ± 0%        113MB ± 0%    ~     (p=0.912 n=10+10)
SSA             1.25GB ± 0%       1.25GB ± 0%    ~     (p=0.481 n=10+10)
Flate           25.3MB ± 0%       25.3MB ± 0%    ~     (p=0.105 n=10+10)
GoParser        31.7MB ± 0%       31.8MB ± 0%  +0.09%  (p=0.016 n=8+10)
Reflect         78.3MB ± 0%       78.2MB ± 0%    ~     (p=0.190 n=10+10)
Tar             26.5MB ± 0%       26.6MB ± 0%  +0.13%  (p=0.011 n=10+10)
XML             42.4MB ± 0%       42.4MB ± 0%    ~     (p=0.971 n=10+10)

name       old allocs/op     new allocs/op     delta
Template          378k ± 1%         378k ± 0%    ~     (p=0.315 n=10+9)
Unicode           321k ± 1%         321k ± 0%    ~     (p=0.436 n=10+10)
GoTypes          1.14M ± 0%        1.14M ± 0%    ~     (p=0.079 n=10+9)
SSA              9.70M ± 0%        9.70M ± 0%  -0.04%  (p=0.035 n=10+10)
Flate             233k ± 1%         234k ± 1%    ~     (p=0.529 n=10+10)
GoParser          315k ± 0%         316k ± 0%    ~     (p=0.095 n=9+10)
Reflect           980k ± 0%         980k ± 0%    ~     (p=0.436 n=10+10)
Tar               249k ± 1%         250k ± 0%    ~     (p=0.280 n=10+10)
XML               391k ± 1%         391k ± 1%    ~     (p=0.481 n=10+10)

Change-Id: I3c93033dddd2e1df8cc54a106a6e615d27859e71
Reviewed-on: https://go-review.googlesource.com/40496
Run-TryBot: Josh Bleecher Snyder <josharian@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>

diff --git a/src/cmd/asm/internal/asm/asm.go b/src/cmd/asm/internal/asm/asm.go
index 7e3a8418dd43bc69be1bf37eb4cddb6faf1c5010..bf232fbe7f9fe058c9365d2dd45a4103f24dc506 100644 (file)
--- a/src/cmd/asm/internal/asm/asm.go
+++ b/src/cmd/asm/internal/asm/asm.go
@@ -160,6 +160,7 @@ func (p *Parser) asmText(word string, operands [][]lex.Token) {
                }
                argSize = p.positiveAtoi(op[1].String())
        }
+       p.ctxt.InitTextSym(nameAddr.Sym, int(flag))
        prog := &obj.Prog{
                Ctxt: p.ctxt,
                As:   obj.ATEXT,
@@ -171,9 +172,8 @@ func (p *Parser) asmText(word string, operands [][]lex.Token) {
                        // Argsize set below.
                },
        }
+       nameAddr.Sym.Text = prog
        prog.To.Val = int32(argSize)
-       p.ctxt.InitTextSym(prog, int(flag))
-
        p.append(prog, "", true)
 }
 

diff --git a/src/cmd/compile/internal/gc/gsubr.go b/src/cmd/compile/internal/gc/gsubr.go
index f3aef208d6ae41dcdf9cd28e2ef3be5e98704c91..bf70cebb75431617e338a60123cef6b42d6f938f 100644 (file)
--- a/src/cmd/compile/internal/gc/gsubr.go
+++ b/src/cmd/compile/internal/gc/gsubr.go
@@ -144,34 +144,45 @@ func (pp *Progs) settext(fn *Node) {
        if pp.Text != nil {
                Fatalf("Progs.settext called twice")
        }
-
        ptxt := pp.Prog(obj.ATEXT)
-       if nam := fn.Func.Nname; !isblank(nam) {
+       if fn.Func.lsym != nil {
+               fn.Func.lsym.Text = ptxt
                ptxt.From.Type = obj.TYPE_MEM
                ptxt.From.Name = obj.NAME_EXTERN
-               ptxt.From.Sym = Linksym(nam.Sym)
-               if fn.Func.Pragma&Systemstack != 0 {
-                       ptxt.From.Sym.Set(obj.AttrCFunc, true)
+               ptxt.From.Sym = fn.Func.lsym
+       }
+       pp.Text = ptxt
+}
+
+func (f *Func) initLSym() {
+       if f.lsym != nil {
+               Fatalf("Func.initLSym called twice")
+       }
+
+       if nam := f.Nname; !isblank(nam) {
+               f.lsym = Linksym(nam.Sym)
+               if f.Pragma&Systemstack != 0 {
+                       f.lsym.Set(obj.AttrCFunc, true)
                }
        }
 
        var flag int
-       if fn.Func.Dupok() {
+       if f.Dupok() {
                flag |= obj.DUPOK
        }
-       if fn.Func.Wrapper() {
+       if f.Wrapper() {
                flag |= obj.WRAPPER
        }
-       if fn.Func.NoFramePointer() {
+       if f.NoFramePointer() {
                flag |= obj.NOFRAME
        }
-       if fn.Func.Needctxt() {
+       if f.Needctxt() {
                flag |= obj.NEEDCTXT
        }
-       if fn.Func.Pragma&Nosplit != 0 {
+       if f.Pragma&Nosplit != 0 {
                flag |= obj.NOSPLIT
        }
-       if fn.Func.ReflectMethod() {
+       if f.ReflectMethod() {
                flag |= obj.REFLECTMETHOD
        }
 
@@ -179,15 +190,13 @@ func (pp *Progs) settext(fn *Node) {
        // See test/recover.go for test cases and src/reflect/value.go
        // for the actual functions being considered.
        if myimportpath == "reflect" {
-               switch fn.Func.Nname.Sym.Name {
+               switch f.Nname.Sym.Name {
                case "callReflect", "callMethod":
                        flag |= obj.WRAPPER
                }
        }
 
-       Ctxt.InitTextSym(ptxt, flag)
-
-       pp.Text = ptxt
+       Ctxt.InitTextSym(f.lsym, flag)
 }
 
 func ggloblnod(nam *Node) {

diff --git a/src/cmd/compile/internal/gc/pgen.go b/src/cmd/compile/internal/gc/pgen.go
index ec6483fe8efc404623a2e6f78f0a65bb1c702472..3a993e55d58795562030b8997c3f028233863798 100644 (file)
--- a/src/cmd/compile/internal/gc/pgen.go
+++ b/src/cmd/compile/internal/gc/pgen.go
@@ -297,6 +297,9 @@ func compile(fn *Node) {
        // From this point, there should be no uses of Curfn. Enforce that.
        Curfn = nil
 
+       // Set up the function's LSym early to avoid data races with the assemblers.
+       fn.Func.initLSym()
+
        // Build an SSA backend function.
        ssafn := buildssa(fn)
        pp := newProgs(fn)

diff --git a/src/cmd/compile/internal/gc/sizeof_test.go b/src/cmd/compile/internal/gc/sizeof_test.go
index bea25dde2b01effba1d8ae2910d451d2db1578da..ca5d0cd9aed0d9631df25c57f7e09bab2b72bb16 100644 (file)
--- a/src/cmd/compile/internal/gc/sizeof_test.go
+++ b/src/cmd/compile/internal/gc/sizeof_test.go
@@ -22,7 +22,7 @@ func TestSizeof(t *testing.T) {
                _32bit uintptr     // size on 32bit platforms
                _64bit uintptr     // size on 64bit platforms
        }{
-               {Func{}, 96, 160},
+               {Func{}, 100, 168},
                {Name{}, 36, 56},
                {Param{}, 28, 56},
                {Node{}, 84, 136},

diff --git a/src/cmd/compile/internal/gc/syntax.go b/src/cmd/compile/internal/gc/syntax.go
index ef70e4b1f7a2f03de64904a3b7e6ba1769f1e346..8ccebded2a3881d02c67817963bf296f95ca8a74 100644 (file)
--- a/src/cmd/compile/internal/gc/syntax.go
+++ b/src/cmd/compile/internal/gc/syntax.go
@@ -9,6 +9,7 @@ package gc
 import (
        "cmd/compile/internal/syntax"
        "cmd/compile/internal/types"
+       "cmd/internal/obj"
        "cmd/internal/src"
 )
 
@@ -331,6 +332,7 @@ type Func struct {
        Top        int   // top context (Ecall, Eproc, etc)
        Closure    *Node // OCLOSURE <-> ODCLFUNC
        Nname      *Node
+       lsym       *obj.LSym
 
        Inl     Nodes // copy of the body for use in inlining
        InlCost int32

diff --git a/src/cmd/internal/obj/plist.go b/src/cmd/internal/obj/plist.go
index b2acd0f29d65f3f5b7f3ebad54f97159340e20fe..069e9541306d2e40442b679fad7a6b1cc1d6403e 100644 (file)
--- a/src/cmd/internal/obj/plist.go
+++ b/src/cmd/internal/obj/plist.go
@@ -112,11 +112,7 @@ func Flushplist(ctxt *Link, plist *Plist, newprog ProgAlloc) {
        ctxt.Text = append(ctxt.Text, text...)
 }
 
-func (ctxt *Link) InitTextSym(p *Prog, flag int) {
-       if p.As != ATEXT {
-               ctxt.Diag("InitTextSym non-ATEXT: %v", p)
-       }
-       s := p.From.Sym
+func (ctxt *Link) InitTextSym(s *LSym, flag int) {
        if s == nil {
                // func _() { }
                return
@@ -139,7 +135,6 @@ func (ctxt *Link) InitTextSym(p *Prog, flag int) {
        s.Set(AttrNeedCtxt, flag&NEEDCTXT != 0)
        s.Set(AttrNoFrame, flag&NOFRAME != 0)
        s.Type = STEXT
-       s.Text = p
 }
 
 func (ctxt *Link) Globl(s *LSym, size int64, flag int) {