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

[Jiri Olsa]

On Thu, Jan 21, 2016 at 09:55:52PM +0900, Namhyung Kim wrote:

SNIP

> > > +		/* 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.

I thought the 'policy' is to handle all allocation failures

> 
> 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..

well, you are adding/duplicating entries now in resort path
and that is not just 'sorting only'

you allow only sort and tracepoint entries to be added in
hierrarych view, so there's no resort needed, but still it
could be added in future? not sure

it still makes more sense to me to do this in 'addition' path,
because you basically add new entries

but have no other grounds for this also I might be missing something ;-)

thanks,
jirka