From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S967069AbdEXIH1 (ORCPT <rfc822;w@1wt.eu>);
        Wed, 24 May 2017 04:07:27 -0400
Received: from mail-wm0-f65.google.com ([74.125.82.65]:33921 "EHLO
        mail-wm0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S966972AbdEXIHC (ORCPT
        <rfc822;linux-kernel@vger.kernel.org>);
        Wed, 24 May 2017 04:07:02 -0400
Date: Wed, 24 May 2017 10:06:58 +0200
From: Ingo Molnar <mingo@kernel.org>
To: Milian Wolff <milian.wolff@kdab.com>
Cc: Namhyung Kim <namhyung@kernel.org>,
        LKML <linux-kernel@vger.kernel.org>, kernel-team@lge.com,
        Arnaldo Carvalho de Melo <acme@kernel.org>,
        Jiri Olsa <jolsa@kernel.org>, Yao Jin <yao.jin@linux.intel.com>
Subject: Re: [PATCH 4/7] perf script: Add --inline option
Message-ID: <20170524080657.x3zu3lpykoox6mo7@gmail.com>
References: <20170524062129.32529-1-namhyung@kernel.org>
 <20170524071330.GA32206@sejong>
 <20170524072142.cto7coohqwhake6t@gmail.com>
 <747830927.nmHmt93QeT@milian-kdab2>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <747830927.nmHmt93QeT@milian-kdab2>
User-Agent: NeoMutt/20170113 (1.7.2)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


* Milian Wolff <milian.wolff@kdab.com> wrote:

> On Wednesday, May 24, 2017 9:21:42 AM CEST Ingo Molnar wrote:
> > * Namhyung Kim <namhyung@kernel.org> wrote:
> > > On Wed, May 24, 2017 at 08:38:11AM +0200, Ingo Molnar wrote:
> > > > * Namhyung Kim <namhyung@kernel.org> wrote:
> > > > > The --inline option is to show inlined functions in callchains.
> > > > > 
> > > > > For example,
> > > > > 
> > > > >   $ perf script
> > > > >   
> > > > >   a.out  5644 11611.467597:     309961 cycles:u:
> > > > >                      790 main (/home/namhyung/tmp/perf/a.out)
> > > > >                    
> > > > >                    20511 __libc_start_main (/usr/lib/libc-2.25.so)
> > > > >                    
> > > > >                      8ba _start (/home/namhyung/tmp/perf/a.out)
> > > > >   
> > > > >   ...
> > > > >   
> > > > >   $ perf script --inline
> > > > >   
> > > > >   a.out  5644 11611.467597:     309961 cycles:u:
> > > > >                      790 main (/home/namhyung/tmp/perf/a.out)
> > > > >                      
> > > > >                          std::__detail::_Adaptor<std::linear_congruent
> > > > >                          ial_engine<unsigned long, 16807ul, 0ul,
> > > > >                          2147483647ul>, double>::operator()
> > > > >                          std::uniform_real_distribution<double>::oper
> > > > >                          ator()<std::linear_congruential_engine<unsign
> > > > >                          ed long, 16807ul, 0ul, 2147483647ul> >
> > > > >                          std::uniform_real_distribution<double>::oper
> > > > >                          ator()<std::linear_congruential_engine<unsign
> > > > >                          ed long, 16807ul, 0ul, 2147483647ul> > main
> > > > >                    
> > > > >                    20511 __libc_start_main (/usr/lib/libc-2.25.so)
> > > > >                    
> > > > >                      8ba _start (/home/namhyung/tmp/perf/a.out)
> > > > >   
> > > > >   ...
> > > > 
> > > > Shouldn't this be the default behavior, to make call chains more
> > > > readable?
> > > 
> > > AFAIK perf report didn't make it default due to a performance impact,
> > > but I didn't know how much it is.  Especially if perf was not built
> > > with libbfd it'll run external addr2line to get inlined functions for
> > > each callchain entry..
> > 
> > So then at least let's make it the default when all libraries are present.
> > Not enabling something when the build is not 'complete' is fair game -
> > distros will typically have all the libraries available.
> > 
> > We need to remember that roughly 99% of all our users will use as few perf
> > command line options as they can get away with - myself included. Adding a
> > non-debugging feature as a non-default command line option is really as if
> > we didn't do anything: very few if any people will use it, and it might
> > bitrot in the future without people noticing.
> > 
> > So we need apply some thought into making it available to two orders of
> > magnitude more people! If someone types 'perf report' we should give the
> > best selection of all the features we have available.
> 
> Just a suggestion: My larger patch set that is in review now adds some caching 
> features which already speeds up the whole process considerably. As such, my 
> suggestion is to wait for this patch set to be integrated. Then we could 
> enable --inline unconditionally, or at least only when libbfd is available.

I'm fine with that - and please make the default-enabling part of your patch 
series, so it does not get forgotten.

Thanks,

	Ingo