Re: [PATCH 01/17] perf hists: Basic support of hierarchical report view

[Namhyung Kim]

Hi Jiri,

On Thu, Jan 21, 2016 at 11:43:30AM +0100, Jiri Olsa wrote:
> On Sun, Jan 17, 2016 at 01:03:01AM +0900, Namhyung Kim wrote:
> 
> SNIP
> 
> > +}
> > +
> > +static bool hists__hierarchy_insert_entry(struct hists *hists,
> > +					  struct rb_root *root,
> > +					  struct hist_entry *he)
> > +{
> > +	struct perf_hpp_fmt *fmt;
> > +	struct hist_entry *new = NULL;
> > +	struct hist_entry *parent = NULL;
> > +	int depth = 0;
> > +
> > +	perf_hpp__for_each_sort_list(fmt) {
> > +		if (!perf_hpp__is_sort_entry(fmt) &&
> > +		    !perf_hpp__is_dynamic_entry(fmt))
> > +			continue;
> > +
> > +		/* insert copy of 'he' for each fmt into the hierarchy */
> > +		new = hierarchy_insert_entry(hists, root, he, fmt);
> > +		if (new == NULL)
> > +			break;
> 
> so hierarchy_insert_entry can fail because of memory allocation
> but the resort path does not cover any error path because it only
> shuffles entries from in-tree into sorted tree

Yes, memory allocation can fail anywhere.  If it happens, there's not
much thing we can do IMHO - just print warning and bail out.
Currently it silently ignores the allocation error and try to proceed.
But I guess it'll fail soon at other place anyway.

AFAICS current code also can fail in callchain_merge()..

Maybe we can change the return type of this function to int and treat
-1 as an error to detect such cases.


> 
> would it make more sense to do this in 'in-tree addition' path?
> and keep the resort functions to do only resort stuff

I don't follow.  There're 3 path to handle hist entries - let's say
them as 'addition', 'collapsing', and 'resort'.  This function does
the 'collapsing' part - it was originally intended to merge sharable
entries (namely for same 'comm' among different threads).  But I used
it to build a hierarchy since I found it useful as follows:

  1. it requires smaller change than doing it in the 'addition' path
  2. it can reuse current callback-based 'addition' paths so mem- and
     branch-mode can be supported easily (but it needs test..).
  3. the 'addition' path can be parallelized so it'll increase memory
     footprint if it build temporary local hierarchies during the path.

The 'resort' path always do sorting only..

Thanks,
Namhyung