Skip to content

Commit

Permalink
rename bad funcs instead of removing their samples
Browse files Browse the repository at this point in the history
Removing the samples for functions that have known inlining issues skews
the PGO profile and might cause previously cold functions to be promoted
to hot functions.

It's probably not a big issue, but this is an easy improvement to make,
so why not.

Kudos to Michael Pratt for pointing this out pointing this out in
#3 (comment)
  • Loading branch information
felixge committed Jun 6, 2024
1 parent 60fb0c2 commit b21d138
Show file tree
Hide file tree
Showing 2 changed files with 14 additions and 9 deletions.
22 changes: 13 additions & 9 deletions noinline.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,30 +6,34 @@ import (
"github.com/google/pprof/profile"
)

const grpcProcessDataFunc = "google.golang.org/grpc/internal/transport.(*loopyWriter).processData"
const (
grpcProcessDataFunc = "google.golang.org/grpc/internal/transport.(*loopyWriter).processData"
doNotInlinePrefix = "DO NOT INLINE: "
)

// ApplyNoInlineHack removes problematic samples from the profile to avoid bad
// ApplyNoInlineHack renames problematic functions in the profile to avoid bad
// inlining decisions that can have a large memory impact.
// See https://github.com/golang/go/issues/65532 for details.
//
// TODO: Delete this once it's fixed upstream.
func ApplyNoInlineHack(prof *profile.Profile) error {
if err := removeLeafSamples(prof, []string{grpcProcessDataFunc}); err != nil {
if err := renameNoInlineFuncs(prof, []string{grpcProcessDataFunc}); err != nil {
return fmt.Errorf("noinline hack: %w", err)
}
return nil
}

func removeLeafSamples(prof *profile.Profile, funcs []string) error {
newSamples := make([]*profile.Sample, 0, len(prof.Sample))
func renameNoInlineFuncs(prof *profile.Profile, noInlineFuncs []string) error {
for _, s := range prof.Sample {
leaf, ok := leafLine(s)
if ok && lineContainsAny(leaf, funcs) {
continue
if ok && lineContainsAny(leaf, noInlineFuncs) {
// There might be multiple samples that point to the same function.
// But once we rename the function for the function for the first
// time, it stops matching the noInlineFuncs list. So we don't end
// up adding the prefix multiple times.
leaf.Function.Name = doNotInlinePrefix + leaf.Function.Name
}
newSamples = append(newSamples, s)
}
prof.Sample = newSamples
return nil
}

Expand Down
1 change: 1 addition & 0 deletions noinline_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ func TestApplyNoInlineHack(t *testing.T) {
require.Equal(t, 17, leafSamples(prof, grpcProcessDataFunc))
require.NoError(t, ApplyNoInlineHack(prof))
require.Equal(t, 0, leafSamples(prof, grpcProcessDataFunc))
require.Equal(t, 17, leafSamples(prof, doNotInlinePrefix+grpcProcessDataFunc))
}

func loadTestProfile(t *testing.T, name string) *profile.Profile {
Expand Down

0 comments on commit b21d138

Please sign in to comment.