]> Cypherpunks repositories - gostls13.git/commitdiff
cmd/compile: don't export unreachable inline method bodies
authorMatthew Dempsky <mdempsky@google.com>
Fri, 27 Oct 2017 22:36:59 +0000 (15:36 -0700)
committerMatthew Dempsky <mdempsky@google.com>
Tue, 31 Oct 2017 19:12:51 +0000 (19:12 +0000)
Previously, anytime we exported a function or method declaration
(which includes methods for every type transitively exported), we
included the inline function bodies, if any. However, in many cases,
it's impossible (or at least very unlikely) for the importing package
to call the method.

For example:

    package p
    type T int
    func (t T) M() { t.u() }
    func (t T) u() {}
    func (t T) v() {}

T.M and T.u are inlineable, and they're both reachable through calls
to T.M, which is exported. However, t.v is also inlineable, but cannot
be reached.

Exception: if p.T is embedded in another type q.U, p.T.v will be
promoted to q.U.v, and the generated wrapper function could have
inlined the call to p.T.v. However, in practice, this doesn't happen,
and a missed inlining opportunity doesn't affect correctness.

To implement this, this CL introduces an extra flood fill pass before
exporting to mark inline bodies that are actually reachable, so the
exporter can skip over methods like t.v.

This reduces Kubernetes build time (as measured by "time go build -a
k8s.io/kubernetes/cmd/...") on an HP Z620 measurably:

    == before ==
    real    0m44.658s
    user    11m19.136s
    sys     0m53.844s

    == after ==
    real    0m41.702s
    user    10m29.732s
    sys     0m50.908s

It also significantly cuts down the cost of enabling mid-stack
inlining (-l=4):

    == before (-l=4) ==
    real    1m19.236s
    user    20m6.528s
    sys     1m17.328s

    == after (-l=4) ==
    real    0m59.100s
    user    13m12.808s
    sys     0m58.776s

Updates #19348.

Change-Id: Iade58233ca42af823a1630517a53848b5d3c7a7e
Reviewed-on: https://go-review.googlesource.com/74110
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
src/cmd/compile/internal/gc/bexport.go
src/cmd/compile/internal/gc/bimport.go
src/cmd/compile/internal/gc/dcl.go
src/cmd/compile/internal/gc/esc.go
src/cmd/compile/internal/gc/inl.go
src/cmd/compile/internal/gc/syntax.go

index 9950f8b855919d4f0fab03c6be0782013f75e3b5..8b1f8a1b806e00c2904e45fe6b9d10a3b7128824 100644 (file)
@@ -174,6 +174,8 @@ type exporter struct {
        typIndex  map[*types.Type]int
        funcList  []*Func
 
+       marked map[*types.Type]bool // types already seen by markType
+
        // position encoding
        posInfoFormat bool
        prevFile      string
@@ -230,6 +232,23 @@ func export(out *bufio.Writer, trace bool) int {
                p.tracef("\n")
        }
 
+       // Mark all inlineable functions that the importer could call.
+       // This is done by tracking down all inlineable methods
+       // reachable from exported types.
+       p.marked = make(map[*types.Type]bool)
+       for _, n := range exportlist {
+               sym := n.Sym
+               if sym.Exported() {
+                       // Closures are added to exportlist, but with Exported
+                       // already set. The export code below skips over them, so
+                       // we have to here as well.
+                       // TODO(mdempsky): Investigate why. This seems suspicious.
+                       continue
+               }
+               p.markType(asNode(sym.Def).Type)
+       }
+       p.marked = nil
+
        // export objects
        //
        // First, export all exported (package-level) objects; i.e., all objects
@@ -436,6 +455,72 @@ func unidealType(typ *types.Type, val Val) *types.Type {
        return typ
 }
 
+// markType recursively visits types reachable from t to identify
+// functions whose inline bodies may be needed.
+func (p *exporter) markType(t *types.Type) {
+       if p.marked[t] {
+               return
+       }
+       p.marked[t] = true
+
+       // If this is a named type, mark all of its associated
+       // methods. Skip interface types because t.Methods contains
+       // only their unexpanded method set (i.e., exclusive of
+       // interface embeddings), and the switch statement below
+       // handles their full method set.
+       if t.Sym != nil && t.Etype != TINTER {
+               for _, m := range t.Methods().Slice() {
+                       if exportname(m.Sym.Name) {
+                               p.markType(m.Type)
+                       }
+               }
+       }
+
+       // Recursively mark any types that can be produced given a
+       // value of type t: dereferencing a pointer; indexing an
+       // array, slice, or map; receiving from a channel; accessing a
+       // struct field or interface method; or calling a function.
+       //
+       // Notably, we don't mark map key or function parameter types,
+       // because the user already needs some way to construct values
+       // of those types.
+       //
+       // It's not critical for correctness that this algorithm is
+       // perfect. Worst case, we might miss opportunities to inline
+       // some function calls in downstream packages.
+       switch t.Etype {
+       case TPTR32, TPTR64, TARRAY, TSLICE, TCHAN:
+               p.markType(t.Elem())
+
+       case TMAP:
+               p.markType(t.Val())
+
+       case TSTRUCT:
+               for _, f := range t.FieldSlice() {
+                       if exportname(f.Sym.Name) || f.Embedded != 0 {
+                               p.markType(f.Type)
+                       }
+               }
+
+       case TFUNC:
+               // If t is the type of a function or method, then
+               // t.Nname() is its ONAME. Mark its inline body and
+               // any recursively called functions for export.
+               inlFlood(asNode(t.Nname()))
+
+               for _, f := range t.Results().FieldSlice() {
+                       p.markType(f.Type)
+               }
+
+       case TINTER:
+               for _, f := range t.FieldSlice() {
+                       if exportname(f.Sym.Name) {
+                               p.markType(f.Type)
+                       }
+               }
+       }
+}
+
 func (p *exporter) obj(sym *types.Sym) {
        // Exported objects may be from different packages because they
        // may be re-exported via an exported alias or as dependencies in
@@ -505,7 +590,7 @@ func (p *exporter) obj(sym *types.Sym) {
                        p.paramList(sig.Results(), inlineable)
 
                        var f *Func
-                       if inlineable {
+                       if inlineable && asNode(sym.Def).Func.ExportInline() {
                                f = asNode(sym.Def).Func
                                // TODO(gri) re-examine reexportdeplist:
                                // Because we can trivially export types
@@ -591,10 +676,28 @@ func fileLine(n *Node) (file string, line int) {
 }
 
 func isInlineable(n *Node) bool {
-       if exportInlined && n != nil && n.Func != nil && n.Func.Inl.Len() != 0 {
-               // when lazily typechecking inlined bodies, some re-exported ones may not have been typechecked yet.
-               // currently that can leave unresolved ONONAMEs in import-dot-ed packages in the wrong package
-               if Debug_typecheckinl == 0 {
+       if exportInlined && n != nil && n.Func != nil {
+               // When lazily typechecking inlined bodies, some
+               // re-exported ones may not have been typechecked yet.
+               // Currently that can leave unresolved ONONAMEs in
+               // import-dot-ed packages in the wrong package.
+               //
+               // TODO(mdempsky): Having the ExportInline check here
+               // instead of the outer if statement means we end up
+               // exporting parameter names even for functions whose
+               // inline body won't be exported by this package. This
+               // is currently necessary because we might first
+               // import a function/method from a package where it
+               // doesn't need to be re-exported, and then from a
+               // package where it does. If this happens, we'll need
+               // the parameter names.
+               //
+               // We could initially do without the parameter names,
+               // and then fill them in when importing the inline
+               // body. But parameter names are attached to the
+               // function type, and modifying types after the fact
+               // is a little sketchy.
+               if Debug_typecheckinl == 0 && n.Func.ExportInline() {
                        typecheckinl(n)
                }
                return true
@@ -693,7 +796,7 @@ func (p *exporter) typ(t *types.Type) {
                        p.bool(m.Nointerface()) // record go:nointerface pragma value (see also #16243)
 
                        var f *Func
-                       if inlineable {
+                       if inlineable && mfn.Func.ExportInline() {
                                f = mfn.Func
                                reexportdeplist(mfn.Func.Inl)
                        }
index 19b5f5a051e3f69aeca08e41d50bb62576a0634b..2b95ac5375393c6c614c1c101b85a421edfd0127 100644 (file)
@@ -188,7 +188,7 @@ func Import(imp *types.Pkg, in *bufio.Reader) {
                // parameter renaming which doesn't matter if we don't have a body.
 
                inlCost := p.int()
-               if f := p.funcList[i]; f != nil {
+               if f := p.funcList[i]; f != nil && f.Func.Inl.Len() == 0 {
                        // function not yet imported - read body and set it
                        funchdr(f)
                        body := p.stmtList()
@@ -357,12 +357,13 @@ func (p *importer) obj(tag int) {
 
                sig := functypefield(nil, params, result)
                importsym(p.imp, sym, ONAME)
-               if asNode(sym.Def) != nil && asNode(sym.Def).Op == ONAME {
+               if old := asNode(sym.Def); old != nil && old.Op == ONAME {
                        // function was imported before (via another import)
-                       if !eqtype(sig, asNode(sym.Def).Type) {
-                               p.formatErrorf("inconsistent definition for func %v during import\n\t%v\n\t%v", sym, asNode(sym.Def).Type, sig)
+                       if !eqtype(sig, old.Type) {
+                               p.formatErrorf("inconsistent definition for func %v during import\n\t%v\n\t%v", sym, old.Type, sig)
                        }
-                       p.funcList = append(p.funcList, nil)
+                       n := asNode(old.Type.Nname())
+                       p.funcList = append(p.funcList, n)
                        break
                }
 
@@ -372,6 +373,8 @@ func (p *importer) obj(tag int) {
                p.funcList = append(p.funcList, n)
                importlist = append(importlist, n)
 
+               sig.SetNname(asTypesNode(n))
+
                if Debug['E'] > 0 {
                        fmt.Printf("import [%q] func %v \n", p.imp.Path, n)
                        if Debug['m'] > 2 && n.Func.Inl.Len() != 0 {
@@ -518,17 +521,19 @@ func (p *importer) typ() *types.Type {
                        nointerface := p.bool()
 
                        mt := functypefield(recv[0], params, result)
-                       addmethod(sym, mt, false, nointerface)
+                       oldm := addmethod(sym, mt, false, nointerface)
 
                        if dup {
                                // An earlier import already declared this type and its methods.
                                // Discard the duplicate method declaration.
-                               p.funcList = append(p.funcList, nil)
+                               n := asNode(oldm.Type.Nname())
+                               p.funcList = append(p.funcList, n)
                                continue
                        }
 
                        n := newfuncnamel(mpos, methodname(sym, recv[0].Type))
                        n.Type = mt
+                       n.SetClass(PFUNC)
                        checkwidth(n.Type)
                        p.funcList = append(p.funcList, n)
                        importlist = append(importlist, n)
@@ -538,7 +543,7 @@ func (p *importer) typ() *types.Type {
                        // (dotmeth's type).Nname.Inl, and dotmeth's type has been pulled
                        // out by typecheck's lookdot as this $$.ttype. So by providing
                        // this back link here we avoid special casing there.
-                       n.Type.FuncType().Nname = asTypesNode(n)
+                       mt.SetNname(asTypesNode(n))
 
                        if Debug['E'] > 0 {
                                fmt.Printf("import [%q] meth %v \n", p.imp.Path, n)
index f99c89d66728b3551973d6f312ae5dc5a9daefb9..b39bdb5aa035a6c7585483423258928ed896ba3c 100644 (file)
@@ -936,7 +936,8 @@ func methodname(s *types.Sym, recv *types.Type) *types.Sym {
 // Add a method, declared as a function.
 // - msym is the method symbol
 // - t is function type (with receiver)
-func addmethod(msym *types.Sym, t *types.Type, local, nointerface bool) {
+// Returns a pointer to the existing or added Field.
+func addmethod(msym *types.Sym, t *types.Type, local, nointerface bool) *types.Field {
        if msym == nil {
                Fatalf("no method symbol")
        }
@@ -945,7 +946,7 @@ func addmethod(msym *types.Sym, t *types.Type, local, nointerface bool) {
        rf := t.Recv() // ptr to this structure
        if rf == nil {
                yyerror("missing receiver")
-               return
+               return nil
        }
 
        mt := methtype(rf.Type)
@@ -955,7 +956,7 @@ func addmethod(msym *types.Sym, t *types.Type, local, nointerface bool) {
                if t != nil && t.IsPtr() {
                        if t.Sym != nil {
                                yyerror("invalid receiver type %v (%v is a pointer type)", pa, t)
-                               return
+                               return nil
                        }
                        t = t.Elem()
                }
@@ -974,23 +975,23 @@ func addmethod(msym *types.Sym, t *types.Type, local, nointerface bool) {
                        // but just in case, fall back to generic error.
                        yyerror("invalid receiver type %v (%L / %L)", pa, pa, t)
                }
-               return
+               return nil
        }
 
        if local && mt.Sym.Pkg != localpkg {
                yyerror("cannot define new methods on non-local type %v", mt)
-               return
+               return nil
        }
 
        if msym.IsBlank() {
-               return
+               return nil
        }
 
        if mt.IsStruct() {
                for _, f := range mt.Fields().Slice() {
                        if f.Sym == msym {
                                yyerror("type %v has both field and method named %v", mt, msym)
-                               return
+                               return nil
                        }
                }
        }
@@ -1004,7 +1005,7 @@ func addmethod(msym *types.Sym, t *types.Type, local, nointerface bool) {
                if !eqtype(t, f.Type) || !eqtype(t.Recv().Type, f.Type.Recv().Type) {
                        yyerror("method redeclared: %v.%v\n\t%v\n\t%v", mt, msym, f.Type, t)
                }
-               return
+               return f
        }
 
        f := types.NewField()
@@ -1014,6 +1015,7 @@ func addmethod(msym *types.Sym, t *types.Type, local, nointerface bool) {
        f.SetNointerface(nointerface)
 
        mt.Methods().Append(f)
+       return f
 }
 
 func funccompile(n *Node) {
index b420c976666f5b434934bbd65af69664fda8846b..d46a42239e57f484826868fede82a4904a44fdf5 100644 (file)
@@ -131,10 +131,7 @@ func (v *bottomUpVisitor) visitcode(n *Node, min uint32) uint32 {
 
        switch n.Op {
        case OCALLFUNC, OCALLMETH:
-               fn := n.Left
-               if n.Op == OCALLMETH {
-                       fn = asNode(n.Left.Sym.Def)
-               }
+               fn := asNode(n.Left.Type.Nname())
                if fn != nil && fn.Op == ONAME && fn.Class() == PFUNC && fn.Name.Defn != nil {
                        m := v.visit(fn.Name.Defn)
                        if m < min {
index 1b52acde3a1e5d9c6a1eea04e799d761862583a0..a509d2d64824a2785d54701683a17fa2df45be51 100644 (file)
@@ -200,6 +200,43 @@ func caninl(fn *Node) {
        Curfn = savefn
 }
 
+// inlFlood marks n's inline body for export and recursively ensures
+// all called functions are marked too.
+func inlFlood(n *Node) {
+       if n == nil {
+               return
+       }
+       if n.Op != ONAME || n.Class() != PFUNC {
+               Fatalf("inlFlood: unexpected %v, %v, %v", n, n.Op, n.Class())
+       }
+       if n.Func == nil {
+               // TODO(mdempsky): Should init have a Func too?
+               if n.Sym.Name == "init" {
+                       return
+               }
+               Fatalf("inlFlood: missing Func on %v", n)
+       }
+       if n.Func.Inl.Len() == 0 {
+               return
+       }
+
+       if n.Func.ExportInline() {
+               return
+       }
+       n.Func.SetExportInline(true)
+
+       typecheckinl(n)
+
+       // Recursively flood any functions called by this one.
+       inspectList(n.Func.Inl, func(n *Node) bool {
+               switch n.Op {
+               case OCALLFUNC, OCALLMETH:
+                       inlFlood(asNode(n.Left.Type.Nname()))
+               }
+               return true
+       })
+}
+
 // hairyVisitor visits a function body to determine its inlining
 // hairiness and whether or not it can be inlined.
 type hairyVisitor struct {
index e28f8a0df3b6c0c3d995409594d3a87b477800e2..be255fb4cc6843d47c884f28536f1c237aaf84d2 100644 (file)
@@ -462,6 +462,7 @@ const (
        funcHasDefer            // contains a defer statement
        funcNilCheckDisabled    // disable nil checks when compiling this function
        funcInlinabilityChecked // inliner has already determined whether the function is inlinable
+       funcExportInline        // include inline body in export data
 )
 
 func (f *Func) Dupok() bool               { return f.flags&funcDupok != 0 }
@@ -473,6 +474,7 @@ func (f *Func) NoFramePointer() bool      { return f.flags&funcNoFramePointer !=
 func (f *Func) HasDefer() bool            { return f.flags&funcHasDefer != 0 }
 func (f *Func) NilCheckDisabled() bool    { return f.flags&funcNilCheckDisabled != 0 }
 func (f *Func) InlinabilityChecked() bool { return f.flags&funcInlinabilityChecked != 0 }
+func (f *Func) ExportInline() bool        { return f.flags&funcExportInline != 0 }
 
 func (f *Func) SetDupok(b bool)               { f.flags.set(funcDupok, b) }
 func (f *Func) SetWrapper(b bool)             { f.flags.set(funcWrapper, b) }
@@ -483,6 +485,7 @@ func (f *Func) SetNoFramePointer(b bool)      { f.flags.set(funcNoFramePointer,
 func (f *Func) SetHasDefer(b bool)            { f.flags.set(funcHasDefer, b) }
 func (f *Func) SetNilCheckDisabled(b bool)    { f.flags.set(funcNilCheckDisabled, b) }
 func (f *Func) SetInlinabilityChecked(b bool) { f.flags.set(funcInlinabilityChecked, b) }
+func (f *Func) SetExportInline(b bool)        { f.flags.set(funcExportInline, b) }
 
 func (f *Func) setWBPos(pos src.XPos) {
        if Debug_wb != 0 {