-
Notifications
You must be signed in to change notification settings - Fork 612
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add -diff flag for better profile comparision #369
Changes from 13 commits
5a69374
b20456b
190ce18
ebd76df
92f6fbe
6921532
14703a0
49ff346
f486417
bd80566
79b3df8
ee3be39
562c51d
4b9834b
9558b3c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,6 +15,7 @@ | |
package driver | ||
|
||
import ( | ||
"errors" | ||
"fmt" | ||
"os" | ||
"strings" | ||
|
@@ -28,6 +29,7 @@ type source struct { | |
ExecName string | ||
BuildID string | ||
Base []string | ||
DiffBase bool | ||
Normalize bool | ||
|
||
Seconds int | ||
|
@@ -43,7 +45,8 @@ type source struct { | |
func parseFlags(o *plugin.Options) (*source, []string, error) { | ||
flag := o.Flagset | ||
// Comparisons. | ||
flagBase := flag.StringList("base", "", "Source for base profile for comparison") | ||
flagBase := flag.StringList("base", "", "Source for base profile for profile subtraction") | ||
flagDiffBase := flag.StringList("diff_base", "", "Source for diff base profile for comparison") | ||
// Source options. | ||
flagSymbolize := flag.String("symbolize", "", "Options for profile symbolization") | ||
flagBuildID := flag.String("buildid", "", "Override build id for first mapping") | ||
|
@@ -85,7 +88,7 @@ func parseFlags(o *plugin.Options) (*source, []string, error) { | |
usageMsgVars) | ||
}) | ||
if len(args) == 0 { | ||
return nil, nil, fmt.Errorf("no profile source specified") | ||
return nil, nil, errors.New("no profile source specified") | ||
} | ||
|
||
var execName string | ||
|
@@ -112,7 +115,7 @@ func parseFlags(o *plugin.Options) (*source, []string, error) { | |
return nil, nil, err | ||
} | ||
if cmd != nil && *flagHTTP != "" { | ||
return nil, nil, fmt.Errorf("-http is not compatible with an output format on the command line") | ||
return nil, nil, errors.New("-http is not compatible with an output format on the command line") | ||
} | ||
|
||
si := pprofVariables["sample_index"].value | ||
|
@@ -140,15 +143,13 @@ func parseFlags(o *plugin.Options) (*source, []string, error) { | |
Comment: *flagAddComment, | ||
} | ||
|
||
for _, s := range *flagBase { | ||
if *s != "" { | ||
source.Base = append(source.Base, *s) | ||
} | ||
if err := source.addBaseProfiles(*flagBase, *flagDiffBase); err != nil { | ||
return nil, nil, err | ||
} | ||
|
||
normalize := pprofVariables["normalize"].boolValue() | ||
if normalize && len(source.Base) == 0 { | ||
return nil, nil, fmt.Errorf("Must have base profile to normalize by") | ||
return nil, nil, errors.New("Must have base profile to normalize by") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
} | ||
source.Normalize = normalize | ||
|
||
|
@@ -158,6 +159,35 @@ func parseFlags(o *plugin.Options) (*source, []string, error) { | |
return source, cmd, nil | ||
} | ||
|
||
// addBaseProfiles adds the list of base profiles or diff base profiles to | ||
// the source. This function will return an error if both base and diff base | ||
// profiles are specified. | ||
func (source *source) addBaseProfiles(flagBase, flagDiffBase []*string) error { | ||
base, diffBase := dropEmpty(flagBase), dropEmpty(flagDiffBase) | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: remove empty line. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
if len(base) > 0 && len(diffBase) > 0 { | ||
return errors.New("-base and -diff_base flags cannot both be specified") | ||
} | ||
|
||
source.Base = base | ||
if len(diffBase) > 0 { | ||
source.Base, source.DiffBase = diffBase, true | ||
} | ||
return nil | ||
} | ||
|
||
// dropEmpty list takes a StringList flag, and outputs an array of non-empty | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It does not take a flag, it takes a slice of string pointers. It does not return an array, it returns a slice. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
// strings associated with the flag. | ||
func dropEmpty(list []*string) []string { | ||
var l []string | ||
for _, s := range list { | ||
if *s != "" { | ||
l = append(l, *s) | ||
} | ||
} | ||
return l | ||
} | ||
|
||
// installFlags creates command line flags for pprof variables. | ||
func installFlags(flag plugin.FlagSet) flagsInstalled { | ||
f := flagsInstalled{ | ||
|
@@ -240,15 +270,15 @@ func outputFormat(bcmd map[string]*bool, acmd map[string]*string) (cmd []string, | |
for n, b := range bcmd { | ||
if *b { | ||
if cmd != nil { | ||
return nil, fmt.Errorf("must set at most one output format") | ||
return nil, errors.New("must set at most one output format") | ||
} | ||
cmd = []string{n} | ||
} | ||
} | ||
for n, s := range acmd { | ||
if *s != "" { | ||
if cmd != nil { | ||
return nil, fmt.Errorf("must set at most one output format") | ||
return nil, errors.New("must set at most one output format") | ||
} | ||
cmd = []string{n, *s} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -288,7 +288,7 @@ type testFlags struct { | |
floats map[string]float64 | ||
strings map[string]string | ||
args []string | ||
stringLists map[string][]*string | ||
stringLists map[string][]string | ||
} | ||
|
||
func (testFlags) ExtraUsage() string { return "" } | ||
|
@@ -355,7 +355,11 @@ func (f testFlags) StringVar(p *string, s, d, c string) { | |
|
||
func (f testFlags) StringList(s, d, c string) *[]*string { | ||
if t, ok := f.stringLists[s]; ok { | ||
return &t | ||
tp := make([]*string, len(t)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why this change? It's not obvious, a comment would be useful. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was to address a previous comment for simplifying testing. Added a comment. |
||
for i, v := range t { | ||
tp[i] = &v | ||
} | ||
return &tp | ||
} | ||
return &[]*string{} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is a bit strange that "Comparing profiles" is a section under "Fetching profiles", is it intentional? I'd expect it to be more related to viewing than to fetching.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. Moved.