All of lore.kernel.org
 help / color / mirror / Atom feed
From: Arun Sharma <asharma@fb.com>
To: Frederic Weisbecker <fweisbec@gmail.com>
Cc: linux-kernel@vger.kernel.org, Ingo Molnar <mingo@elte.hu>,
	Arnaldo Carvalho de Melo <acme@redhat.com>,
	Mike Galbraith <efault@gmx.de>, Paul Mackerras <paulus@samba.org>,
	Peter Zijlstra <peterz@infradead.org>,
	Stephane Eranian <eranian@google.com>,
	Namhyung Kim <namhyung.kim@lge.com>,
	Tom Zanussi <tzanussi@gmail.com>,
	linux-perf-users@vger.kernel.org
Subject: Re: [PATCH] perf: Add a new sort order: SORT_INCLUSIVE (v4)
Date: Thu, 15 Mar 2012 10:58:38 -0700	[thread overview]
Message-ID: <4F622DCE.4090608@fb.com> (raw)
In-Reply-To: <20120315141402.GA550@somewhere.redhat.com>

On 3/15/12 7:14 AM, Frederic Weisbecker wrote:

> I still feel concerned about this.
>
> If I have only one event with a period of 1 and with that callchain:
>
> 	a ->  b ->  c
>
> Then I produce three hists
>
> 	1) a ->  b ->  c
> 	2) a ->  b
> 	3) a
>
> Each hist have a period of 1, but the total period is 1.
> So the end result should be (IIUC):
>
> 100%    foo     a
> 100%    foo     b
>                  |
>                  --- a
> 100%    foo     c
>                  |
>                  --- b
>                      |
>                      --- c
>

That is correct. The first column no longer adds up to 100%.
  		
> And the percentages on callchain branches will have the same kind
> of weird things.

I expect --sort inclusive to be used with -g graph,0.5,caller. I can
polish this in the next rev where a single top level flag will set this up.

The percentages on the branches should still be accurate (as a 
percentage of total_period). Please let me know if this is not the case.

>
> So I'm not sure this is a good direction. I'd rather advocate to create
> true hists for each callers, all having the same real period as the leaf.
>

Please see the v5 I just posted. The callers have a true histogram entry 
in every sense, except that period_self == 0.

If we don't do this, total_period will be inflated.

> Also this feature reminds me a lot the -b option in perf report.
> Branch sorting and callchain inclusive sorting are a bit different in
> the way they handle the things but the core idea is the same. Callchains
> are branches as well.
>

Yes - I kept asking why the branch stack stuff doesn't use the existing 
callchain logic.

> Branch sorting (-b) adds a hist for every branch taken, and the period
> is always 1. I wonder if this makes more sense than using the original
> period of the event for all branches of the event. Not sure.
>
> Anyway I wonder if both features can be better integrated. After all
> they are about the same thing. The difference is that the source of
> the branches is not the same and that callchains can be depicted into
> trees.
>
> So perhaps we can have -b specifying the desired source, in case both
> are present: -b callchain and -b branch. Both at the same time wouldn't
> make much sense I think.
>
> And the source could default to either if we don't have callchain and
> branch at the same time in the events.
>
> Just an idea...

I haven't played much with the branch stack logic. Will do so and get back.

In the meanwhile, my impression is that there are two high level use cases:

* Compiler optimizers, tracing JITs etc

Which try to focus on a single branch and try to understand what 
happened with that branch

* Programmers who're trying to understand the behavior of the code they 
wrote in production

I think the branch-stack stuff primarily caters to the former and 
inclusive callchain stuff to the latter. I was thinking that getting the 
branch-stack data into callchains will make the data more useful to more 
people.

  -Arun

WARNING: multiple messages have this Message-ID (diff)
From: Arun Sharma <asharma@fb.com>
To: Frederic Weisbecker <fweisbec@gmail.com>
Cc: <linux-kernel@vger.kernel.org>, Ingo Molnar <mingo@elte.hu>,
	Arnaldo Carvalho de Melo <acme@redhat.com>,
	Mike Galbraith <efault@gmx.de>, Paul Mackerras <paulus@samba.org>,
	Peter Zijlstra <peterz@infradead.org>,
	Stephane Eranian <eranian@google.com>,
	Namhyung Kim <namhyung.kim@lge.com>,
	Tom Zanussi <tzanussi@gmail.com>,
	<linux-perf-users@vger.kernel.org>
