From: Jin Lin Date: Thu, 8 Feb 2024 20:52:07 +0000 (-0800) Subject: cmd/compile: update the incorrect assignment of call site offset. X-Git-Tag: go1.23rc1~1227 X-Git-Url: http://www.git.cypherpunks.su/?a=commitdiff_plain;h=185f31bf303b265b3a7b573a5fca613ca40bf503;p=gostls13.git cmd/compile: update the incorrect assignment of call site offset. The call site calculation in the previous version is incorrect. For the PGO preprocess file, the compiler should directly use the call site offset value. Additionly, this change refactors the preprocess tool to clean up unused fields including startline, the flat and the cum. Change-Id: I7bffed3215d4c016d9a9e4034bfd373bf50ab43f Reviewed-on: https://go-review.googlesource.com/c/go/+/562795 Reviewed-by: Cherry Mui LUCI-TryBot-Result: Go LUCI Reviewed-by: Michael Pratt --- diff --git a/src/cmd/compile/internal/pgo/irgraph.go b/src/cmd/compile/internal/pgo/irgraph.go index 224f14368f..9ed16d224b 100644 --- a/src/cmd/compile/internal/pgo/irgraph.go +++ b/src/cmd/compile/internal/pgo/irgraph.go @@ -108,7 +108,6 @@ type NamedCallEdge struct { CallerName string CalleeName string CallSiteOffset int // Line offset from function start line. - CallStartLine int // Start line of the function. Can be 0 which means missing. } // NamedEdgeMap contains all unique call edges in the profile and their @@ -336,20 +335,19 @@ func createNamedEdgeMapFromPreprocess(r io.Reader) (edgeMap NamedEdgeMap, totalW split := strings.Split(readStr, " ") - if len(split) != 5 { - return NamedEdgeMap{}, 0, fmt.Errorf("preprocessed profile entry got %v want 5 fields", split) + if len(split) != 2 { + return NamedEdgeMap{}, 0, fmt.Errorf("preprocessed profile entry got %v want 2 fields", split) } co, _ := strconv.Atoi(split[0]) - cs, _ := strconv.Atoi(split[1]) namedEdge := NamedCallEdge{ CallerName: callerName, - CallSiteOffset: co - cs, + CalleeName: calleeName, + CallSiteOffset: co, } - namedEdge.CalleeName = calleeName - EWeight, _ := strconv.ParseInt(split[4], 10, 64) + EWeight, _ := strconv.ParseInt(split[1], 10, 64) weight[namedEdge] += EWeight totalWeight += EWeight diff --git a/src/cmd/compile/internal/test/pgo_devirtualize_test.go b/src/cmd/compile/internal/test/pgo_devirtualize_test.go index f451243683..af09107dc0 100644 --- a/src/cmd/compile/internal/test/pgo_devirtualize_test.go +++ b/src/cmd/compile/internal/test/pgo_devirtualize_test.go @@ -19,8 +19,11 @@ type devirtualization struct { callee string } +const profFileName = "devirt.pprof" +const preProfFileName = "devirt.pprof.node_map" + // testPGODevirtualize tests that specific PGO devirtualize rewrites are performed. -func testPGODevirtualize(t *testing.T, dir string, want []devirtualization) { +func testPGODevirtualize(t *testing.T, dir string, want []devirtualization, pgoProfileName string) { testenv.MustHaveGoRun(t) t.Parallel() @@ -45,7 +48,7 @@ go 1.21 } // Build the test with the profile. - pprof := filepath.Join(dir, "devirt.pprof") + pprof := filepath.Join(dir, pgoProfileName) gcflag := fmt.Sprintf("-gcflags=-m=2 -pgoprofile=%s -d=pgodebug=3", pprof) out := filepath.Join(dir, "test.exe") cmd = testenv.CleanCmdEnv(testenv.Command(t, testenv.GoToolPath(t), "test", "-o", out, gcflag, ".")) @@ -126,7 +129,70 @@ func TestPGODevirtualize(t *testing.T) { if err := os.Mkdir(filepath.Join(dir, "mult.pkg"), 0755); err != nil { t.Fatalf("error creating dir: %v", err) } - for _, file := range []string{"devirt.go", "devirt_test.go", "devirt.pprof", filepath.Join("mult.pkg", "mult.go")} { + for _, file := range []string{"devirt.go", "devirt_test.go", profFileName, filepath.Join("mult.pkg", "mult.go")} { + if err := copyFile(filepath.Join(dir, file), filepath.Join(srcDir, file)); err != nil { + t.Fatalf("error copying %s: %v", file, err) + } + } + + want := []devirtualization{ + // ExerciseIface + { + pos: "./devirt.go:101:20", + callee: "mult.Mult.Multiply", + }, + { + pos: "./devirt.go:101:39", + callee: "Add.Add", + }, + // ExerciseFuncConcrete + { + pos: "./devirt.go:173:36", + callee: "AddFn", + }, + { + pos: "./devirt.go:173:15", + callee: "mult.MultFn", + }, + // ExerciseFuncField + { + pos: "./devirt.go:207:35", + callee: "AddFn", + }, + { + pos: "./devirt.go:207:19", + callee: "mult.MultFn", + }, + // ExerciseFuncClosure + // TODO(prattmic): Closure callees not implemented. + //{ + // pos: "./devirt.go:249:27", + // callee: "AddClosure.func1", + //}, + //{ + // pos: "./devirt.go:249:15", + // callee: "mult.MultClosure.func1", + //}, + } + + testPGODevirtualize(t, dir, want, profFileName) +} + +// TestPGOPreprocessDevirtualize tests that specific functions are devirtualized when PGO +// is applied to the exact source that was profiled. The input profile is PGO preprocessed file. +func TestPGOPreprocessDevirtualize(t *testing.T) { + wd, err := os.Getwd() + if err != nil { + t.Fatalf("error getting wd: %v", err) + } + srcDir := filepath.Join(wd, "testdata", "pgo", "devirtualize") + + // Copy the module to a scratch location so we can add a go.mod. + dir := t.TempDir() + if err := os.Mkdir(filepath.Join(dir, "mult.pkg"), 0755); err != nil { + t.Fatalf("error creating dir: %v", err) + } + for _, file := range []string{"devirt.go", "devirt_test.go", preProfFileName, filepath.Join("mult.pkg", "mult.go")} { if err := copyFile(filepath.Join(dir, file), filepath.Join(srcDir, file)); err != nil { t.Fatalf("error copying %s: %v", file, err) } @@ -172,7 +238,7 @@ func TestPGODevirtualize(t *testing.T) { //}, } - testPGODevirtualize(t, dir, want) + testPGODevirtualize(t, dir, want, preProfFileName) } // Regression test for https://go.dev/issue/65615. If a target function changes @@ -190,7 +256,7 @@ func TestLookupFuncGeneric(t *testing.T) { if err := os.Mkdir(filepath.Join(dir, "mult.pkg"), 0755); err != nil { t.Fatalf("error creating dir: %v", err) } - for _, file := range []string{"devirt.go", "devirt_test.go", "devirt.pprof", filepath.Join("mult.pkg", "mult.go")} { + for _, file := range []string{"devirt.go", "devirt_test.go", profFileName, filepath.Join("mult.pkg", "mult.go")} { if err := copyFile(filepath.Join(dir, file), filepath.Join(srcDir, file)); err != nil { t.Fatalf("error copying %s: %v", file, err) } @@ -238,7 +304,7 @@ func TestLookupFuncGeneric(t *testing.T) { //}, } - testPGODevirtualize(t, dir, want) + testPGODevirtualize(t, dir, want, profFileName) } var multFnRe = regexp.MustCompile(`func MultFn\(a, b int64\) int64`) diff --git a/src/cmd/compile/internal/test/pgo_inl_test.go b/src/cmd/compile/internal/test/pgo_inl_test.go index 3aafaee197..7d665655d5 100644 --- a/src/cmd/compile/internal/test/pgo_inl_test.go +++ b/src/cmd/compile/internal/test/pgo_inl_test.go @@ -18,6 +18,9 @@ import ( "testing" ) +const profFile = "inline_hot.pprof" +const preProfFile = "inline_hot.pprof.node_map" + func buildPGOInliningTest(t *testing.T, dir string, gcflag string) []byte { const pkg = "example.com/pgo/inline" @@ -43,12 +46,7 @@ go 1.19 } // testPGOIntendedInlining tests that specific functions are inlined. -func testPGOIntendedInlining(t *testing.T, dir string, preprocessed ...bool) { - defaultPGOPackValue := false - if len(preprocessed) > 0 { - defaultPGOPackValue = preprocessed[0] - } - +func testPGOIntendedInlining(t *testing.T, dir string, profFile string) { testenv.MustHaveGoRun(t) t.Parallel() @@ -91,13 +89,7 @@ func testPGOIntendedInlining(t *testing.T, dir string, preprocessed ...bool) { // Build the test with the profile. Use a smaller threshold to test. // TODO: maybe adjust the test to work with default threshold. - var pprof string - if defaultPGOPackValue == false { - pprof = filepath.Join(dir, "inline_hot.pprof") - } else { - pprof = filepath.Join(dir, "inline_hot.pprof.node_map") - } - gcflag := fmt.Sprintf("-m -m -pgoprofile=%s -d=pgoinlinebudget=160,pgoinlinecdfthreshold=90", pprof) + gcflag := fmt.Sprintf("-m -m -pgoprofile=%s -d=pgoinlinebudget=160,pgoinlinecdfthreshold=90", profFile) out := buildPGOInliningTest(t, dir, gcflag) scanner := bufio.NewScanner(bytes.NewReader(out)) @@ -165,13 +157,13 @@ func TestPGOIntendedInlining(t *testing.T) { // Copy the module to a scratch location so we can add a go.mod. dir := t.TempDir() - for _, file := range []string{"inline_hot.go", "inline_hot_test.go", "inline_hot.pprof"} { + for _, file := range []string{"inline_hot.go", "inline_hot_test.go", profFile} { if err := copyFile(filepath.Join(dir, file), filepath.Join(srcDir, file)); err != nil { t.Fatalf("error copying %s: %v", file, err) } } - testPGOIntendedInlining(t, dir) + testPGOIntendedInlining(t, dir, profFile) } // TestPGOIntendedInlining tests that specific functions are inlined when PGO @@ -186,13 +178,13 @@ func TestPGOPreprocessInlining(t *testing.T) { // Copy the module to a scratch location so we can add a go.mod. dir := t.TempDir() - for _, file := range []string{"inline_hot.go", "inline_hot_test.go", "inline_hot.pprof.node_map"} { + for _, file := range []string{"inline_hot.go", "inline_hot_test.go", preProfFile} { if err := copyFile(filepath.Join(dir, file), filepath.Join(srcDir, file)); err != nil { t.Fatalf("error copying %s: %v", file, err) } } - testPGOIntendedInlining(t, dir, true) + testPGOIntendedInlining(t, dir, preProfFile) } // TestPGOIntendedInlining tests that specific functions are inlined when PGO @@ -208,7 +200,7 @@ func TestPGOIntendedInliningShiftedLines(t *testing.T) { dir := t.TempDir() // Copy most of the files unmodified. - for _, file := range []string{"inline_hot_test.go", "inline_hot.pprof"} { + for _, file := range []string{"inline_hot_test.go", profFile} { if err := copyFile(filepath.Join(dir, file), filepath.Join(srcDir, file)); err != nil { t.Fatalf("error copying %s : %v", file, err) } @@ -240,7 +232,7 @@ func TestPGOIntendedInliningShiftedLines(t *testing.T) { dst.Close() - testPGOIntendedInlining(t, dir) + testPGOIntendedInlining(t, dir, profFile) } // TestPGOSingleIndex tests that the sample index can not be 1 and compilation @@ -270,15 +262,15 @@ func TestPGOSingleIndex(t *testing.T) { // Copy the module to a scratch location so we can add a go.mod. dir := t.TempDir() - originalPprofFile, err := os.Open(filepath.Join(srcDir, "inline_hot.pprof")) + originalPprofFile, err := os.Open(filepath.Join(srcDir, profFile)) if err != nil { - t.Fatalf("error opening inline_hot.pprof: %v", err) + t.Fatalf("error opening %v: %v", profFile, err) } defer originalPprofFile.Close() p, err := profile.Parse(originalPprofFile) if err != nil { - t.Fatalf("error parsing inline_hot.pprof: %v", err) + t.Fatalf("error parsing %v: %v", profFile, err) } // Move the samples count value-type to the 0 index. @@ -289,14 +281,14 @@ func TestPGOSingleIndex(t *testing.T) { s.Value = []int64{s.Value[tc.originalIndex]} } - modifiedPprofFile, err := os.Create(filepath.Join(dir, "inline_hot.pprof")) + modifiedPprofFile, err := os.Create(filepath.Join(dir, profFile)) if err != nil { - t.Fatalf("error creating inline_hot.pprof: %v", err) + t.Fatalf("error creating %v: %v", profFile, err) } defer modifiedPprofFile.Close() if err := p.Write(modifiedPprofFile); err != nil { - t.Fatalf("error writing inline_hot.pprof: %v", err) + t.Fatalf("error writing %v: %v", profFile, err) } for _, file := range []string{"inline_hot.go", "inline_hot_test.go"} { @@ -305,7 +297,7 @@ func TestPGOSingleIndex(t *testing.T) { } } - testPGOIntendedInlining(t, dir) + testPGOIntendedInlining(t, dir, profFile) }) } } @@ -343,13 +335,13 @@ func TestPGOHash(t *testing.T) { // Copy the module to a scratch location so we can add a go.mod. dir := t.TempDir() - for _, file := range []string{"inline_hot.go", "inline_hot_test.go", "inline_hot.pprof"} { + for _, file := range []string{"inline_hot.go", "inline_hot_test.go", profFile} { if err := copyFile(filepath.Join(dir, file), filepath.Join(srcDir, file)); err != nil { t.Fatalf("error copying %s: %v", file, err) } } - pprof := filepath.Join(dir, "inline_hot.pprof") + pprof := filepath.Join(dir, profFile) // build with -trimpath so the source location (thus the hash) // does not depend on the temporary directory path. gcflag0 := fmt.Sprintf("-pgoprofile=%s -trimpath %s=>%s -d=pgoinlinebudget=160,pgoinlinecdfthreshold=90,pgodebug=1", pprof, dir, pkg) diff --git a/src/cmd/compile/internal/test/testdata/pgo/devirtualize/devirt.pprof.node_map b/src/cmd/compile/internal/test/testdata/pgo/devirtualize/devirt.pprof.node_map new file mode 100644 index 0000000000..c55f990e84 --- /dev/null +++ b/src/cmd/compile/internal/test/testdata/pgo/devirtualize/devirt.pprof.node_map @@ -0,0 +1,52 @@ +GO PREPROFILE V1 +example.com/pgo/devirtualize.ExerciseFuncClosure +example.com/pgo/devirtualize/mult%2epkg.MultClosure.func1 +18 93 +example.com/pgo/devirtualize.ExerciseIface +example.com/pgo/devirtualize/mult%2epkg.NegMult.Multiply +49 4 +example.com/pgo/devirtualize.ExerciseFuncConcrete +example.com/pgo/devirtualize.AddFn +48 103 +example.com/pgo/devirtualize.ExerciseFuncField +example.com/pgo/devirtualize/mult%2epkg.NegMultFn +23 8 +example.com/pgo/devirtualize.ExerciseFuncField +example.com/pgo/devirtualize/mult%2epkg.MultFn +23 94 +example.com/pgo/devirtualize.ExerciseIface +example.com/pgo/devirtualize/mult%2epkg.Mult.Multiply +49 40 +example.com/pgo/devirtualize.ExerciseIface +example.com/pgo/devirtualize.Add.Add +49 55 +example.com/pgo/devirtualize.ExerciseFuncConcrete +example.com/pgo/devirtualize/mult%2epkg.NegMultFn +48 8 +example.com/pgo/devirtualize.ExerciseFuncClosure +example.com/pgo/devirtualize/mult%2epkg.NegMultClosure.func1 +18 10 +example.com/pgo/devirtualize.ExerciseIface +example.com/pgo/devirtualize.Sub.Add +49 7 +example.com/pgo/devirtualize.ExerciseFuncField +example.com/pgo/devirtualize.AddFn +23 101 +example.com/pgo/devirtualize.ExerciseFuncField +example.com/pgo/devirtualize.SubFn +23 12 +example.com/pgo/devirtualize.BenchmarkDevirtFuncConcrete +example.com/pgo/devirtualize.ExerciseFuncConcrete +1 2 +example.com/pgo/devirtualize.ExerciseFuncConcrete +example.com/pgo/devirtualize/mult%2epkg.MultFn +48 91 +example.com/pgo/devirtualize.ExerciseFuncConcrete +example.com/pgo/devirtualize.SubFn +48 5 +example.com/pgo/devirtualize.ExerciseFuncClosure +example.com/pgo/devirtualize.Add.Add +18 92 +example.com/pgo/devirtualize.ExerciseFuncClosure +example.com/pgo/devirtualize.Sub.Add +18 14 diff --git a/src/cmd/compile/internal/test/testdata/pgo/inline/inline_hot.pprof.node_map b/src/cmd/compile/internal/test/testdata/pgo/inline/inline_hot.pprof.node_map index bc5bc66b61..6e5f937a50 100644 --- a/src/cmd/compile/internal/test/testdata/pgo/inline/inline_hot.pprof.node_map +++ b/src/cmd/compile/internal/test/testdata/pgo/inline/inline_hot.pprof.node_map @@ -1,13 +1,13 @@ GO PREPROFILE V1 example.com/pgo/inline.benchmarkB example.com/pgo/inline.A -18 17 0 1 1 +18 1 example.com/pgo/inline.(*BS).NS example.com/pgo/inline.T -13 53 124 129 2 +8 3 example.com/pgo/inline.(*BS).NS example.com/pgo/inline.T -8 53 124 129 3 +13 2 example.com/pgo/inline.A example.com/pgo/inline.(*BS).NS -7 74 1 130 129 +7 129 diff --git a/src/cmd/preprofile/main.go b/src/cmd/preprofile/main.go index cf747266ca..806f25fee8 100644 --- a/src/cmd/preprofile/main.go +++ b/src/cmd/preprofile/main.go @@ -35,11 +35,11 @@ import ( // Header // caller_name // callee_name -// "call site offset" "caller's start line number" "flat" "cum" "call edge weight" +// "call site offset" "call edge weight" // ... // caller_name // callee_name -// "call site offset" "caller's start line number" "flat" "cum" "call edge weight" +// "call site offset" "call edge weight" func usage() { fmt.Fprintf(os.Stderr, "MUST have (pprof) input file \n") @@ -52,13 +52,6 @@ type NodeMapKey struct { CallerName string CalleeName string CallSiteOffset int // Line offset from function start line. - CallStartLine int // Start line of the function. Can be 0 which means missing. -} - -type Weights struct { - NFlat int64 - NCum int64 - EWeight int64 } func readPprofFile(profileFile string, outputFile string, verbose bool) bool { @@ -101,33 +94,19 @@ func readPprofFile(profileFile string, outputFile string, verbose bool) bool { SampleValue: func(v []int64) int64 { return v[valueIndex] }, }) - nFlat := make(map[string]int64) - nCum := make(map[string]int64) - - // Accummulate weights for the same node. - for _, n := range g.Nodes { - canonicalName := n.Info.Name - nFlat[canonicalName] += n.FlatValue() - nCum[canonicalName] += n.CumValue() - } - - TotalNodeWeight := int64(0) TotalEdgeWeight := int64(0) - NodeMap := make(map[NodeMapKey]*Weights) - NodeWeightMap := make(map[string]int64) + NodeMap := make(map[NodeMapKey]int64) for _, n := range g.Nodes { - TotalNodeWeight += n.FlatValue() canonicalName := n.Info.Name // Create the key to the nodeMapKey. nodeinfo := NodeMapKey{ CallerName: canonicalName, CallSiteOffset: n.Info.Lineno - n.Info.StartLine, - CallStartLine: n.Info.StartLine, } - if nodeinfo.CallStartLine == 0 { + if n.Info.StartLine == 0 { if verbose { log.Println("[PGO] warning: " + canonicalName + " relative line number is missing from the profile") } @@ -137,27 +116,14 @@ func readPprofFile(profileFile string, outputFile string, verbose bool) bool { TotalEdgeWeight += e.WeightValue() nodeinfo.CalleeName = e.Dest.Info.Name if w, ok := NodeMap[nodeinfo]; ok { - w.EWeight += e.WeightValue() + w += e.WeightValue() } else { - weights := new(Weights) - weights.NFlat = nFlat[canonicalName] - weights.NCum = nCum[canonicalName] - weights.EWeight = e.WeightValue() - NodeMap[nodeinfo] = weights + w = e.WeightValue() + NodeMap[nodeinfo] = w } } } - for _, n := range g.Nodes { - lineno := fmt.Sprintf("%v", n.Info.Lineno) - canonicalName := n.Info.Name + "-" + lineno - if _, ok := (NodeWeightMap)[canonicalName]; ok { - (NodeWeightMap)[canonicalName] += n.CumValue() - } else { - (NodeWeightMap)[canonicalName] = n.CumValue() - } - } - var fNodeMap *os.File if outputFile == "" { fNodeMap = os.Stdout @@ -190,16 +156,13 @@ func readPprofFile(profileFile string, outputFile string, verbose bool) bool { line = key.CalleeName + "\n" w.WriteString(line) line = strconv.Itoa(key.CallSiteOffset) - line = line + separator + strconv.Itoa(key.CallStartLine) - line = line + separator + strconv.FormatInt(element.NFlat, 10) - line = line + separator + strconv.FormatInt(element.NCum, 10) - line = line + separator + strconv.FormatInt(element.EWeight, 10) + "\n" + line = line + separator + strconv.FormatInt(element, 10) + "\n" w.WriteString(line) w.Flush() count += 1 } - if TotalNodeWeight == 0 || TotalEdgeWeight == 0 { + if TotalEdgeWeight == 0 { return false }