From 91433eb5772ab4aa62efb9f5cde07e4a1556e96e Mon Sep 17 00:00:00 2001 From: Josh Bleecher Snyder Date: Thu, 6 Apr 2017 06:19:56 -0700 Subject: [PATCH] cmd/compile: make typenamesym do less work MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit typenamesym is called from three places: typename, ngotype, and Type.Symbol. Only in typename do we actually need a Node. ngotype and Type.Symbol require only a Sym. And writing the newly created Node to Sym.Def is unsafe in a concurrent backend. Rather than use a mutex protect to Sym.Def, make typenamesym not touch Sym.Def. The assignment to Sym.Def was serving a second purpose, namely to prevent duplicate entries on signatlist. Preserve that functionality by switching signatlist to a map. This in turn requires that we sort signatlist when exporting it, to preserve reproducibility. We'd like to use Type.cmp for sorting, but that causes infinite recursion at the moment; see #19869. For now, use Type.LongString as the sort key, which is a complete description of the type. Type.LongString is relatively expensive, but we calculate it only once per type, and signatlist is generally fairly small, so the performance impact is minimal. Updates #15756 name old alloc/op new alloc/op delta Template 39.4MB ± 0% 39.4MB ± 0% ~ (p=0.222 n=5+5) Unicode 29.8MB ± 0% 29.8MB ± 0% ~ (p=0.151 n=5+5) GoTypes 113MB ± 0% 113MB ± 0% ~ (p=0.095 n=5+5) SSA 1.25GB ± 0% 1.25GB ± 0% +0.04% (p=0.008 n=5+5) Flate 25.3MB ± 0% 25.4MB ± 0% ~ (p=0.056 n=5+5) GoParser 31.8MB ± 0% 31.8MB ± 0% ~ (p=0.310 n=5+5) Reflect 78.3MB ± 0% 78.3MB ± 0% ~ (p=0.690 n=5+5) Tar 26.7MB ± 0% 26.7MB ± 0% ~ (p=0.548 n=5+5) XML 42.2MB ± 0% 42.2MB ± 0% ~ (p=0.222 n=5+5) name old allocs/op new allocs/op delta Template 387k ± 0% 388k ± 0% ~ (p=0.056 n=5+5) Unicode 320k ± 0% 321k ± 0% +0.32% (p=0.032 n=5+5) GoTypes 1.14M ± 0% 1.15M ± 0% ~ (p=0.095 n=5+5) SSA 9.70M ± 0% 9.72M ± 0% +0.18% (p=0.008 n=5+5) Flate 234k ± 0% 235k ± 0% +0.60% (p=0.008 n=5+5) GoParser 317k ± 0% 317k ± 0% ~ (p=1.000 n=5+5) Reflect 982k ± 0% 983k ± 0% ~ (p=0.841 n=5+5) Tar 252k ± 1% 252k ± 0% ~ (p=0.310 n=5+5) XML 393k ± 0% 392k ± 0% ~ (p=0.548 n=5+5) Change-Id: I53a3b95d19cf1a7b7511a94fba896706addf84fb Reviewed-on: https://go-review.googlesource.com/39710 Run-TryBot: Josh Bleecher Snyder TryBot-Result: Gobot Gobot Reviewed-by: Matthew Dempsky --- src/cmd/compile/internal/gc/reflect.go | 60 ++++++++++++++++++-------- src/cmd/compile/internal/gc/type.go | 2 + 2 files changed, 44 insertions(+), 18 deletions(-) diff --git a/src/cmd/compile/internal/gc/reflect.go b/src/cmd/compile/internal/gc/reflect.go index b11ca7082a..815086777a 100644 --- a/src/cmd/compile/internal/gc/reflect.go +++ b/src/cmd/compile/internal/gc/reflect.go @@ -34,7 +34,7 @@ type ptabEntry struct { } // runtime interface and reflection data structures -var signatlist []*Type +var signatlist = make(map[*Type]bool) var itabs []itabEntry var ptabs []ptabEntry @@ -933,24 +933,22 @@ func typesymprefix(prefix string, t *Type) *Sym { func typenamesym(t *Type) *Sym { if t == nil || (t.IsPtr() && t.Elem() == nil) || t.IsUntyped() { - Fatalf("typename %v", t) + Fatalf("typenamesym %v", t) } s := typesym(t) + addsignat(t) + return s +} + +func typename(t *Type) *Node { + s := typenamesym(t) if s.Def == nil { n := newnamel(src.NoXPos, s) n.Type = Types[TUINT8] n.Class = PEXTERN n.Typecheck = 1 s.Def = n - - signatlist = append(signatlist, t) } - - return s.Def.Sym -} - -func typename(t *Type) *Node { - s := typenamesym(t) n := nod(OADDR, s.Def, nil) n.Type = typPtr(s.Def.Type) n.SetAddable(true) @@ -1417,21 +1415,35 @@ func itabsym(it *obj.LSym, offset int64) *obj.LSym { return syms[methodnum] } +func addsignat(t *Type) { + signatlist[t] = true +} + func dumptypestructs() { // copy types from externdcl list to signatlist for _, n := range externdcl { if n.Op == OTYPE { - signatlist = append(signatlist, n.Type) + addsignat(n.Type) } } - // Process signatlist. This can't use range, as entries are - // added to the list while it is being processed. - for i := 0; i < len(signatlist); i++ { - t := signatlist[i] - dtypesym(t) - if t.Sym != nil { - dtypesym(typPtr(t)) + // Process signatlist. Use a loop, as dtypesym adds + // entries to signatlist while it is being processed. + signats := make([]typeAndStr, len(signatlist)) + for len(signatlist) > 0 { + signats = signats[:0] + // Transfer entries to a slice and sort, for reproducible builds. + for t := range signatlist { + signats = append(signats, typeAndStr{t: t, s: t.LongString()}) + delete(signatlist, t) + } + sort.Sort(typesByLongString(signats)) + for _, ts := range signats { + t := ts.t + dtypesym(t) + if t.Sym != nil { + dtypesym(typPtr(t)) + } } } @@ -1532,6 +1544,18 @@ func dumptypestructs() { } } +type typeAndStr struct { + t *Type + s string +} + +// TODO(josharian): simplify this to just use Type.cmp once issue 19869 has been fixed. +type typesByLongString []typeAndStr + +func (a typesByLongString) Len() int { return len(a) } +func (a typesByLongString) Less(i, j int) bool { return a[i].s < a[j].s } +func (a typesByLongString) Swap(i, j int) { a[i], a[j] = a[j], a[i] } + type pkgByPath []*Pkg func (a pkgByPath) Len() int { return len(a) } diff --git a/src/cmd/compile/internal/gc/type.go b/src/cmd/compile/internal/gc/type.go index b741112711..61220648cd 100644 --- a/src/cmd/compile/internal/gc/type.go +++ b/src/cmd/compile/internal/gc/type.go @@ -976,6 +976,8 @@ func (r *Sym) cmpsym(s *Sym) ssa.Cmp { // cmp compares two *Types t and x, returning ssa.CMPlt, // ssa.CMPeq, ssa.CMPgt as tx, for an arbitrary // and optimizer-centric notion of comparison. +// TODO(josharian): make this safe for recursive interface types +// and use in signatlist sorting. See issue 19869. func (t *Type) cmp(x *Type) ssa.Cmp { // This follows the structure of eqtype in subr.go // with two exceptions. -- 2.48.1