From 3692925c5e7e2f2b7728b8c0559403862d7bc681 Mon Sep 17 00:00:00 2001 From: Josh Bleecher Snyder Date: Wed, 12 Apr 2017 13:23:07 -0700 Subject: [PATCH] cmd/compile: move Text.From.Sym initialization earlier MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit 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 TryBot-Result: Gobot Gobot Reviewed-by: Cherry Zhang --- src/cmd/asm/internal/asm/asm.go | 4 +-- src/cmd/compile/internal/gc/gsubr.go | 39 +++++++++++++--------- src/cmd/compile/internal/gc/pgen.go | 3 ++ src/cmd/compile/internal/gc/sizeof_test.go | 2 +- src/cmd/compile/internal/gc/syntax.go | 2 ++ src/cmd/internal/obj/plist.go | 7 +--- 6 files changed, 33 insertions(+), 24 deletions(-) diff --git a/src/cmd/asm/internal/asm/asm.go b/src/cmd/asm/internal/asm/asm.go index 7e3a8418dd..bf232fbe7f 100644 --- 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 f3aef208d6..bf70cebb75 100644 --- 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 ec6483fe8e..3a993e55d5 100644 --- 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 bea25dde2b..ca5d0cd9ae 100644 --- 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 ef70e4b1f7..8ccebded2a 100644 --- 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 b2acd0f29d..069e954130 100644 --- 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) { -- 2.48.1