Subject: Re: [PATCH] perf: Add a new sort order: SORT_INCLUSIVE (v4)
Date: Thu, 15 Mar 2012 10:58:38 -0700	[thread overview]
Message-ID: <4F622DCE.4090608@fb.com> (raw)
In-Reply-To: <20120315141402.GA550@somewhere.redhat.com>

On 3/15/12 7:14 AM, Frederic Weisbecker wrote:

> I still feel concerned about this.
>
> If I have only one event with a period of 1 and with that callchain:
>
> 	a ->  b ->  c
>
> Then I produce three hists
>
> 	1) a ->  b ->  c
> 	2) a ->  b
> 	3) a
>
> Each hist have a period of 1, but the total period is 1.
> So the end result should be (IIUC):
>
> 100%    foo     a
> 100%    foo     b
>                  |
>                  --- a
> 100%    foo     c
>                  |
>                  --- b
>                      |
>                      --- c
>

That is correct. The first column no longer adds up to 100%.
  		
> And the percentages on callchain branches will have the same kind
> of weird things.

I expect --sort inclusive to be used with -g graph,0.5,caller. I can
polish this in the next rev where a single top level flag will set this up.

The percentages on the branches should still be accurate (as a 
percentage of total_period). Please let me know if this is not the case.

>
> So I'm not sure this is a good direction. I'd rather advocate to create
> true hists for each callers, all having the same real period as the leaf.
>

Please see the v5 I just posted. The callers have a true histogram entry 
in every sense, except that period_self == 0.

If we don't do this, total_period will be inflated.

> Also this feature reminds me a lot the -b option in perf report.
> Branch sorting and callchain inclusive sorting are a bit different in
> the way they handle the things but the core idea is the same. Callchains
> are branches as well.
>

Yes - I kept asking why the branch stack stuff doesn't use the existing 
callchain logic.

> Branch sorting (-b) adds a hist for every branch taken, and the period
> is always 1. I wonder if this makes more sense than using the original
> period of the event for all branches of the event. Not sure.
>
> Anyway I wonder if both features can be better integrated. After all
> they are about the same thing. The difference is that the source of
> the branches is not the same and that callchains can be depicted into
> trees.
>
> So perhaps we can have -b specifying the desired source, in case both
> are present: -b callchain and -b branch. Both at the same time wouldn't
> make much sense I think.
>
> And the source could default to either if we don't have callchain and
> branch at the same time in the events.
>
> Just an idea...

I haven't played much with the branch stack logic. Will do so and get back.

In the meanwhile, my impression is that there are two high level use cases:

* Compiler optimizers, tracing JITs etc

Which try to focus on a single branch and try to understand what 
happened with that branch

* Programmers who're trying to understand the behavior of the code they 
wrote in production

I think the branch-stack stuff primarily caters to the former and 
inclusive callchain stuff to the latter. I was thinking that getting the 
branch-stack data into callchains will make the data more useful to more 
people.

  -Arun

  reply	other threads:[~2012-03-15 17:58 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-03-14 17:36 [PATCH] perf: Add a new sort order: SORT_INCLUSIVE (v4) Arun Sharma
2012-03-15  1:02 ` Namhyung Kim
2012-03-15  1:02   ` Namhyung Kim
2012-03-15 14:14 ` Frederic Weisbecker
2012-03-15 17:58   ` Arun Sharma [this message]
2012-03-15 17:58     ` Arun Sharma
2012-03-19 15:57     ` Frederic Weisbecker
2012-03-20 23:28       ` Arun Sharma
2012-03-20 23:28         ` Arun Sharma
2012-03-25  2:14         ` Frederic Weisbecker
2012-03-27 18:09           ` Arun Sharma
2012-03-27 18:09             ` Arun Sharma
2012-03-27 19:38             ` Peter Zijlstra

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=4F622DCE.4090608@fb.com \
    --to=asharma@fb.com \
    --cc=acme@redhat.com \
    --cc=efault@gmx.de \
    --cc=eranian@google.com \
    --cc=fweisbec@gmail.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-perf-users@vger.kernel.org \
    --cc=mingo@elte.hu \
    --cc=namhyung.kim@lge.com \
    --cc=paulus@samba.org \
    --cc=peterz@infradead.org \
    --cc=tzanussi@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.