Skip to content

Commit

Permalink
address comments on code
Browse files Browse the repository at this point in the history
  • Loading branch information
nolanmar511 committed May 9, 2018
1 parent f486417 commit bd80566
Show file tree
Hide file tree
Showing 6 changed files with 146 additions and 69 deletions.
2 changes: 1 addition & 1 deletion doc/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -229,7 +229,7 @@ the output are relative to the total of samples in the diff base profile.
* **-base= _profile_:** useful for subtracting a cumulative profile, like a
[golang block profile](https://golang.org/doc/diagnostics.html#profiling),
from another cumulative profile collected from the same program at a later time.
When comparing cumulative profiles collected on the same profile, percentages in
When comparing cumulative profiles collected on the same program, percentages in
the output are relative to the difference between the total for the source
profile and the total for the base profile.

Expand Down
14 changes: 7 additions & 7 deletions internal/driver/cli.go
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ func parseFlags(o *plugin.Options) (*source, []string, error) {
Comment: *flagAddComment,
}

if err := source.addBaseProfiles(flagBase, flagDiffBase); err != nil {
if err := source.addBaseProfiles(*flagBase, *flagDiffBase); err != nil {
return nil, nil, err
}

Expand All @@ -161,9 +161,9 @@ func parseFlags(o *plugin.Options) (*source, []string, error) {
// 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 := parseStringListFlag(flagBase)
diffBase := parseStringListFlag(flagDiffBase)
func (source *source) addBaseProfiles(flagBase, flagDiffBase []*string) error {
base := dropEmpty(flagBase)
diffBase := dropEmpty(flagDiffBase)

if len(base) > 0 && len(diffBase) > 0 {
return fmt.Errorf("-base and -diff_base flags cannot both be specified")
Expand All @@ -177,11 +177,11 @@ func (source *source) addBaseProfiles(flagBase, flagDiffBase *[]*string) error {
return nil
}

// parseStringListFlag list takes a StringList flag, and outputs an array of non-empty
// dropEmpty list takes a StringList flag, and outputs an array of non-empty
// strings associated with the flag.
func parseStringListFlag(list *[]*string) []string {
func dropEmpty(list []*string) []string {
var l []string
for _, s := range *list {
for _, s := range list {
if *s != "" {
l = append(l, *s)
}
Expand Down
10 changes: 5 additions & 5 deletions internal/driver/fetch.go
Original file line number Diff line number Diff line change
Expand Up @@ -57,12 +57,15 @@ func fetchProfiles(s *source, o *plugin.Options) (*profile.Profile, error) {
})
}

p, pbase, m, mbase, save, err := grabSourcesAndBases(sources, bases, s.DiffBase, o.Fetch, o.Obj, o.UI)
p, pbase, m, mbase, save, err := grabSourcesAndBases(sources, bases, o.Fetch, o.Obj, o.UI)
if err != nil {
return nil, err
}

if pbase != nil {
if s.DiffBase {
pbase.SetLabel("pprof::base", []string{"true"})
}
if s.Normalize {
err := p.Normalize(pbase)
if err != nil {
Expand Down Expand Up @@ -120,7 +123,7 @@ func fetchProfiles(s *source, o *plugin.Options) (*profile.Profile, error) {
return p, nil
}

func grabSourcesAndBases(sources, bases []profileSource, isDiffBase bool, fetch plugin.Fetcher, obj plugin.ObjTool, ui plugin.UI) (*profile.Profile, *profile.Profile, plugin.MappingSources, plugin.MappingSources, bool, error) {
func grabSourcesAndBases(sources, bases []profileSource, fetch plugin.Fetcher, obj plugin.ObjTool, ui plugin.UI) (*profile.Profile, *profile.Profile, plugin.MappingSources, plugin.MappingSources, bool, error) {
wg := sync.WaitGroup{}
wg.Add(2)
var psrc, pbase *profile.Profile
Expand All @@ -135,9 +138,6 @@ func grabSourcesAndBases(sources, bases []profileSource, isDiffBase bool, fetch
go func() {
defer wg.Done()
pbase, mbase, savebase, countbase, errbase = chunkedGrab(bases, fetch, obj, ui)
if pbase != nil && isDiffBase {
pbase.SetTag("pprof::base", []string{"true"})
}
}()
wg.Wait()
save := savesrc || savebase
Expand Down
4 changes: 2 additions & 2 deletions internal/report/report.go
Original file line number Diff line number Diff line change
Expand Up @@ -266,7 +266,7 @@ func (rpt *Report) newGraph(nodes graph.NodeSet) *graph.Graph {

// Remove tag marking samples from the base profiles, so it does not appear
// as a nodelet in the graph view.
prof.SetTag("pprof::base", []string{})
prof.RemoveLabel("pprof::base")

formatTag := func(v int64, key string) string {
return measurement.ScaledLabel(v, key, o.OutputUnit)
Expand Down Expand Up @@ -1232,7 +1232,7 @@ func computeTotal(prof *profile.Profile, value, meanDiv func(v []int64) int64) i
}
total += v
div += d
if sample.HasTag("pprof::base", "true") {
if sample.HasLabel("pprof::base", "true") {
diffTotal += v
diffDiv += d
}
Expand Down
18 changes: 14 additions & 4 deletions profile/profile.go
Original file line number Diff line number Diff line change
Expand Up @@ -674,9 +674,9 @@ func numLabelsToString(numLabels map[string][]int64, numUnits map[string][]strin
return strings.Join(ls, " ")
}

// SetTag sets the specified key to the specified value for all samples in the
// SetLabel sets the specified key to the specified value for all samples in the
// profile.
func (p *Profile) SetTag(key string, value []string) {
func (p *Profile) SetLabel(key string, value []string) {
for _, sample := range p.Sample {
if sample.Label == nil {
sample.Label = map[string][]string{}
Expand All @@ -685,8 +685,18 @@ func (p *Profile) SetTag(key string, value []string) {
}
}

// HasTag returns true if a sample has a tag with indicated key and value.
func (s *Sample) HasTag(key, value string) bool {
// RemoveLabel removes all labels associated with the specified key for all
// samples in the profile.
func (p *Profile) RemoveLabel(key string) {
for _, sample := range p.Sample {
if _, ok := sample.Label[key]; ok {
delete(sample.Label, key)
}
}
}

// HasLabel returns true if a sample has a label with indicated key and value.
func (s *Sample) HasLabel(key, value string) bool {
if values, ok := s.Label[key]; ok {
for _, v := range values {
if v == value {
Expand Down
Loading

0 comments on commit bd80566

Please sign in to comment.