Scrolling down is broken when using "perf top --hierarchy".
When it starts up everything is OK and one can scroll up and down to all
entries. But as further and further new entries get added to the list,
scrolling down is blocked (at the position of the last entry that was
shown directly after startup).
--
Markus
Hi Markus,
On Thu, Oct 06, 2016 at 06:33:33PM +0200, Markus Trippelsdorf wrote:
> Scrolling down is broken when using "perf top --hierarchy".
> When it starts up everything is OK and one can scroll up and down to all
> entries. But as further and further new entries get added to the list,
> scrolling down is blocked (at the position of the last entry that was
> shown directly after startup).
I think below patch will fix the problem. Please check.
>From 38ef0f20e6b787fcdab265307ac679cfac6dd0ff Mon Sep 17 00:00:00 2001
From: Namhyung Kim <[email protected]>
Date: Fri, 7 Oct 2016 10:09:42 +0900
Subject: [PATCH] perf top: Fix refreshing hierarchy entries on TUI
Markus reported that perf top --hierarch cannot scroll down after
refresh. This was because the number of entries are not updated when
hierarchy is enabled.
Unlike normal report view, hierarchy mode needs to keep its own entry
count since it can have non-leaf entries which can expand/collapse.
Reported-by: Markus Trippelsdorf <[email protected]>
Signed-off-by: Namhyung Kim <[email protected]>
---
tools/perf/ui/browsers/hists.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index fb8e42c7507a..4ffff7be9299 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -601,7 +601,8 @@ int hist_browser__run(struct hist_browser *browser, const char *help)
u64 nr_entries;
hbt->timer(hbt->arg);
- if (hist_browser__has_filter(browser))
+ if (hist_browser__has_filter(browser) ||
+ symbol_conf.report_hierarchy)
hist_browser__update_nr_entries(browser);
nr_entries = hist_browser__nr_entries(browser);
--
2.9.3
On 2016.10.07 at 10:17 +0900, Namhyung Kim wrote:
> On Thu, Oct 06, 2016 at 06:33:33PM +0200, Markus Trippelsdorf wrote:
> > Scrolling down is broken when using "perf top --hierarchy".
> > When it starts up everything is OK and one can scroll up and down to all
> > entries. But as further and further new entries get added to the list,
> > scrolling down is blocked (at the position of the last entry that was
> > shown directly after startup).
>
> I think below patch will fix the problem. Please check.
Yes. It works fine now. Many thanks.
--
Markus
On Fri, Oct 07, 2016 at 05:51:18AM +0200, Markus Trippelsdorf wrote:
> On 2016.10.07 at 10:17 +0900, Namhyung Kim wrote:
> > On Thu, Oct 06, 2016 at 06:33:33PM +0200, Markus Trippelsdorf wrote:
> > > Scrolling down is broken when using "perf top --hierarchy".
> > > When it starts up everything is OK and one can scroll up and down to all
> > > entries. But as further and further new entries get added to the list,
> > > scrolling down is blocked (at the position of the last entry that was
> > > shown directly after startup).
> >
> > I think below patch will fix the problem. Please check.
>
> Yes. It works fine now. Many thanks.
Good. Can I add your Tested-by then?
Thanks,
Namhyung
On 2016.10.07 at 13:22 +0900, Namhyung Kim wrote:
> On Fri, Oct 07, 2016 at 05:51:18AM +0200, Markus Trippelsdorf wrote:
> > On 2016.10.07 at 10:17 +0900, Namhyung Kim wrote:
> > > On Thu, Oct 06, 2016 at 06:33:33PM +0200, Markus Trippelsdorf wrote:
> > > > Scrolling down is broken when using "perf top --hierarchy".
> > > > When it starts up everything is OK and one can scroll up and down to all
> > > > entries. But as further and further new entries get added to the list,
> > > > scrolling down is blocked (at the position of the last entry that was
> > > > shown directly after startup).
> > >
> > > I think below patch will fix the problem. Please check.
> >
> > Yes. It works fine now. Many thanks.
>
> Good. Can I add your Tested-by then?
Sure.
(And in the long run you should think of making "perf top --hierarchy"
the default for perf top, because it gives a much better (uncluttered)
overview of what is going on.)
--
Markus
Cc-ing perf maintainers,
On Fri, Oct 07, 2016 at 06:32:29AM +0200, Markus Trippelsdorf wrote:
> On 2016.10.07 at 13:22 +0900, Namhyung Kim wrote:
> > On Fri, Oct 07, 2016 at 05:51:18AM +0200, Markus Trippelsdorf wrote:
> > > On 2016.10.07 at 10:17 +0900, Namhyung Kim wrote:
> > > > On Thu, Oct 06, 2016 at 06:33:33PM +0200, Markus Trippelsdorf wrote:
> > > > > Scrolling down is broken when using "perf top --hierarchy".
> > > > > When it starts up everything is OK and one can scroll up and down to all
> > > > > entries. But as further and further new entries get added to the list,
> > > > > scrolling down is blocked (at the position of the last entry that was
> > > > > shown directly after startup).
> > > >
> > > > I think below patch will fix the problem. Please check.
> > >
> > > Yes. It works fine now. Many thanks.
> >
> > Good. Can I add your Tested-by then?
>
> Sure.
Ok, I'll send a formal patch with it.
>
> (And in the long run you should think of making "perf top --hierarchy"
> the default for perf top, because it gives a much better (uncluttered)
> overview of what is going on.)
I think it's a matter of taste. Some people prefer to see the top
single function or something (i.e. current behavior) while others
prefer to see a higher-level view.
But we can think again about the default at least for perf-top. I
worried about changing default behavior because last time we did it
for children mode many people complained about it. But I do think the
hierarchy mode is useful for many people though.
Hmm.. I thought that it already has a config option to enable hierarch
mode by default, but I cannot find it now.
Thanks,
Namhyung
On 2016.10.07 at 06:32 +0200, Markus Trippelsdorf wrote:
> On 2016.10.07 at 13:22 +0900, Namhyung Kim wrote:
> > On Fri, Oct 07, 2016 at 05:51:18AM +0200, Markus Trippelsdorf wrote:
> > > On 2016.10.07 at 10:17 +0900, Namhyung Kim wrote:
> > > > On Thu, Oct 06, 2016 at 06:33:33PM +0200, Markus Trippelsdorf wrote:
> > > > > Scrolling down is broken when using "perf top --hierarchy".
> > > > > When it starts up everything is OK and one can scroll up and down to all
> > > > > entries. But as further and further new entries get added to the list,
> > > > > scrolling down is blocked (at the position of the last entry that was
> > > > > shown directly after startup).
> > > >
> > > > I think below patch will fix the problem. Please check.
> > >
> > > Yes. It works fine now. Many thanks.
> >
> > Good. Can I add your Tested-by then?
>
> Sure.
And BTW symbols are currently always cut off at 60 characters in expanded entries.
--
Markus
On 2016.10.07 at 06:56 +0200, Markus Trippelsdorf wrote:
> On 2016.10.07 at 06:32 +0200, Markus Trippelsdorf wrote:
> > On 2016.10.07 at 13:22 +0900, Namhyung Kim wrote:
> > > On Fri, Oct 07, 2016 at 05:51:18AM +0200, Markus Trippelsdorf wrote:
> > > > On 2016.10.07 at 10:17 +0900, Namhyung Kim wrote:
> > > > > On Thu, Oct 06, 2016 at 06:33:33PM +0200, Markus Trippelsdorf wrote:
> > > > > > Scrolling down is broken when using "perf top --hierarchy".
> > > > > > When it starts up everything is OK and one can scroll up and down to all
> > > > > > entries. But as further and further new entries get added to the list,
> > > > > > scrolling down is blocked (at the position of the last entry that was
> > > > > > shown directly after startup).
> > > > >
> > > > > I think below patch will fix the problem. Please check.
> > > >
> > > > Yes. It works fine now. Many thanks.
> > >
> > > Good. Can I add your Tested-by then?
> >
> > Sure.
>
> And BTW symbols are currently always cut off at 60 characters in
> expanded entries.
Hmm, no. Sometimes they are cut off, sometimes they are not. I haven't
figured out what triggered this strange behavior.
--
Markus
Em Fri, Oct 07, 2016 at 01:53:57PM +0900, Namhyung Kim escreveu:
> Cc-ing perf maintainers,
>
> On Fri, Oct 07, 2016 at 06:32:29AM +0200, Markus Trippelsdorf wrote:
> > On 2016.10.07 at 13:22 +0900, Namhyung Kim wrote:
> > > On Fri, Oct 07, 2016 at 05:51:18AM +0200, Markus Trippelsdorf wrote:
> > > > On 2016.10.07 at 10:17 +0900, Namhyung Kim wrote:
> > > > > On Thu, Oct 06, 2016 at 06:33:33PM +0200, Markus Trippelsdorf wrote:
> > > > > > Scrolling down is broken when using "perf top --hierarchy".
> > > > > > When it starts up everything is OK and one can scroll up and down to all
> > > > > > entries. But as further and further new entries get added to the list,
> > > > > > scrolling down is blocked (at the position of the last entry that was
> > > > > > shown directly after startup).
> > > > >
> > > > > I think below patch will fix the problem. Please check.
> > > >
> > > > Yes. It works fine now. Many thanks.
> > >
> > > Good. Can I add your Tested-by then?
> >
> > Sure.
>
> Ok, I'll send a formal patch with it.
>
> >
> > (And in the long run you should think of making "perf top --hierarchy"
> > the default for perf top, because it gives a much better (uncluttered)
> > overview of what is going on.)
>
> I think it's a matter of taste. Some people prefer to see the top
> single function or something (i.e. current behavior) while others
> prefer to see a higher-level view.
>
> But we can think again about the default at least for perf-top. I
> worried about changing default behavior because last time we did it
> for children mode many people complained about it. But I do think the
> hierarchy mode is useful for many people though.
So, I think in such cases we could experiment with asking the user about
switching to the new mode by showing a popup message telling what it is
about, if the user says "yes, I want to try it" switch to it and if
another hotkey is pressed later, write what was chosen (yes, switch to
this new mode, no, I don't like it, don't pester me about it anymore) to
its ~/.perfconfig file so that next time it goes straight to this new
mode, else don't ask the user again and keep using whatever mode was
there already.
What do you think?
- Arnaldo
> Hmm.. I thought that it already has a config option to enable hierarch
> mode by default, but I cannot find it now.
>
> Thanks,
> Namhyung
On 2016.10.07 at 07:09 +0200, Markus Trippelsdorf wrote:
> On 2016.10.07 at 06:56 +0200, Markus Trippelsdorf wrote:
> > On 2016.10.07 at 06:32 +0200, Markus Trippelsdorf wrote:
> > > On 2016.10.07 at 13:22 +0900, Namhyung Kim wrote:
> > > > On Fri, Oct 07, 2016 at 05:51:18AM +0200, Markus Trippelsdorf wrote:
> > > > > On 2016.10.07 at 10:17 +0900, Namhyung Kim wrote:
> > > > > > On Thu, Oct 06, 2016 at 06:33:33PM +0200, Markus Trippelsdorf wrote:
> > > > > > > Scrolling down is broken when using "perf top --hierarchy".
> > > > > > > When it starts up everything is OK and one can scroll up and down to all
> > > > > > > entries. But as further and further new entries get added to the list,
> > > > > > > scrolling down is blocked (at the position of the last entry that was
> > > > > > > shown directly after startup).
> > > > > >
> > > > > > I think below patch will fix the problem. Please check.
> > > > >
> > > > > Yes. It works fine now. Many thanks.
> > > >
> > > > Good. Can I add your Tested-by then?
> > >
> > > Sure.
> >
> > And BTW symbols are currently always cut off at 60 characters in
> > expanded entries.
>
> Hmm, no. Sometimes they are cut off, sometimes they are not. I haven't
> figured out what triggered this strange behavior.
Here is an example:
% echo $COLUMNS
179
% perf top --hierarchy
+ 34.81% [kernel]
- 20.89% chrome
0.51% [.] v8::internal::IncrementalMarking:
0.43% [.] tc_malloc
0.29% [.] sqlite3BtreeMovetoUnpacked
0.28% [.] tc_free
0.24% [.] v8::internal::BodyDescriptorBase:
0.24% [.] sqlite3VdbeExec
0.22% [.] v8::internal::MarkCompactCollecto
0.19% [.] blink::SelectorChecker::checkOne
0.19% [.] SkBlitRow::Color32
0.18% [.] SkBlitLCD16OpaqueRow_SSE2
0.17% [.] btreeInitPage.part.366
0.16% [.] blink::SelectorChecker::matchSele
0.15% [.] blink::ElementRuleCollector::coll
0.15% [.] blink::CSSTokenizer::consumeName
0.14% [.] sqlite3GetVarint
0.13% [.] operator new[]
0.12% [.] FPDFAPI_inflate_fast
0.11% [.] v8::internal::HeapObject::SizeFro
0.09% [.] tracked_objects::ThreadData::Tall
...
--
Markus
On 2016.10.08 at 13:21 +0200, Markus Trippelsdorf wrote:
> On 2016.10.07 at 07:09 +0200, Markus Trippelsdorf wrote:
> > On 2016.10.07 at 06:56 +0200, Markus Trippelsdorf wrote:
> > > On 2016.10.07 at 06:32 +0200, Markus Trippelsdorf wrote:
> > > > On 2016.10.07 at 13:22 +0900, Namhyung Kim wrote:
> > > > > On Fri, Oct 07, 2016 at 05:51:18AM +0200, Markus Trippelsdorf wrote:
> > > > > > On 2016.10.07 at 10:17 +0900, Namhyung Kim wrote:
> > > > > > > On Thu, Oct 06, 2016 at 06:33:33PM +0200, Markus Trippelsdorf wrote:
> > > > > > > > Scrolling down is broken when using "perf top --hierarchy".
> > > > > > > > When it starts up everything is OK and one can scroll up and down to all
> > > > > > > > entries. But as further and further new entries get added to the list,
> > > > > > > > scrolling down is blocked (at the position of the last entry that was
> > > > > > > > shown directly after startup).
> > > > > > >
> > > > > > > I think below patch will fix the problem. Please check.
> > > > > >
> > > > > > Yes. It works fine now. Many thanks.
> > > > >
> > > > > Good. Can I add your Tested-by then?
> > > >
> > > > Sure.
> > >
> > > And BTW symbols are currently always cut off at 60 characters in
> > > expanded entries.
> >
> > Hmm, no. Sometimes they are cut off, sometimes they are not. I haven't
> > figured out what triggered this strange behavior.
>
> Here is an example:
>
> % echo $COLUMNS
> 179
> % perf top --hierarchy
> + 34.81% [kernel]
> - 20.89% chrome
> 0.51% [.] v8::internal::IncrementalMarking:
> 0.43% [.] tc_malloc
> 0.29% [.] sqlite3BtreeMovetoUnpacked
> 0.28% [.] tc_free
> 0.24% [.] v8::internal::BodyDescriptorBase:
> 0.24% [.] sqlite3VdbeExec
> 0.22% [.] v8::internal::MarkCompactCollecto
> 0.19% [.] blink::SelectorChecker::checkOne
> 0.19% [.] SkBlitRow::Color32
> 0.18% [.] SkBlitLCD16OpaqueRow_SSE2
> 0.17% [.] btreeInitPage.part.366
> 0.16% [.] blink::SelectorChecker::matchSele
> 0.15% [.] blink::ElementRuleCollector::coll
> 0.15% [.] blink::CSSTokenizer::consumeName
> 0.14% [.] sqlite3GetVarint
> 0.13% [.] operator new[]
> 0.12% [.] FPDFAPI_inflate_fast
> 0.11% [.] v8::internal::HeapObject::SizeFro
> 0.09% [.] tracked_objects::ThreadData::Tall
> ...
To continue this monologue, perf doesn't even look at these entries. So some
hists__calc_col_len() calls seem to be missing for the "perf top --hierarchy"
case or hists__reset_col_len() is called too early or too often.
Anyway, the following hack "fixes" the issue for me:
diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index b02992efb513..7e468fa56980 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -67,7 +67,7 @@ void hists__calc_col_len(struct hists *hists, struct hist_entry *h)
* +3 accounts for ' y ' symtab origin info
*/
if (h->ms.sym) {
- symlen = h->ms.sym->namelen + 4;
+ symlen = 200;
if (verbose)
symlen += BITS_PER_LONG / 4 + 2 + 3;
hists__new_col_len(hists, HISTC_SYMBOL, symlen);
--
Markus
Hi,
Sorry for late reply.
On Mon, Oct 10, 2016 at 07:54:27PM +0200, Markus Trippelsdorf wrote:
> On 2016.10.08 at 13:21 +0200, Markus Trippelsdorf wrote:
> > On 2016.10.07 at 07:09 +0200, Markus Trippelsdorf wrote:
> > > On 2016.10.07 at 06:56 +0200, Markus Trippelsdorf wrote:
> > > > On 2016.10.07 at 06:32 +0200, Markus Trippelsdorf wrote:
> > > > > On 2016.10.07 at 13:22 +0900, Namhyung Kim wrote:
> > > > > > On Fri, Oct 07, 2016 at 05:51:18AM +0200, Markus Trippelsdorf wrote:
> > > > > > > On 2016.10.07 at 10:17 +0900, Namhyung Kim wrote:
> > > > > > > > On Thu, Oct 06, 2016 at 06:33:33PM +0200, Markus Trippelsdorf wrote:
> > > > > > > > > Scrolling down is broken when using "perf top --hierarchy".
> > > > > > > > > When it starts up everything is OK and one can scroll up and down to all
> > > > > > > > > entries. But as further and further new entries get added to the list,
> > > > > > > > > scrolling down is blocked (at the position of the last entry that was
> > > > > > > > > shown directly after startup).
> > > > > > > >
> > > > > > > > I think below patch will fix the problem. Please check.
> > > > > > >
> > > > > > > Yes. It works fine now. Many thanks.
> > > > > >
> > > > > > Good. Can I add your Tested-by then?
> > > > >
> > > > > Sure.
> > > >
> > > > And BTW symbols are currently always cut off at 60 characters in
> > > > expanded entries.
> > >
> > > Hmm, no. Sometimes they are cut off, sometimes they are not. I haven't
> > > figured out what triggered this strange behavior.
> >
> > Here is an example:
> >
> > % echo $COLUMNS
> > 179
> > % perf top --hierarchy
> > + 34.81% [kernel]
> > - 20.89% chrome
> > 0.51% [.] v8::internal::IncrementalMarking:
> > 0.43% [.] tc_malloc
> > 0.29% [.] sqlite3BtreeMovetoUnpacked
> > 0.28% [.] tc_free
> > 0.24% [.] v8::internal::BodyDescriptorBase:
> > 0.24% [.] sqlite3VdbeExec
> > 0.22% [.] v8::internal::MarkCompactCollecto
> > 0.19% [.] blink::SelectorChecker::checkOne
> > 0.19% [.] SkBlitRow::Color32
> > 0.18% [.] SkBlitLCD16OpaqueRow_SSE2
> > 0.17% [.] btreeInitPage.part.366
> > 0.16% [.] blink::SelectorChecker::matchSele
> > 0.15% [.] blink::ElementRuleCollector::coll
> > 0.15% [.] blink::CSSTokenizer::consumeName
> > 0.14% [.] sqlite3GetVarint
> > 0.13% [.] operator new[]
> > 0.12% [.] FPDFAPI_inflate_fast
> > 0.11% [.] v8::internal::HeapObject::SizeFro
> > 0.09% [.] tracked_objects::ThreadData::Tall
> > ...
>
> To continue this monologue, perf doesn't even look at these entries. So some
> hists__calc_col_len() calls seem to be missing for the "perf top --hierarchy"
> case or hists__reset_col_len() is called too early or too often.
Right, it missed to call the function for leaf entries. Could you
please check below patch fixes the problem?
Thanks,
Namhyung
>From 83383fa1412ddb4a1b7113aea799df63887bb4f8 Mon Sep 17 00:00:00 2001
From: Namhyung Kim <[email protected]>
Date: Mon, 24 Oct 2016 13:32:23 +0900
Subject: [PATCH] perf hists: Fix column length on --hierarchy
Markus reported that there's a weird behavior on perf top --hierarch
regarding the column length. Looking at the code, I found a debious
code which affects the symtoms. When --hierarchy option is used, the
last column length might be inaccurate since it skips to update the
length on leaf entries. I cannot remember why it did and looks like a
leftover from previous version during the development. Anyway updating
the column length often is not harmful. So let's move the code out.
Reported-by: Markus Trippelsdorf <[email protected]>
Signed-off-by: Namhyung Kim <[email protected]>
---
tools/perf/util/hist.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index b02992efb513..a69f027368ef 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -1600,18 +1600,18 @@ static void hists__hierarchy_output_resort(struct hists *hists,
if (prog)
ui_progress__update(prog, 1);
+ hists->nr_entries++;
+ if (!he->filtered) {
+ hists->nr_non_filtered_entries++;
+ hists__calc_col_len(hists, he);
+ }
+
if (!he->leaf) {
hists__hierarchy_output_resort(hists, prog,
&he->hroot_in,
&he->hroot_out,
min_callchain_hits,
use_callchain);
- hists->nr_entries++;
- if (!he->filtered) {
- hists->nr_non_filtered_entries++;
- hists__calc_col_len(hists, he);
- }
-
continue;
}
--
2.10.0
Hi Arnaldo,
Sorry for late reply.
On Fri, Oct 07, 2016 at 11:35:45AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Fri, Oct 07, 2016 at 01:53:57PM +0900, Namhyung Kim escreveu:
> > Cc-ing perf maintainers,
> >
> > On Fri, Oct 07, 2016 at 06:32:29AM +0200, Markus Trippelsdorf wrote:
> > > On 2016.10.07 at 13:22 +0900, Namhyung Kim wrote:
> > > > On Fri, Oct 07, 2016 at 05:51:18AM +0200, Markus Trippelsdorf wrote:
> > > > > On 2016.10.07 at 10:17 +0900, Namhyung Kim wrote:
> > > > > > On Thu, Oct 06, 2016 at 06:33:33PM +0200, Markus Trippelsdorf wrote:
> > > > > > > Scrolling down is broken when using "perf top --hierarchy".
> > > > > > > When it starts up everything is OK and one can scroll up and down to all
> > > > > > > entries. But as further and further new entries get added to the list,
> > > > > > > scrolling down is blocked (at the position of the last entry that was
> > > > > > > shown directly after startup).
> > > > > >
> > > > > > I think below patch will fix the problem. Please check.
> > > > >
> > > > > Yes. It works fine now. Many thanks.
> > > >
> > > > Good. Can I add your Tested-by then?
> > >
> > > Sure.
> >
> > Ok, I'll send a formal patch with it.
> >
> > >
> > > (And in the long run you should think of making "perf top --hierarchy"
> > > the default for perf top, because it gives a much better (uncluttered)
> > > overview of what is going on.)
> >
> > I think it's a matter of taste. Some people prefer to see the top
> > single function or something (i.e. current behavior) while others
> > prefer to see a higher-level view.
> >
> > But we can think again about the default at least for perf-top. I
> > worried about changing default behavior because last time we did it
> > for children mode many people complained about it. But I do think the
> > hierarchy mode is useful for many people though.
>
> So, I think in such cases we could experiment with asking the user about
> switching to the new mode by showing a popup message telling what it is
> about, if the user says "yes, I want to try it" switch to it and if
> another hotkey is pressed later, write what was chosen (yes, switch to
> this new mode, no, I don't like it, don't pester me about it anymore) to
> its ~/.perfconfig file so that next time it goes straight to this new
> mode, else don't ask the user again and keep using whatever mode was
> there already.
>
> What do you think?
I think it's a flexible way to set the default behavior while it seems
like a little bit complicated for implementation. Also I think it's
better to popup another dialog at the end and asks for comfirmation
(but it might not be appropriate for --stdio).
And to do that, we need to have a (programmable) way of dealing with
the configs.
Taeung, is there an update on your config patchset (especially for
write support)?
Thanks,
Namhyung
On 2016.10.24 at 13:55 +0900, Namhyung Kim wrote:
> Hi,
>
> Sorry for late reply.
>
> On Mon, Oct 10, 2016 at 07:54:27PM +0200, Markus Trippelsdorf wrote:
> > On 2016.10.08 at 13:21 +0200, Markus Trippelsdorf wrote:
> > > On 2016.10.07 at 07:09 +0200, Markus Trippelsdorf wrote:
> > > > On 2016.10.07 at 06:56 +0200, Markus Trippelsdorf wrote:
> > > > > On 2016.10.07 at 06:32 +0200, Markus Trippelsdorf wrote:
> > > > > > On 2016.10.07 at 13:22 +0900, Namhyung Kim wrote:
> > > > > > > On Fri, Oct 07, 2016 at 05:51:18AM +0200, Markus Trippelsdorf wrote:
> > > > > > > > On 2016.10.07 at 10:17 +0900, Namhyung Kim wrote:
> > > > > > > > > On Thu, Oct 06, 2016 at 06:33:33PM +0200, Markus Trippelsdorf wrote:
> > > > > > > > > > Scrolling down is broken when using "perf top --hierarchy".
> > > > > > > > > > When it starts up everything is OK and one can scroll up and down to all
> > > > > > > > > > entries. But as further and further new entries get added to the list,
> > > > > > > > > > scrolling down is blocked (at the position of the last entry that was
> > > > > > > > > > shown directly after startup).
> > > > > > > > >
> > > > > > > > > I think below patch will fix the problem. Please check.
> > > > > > > >
> > > > > > > > Yes. It works fine now. Many thanks.
> > > > > > >
> > > > > > > Good. Can I add your Tested-by then?
> > > > > >
> > > > > > Sure.
> > > > >
> > > > > And BTW symbols are currently always cut off at 60 characters in
> > > > > expanded entries.
> > > >
> > > > Hmm, no. Sometimes they are cut off, sometimes they are not. I haven't
> > > > figured out what triggered this strange behavior.
> > >
> > > Here is an example:
> > >
> > > % echo $COLUMNS
> > > 179
> > > % perf top --hierarchy
> > > + 34.81% [kernel]
> > > - 20.89% chrome
> > > 0.51% [.] v8::internal::IncrementalMarking:
> > > 0.43% [.] tc_malloc
> > > 0.29% [.] sqlite3BtreeMovetoUnpacked
> > > 0.28% [.] tc_free
> > > 0.24% [.] v8::internal::BodyDescriptorBase:
> > > 0.24% [.] sqlite3VdbeExec
> > > 0.22% [.] v8::internal::MarkCompactCollecto
> > > 0.19% [.] blink::SelectorChecker::checkOne
> > > 0.19% [.] SkBlitRow::Color32
> > > 0.18% [.] SkBlitLCD16OpaqueRow_SSE2
> > > 0.17% [.] btreeInitPage.part.366
> > > 0.16% [.] blink::SelectorChecker::matchSele
> > > 0.15% [.] blink::ElementRuleCollector::coll
> > > 0.15% [.] blink::CSSTokenizer::consumeName
> > > 0.14% [.] sqlite3GetVarint
> > > 0.13% [.] operator new[]
> > > 0.12% [.] FPDFAPI_inflate_fast
> > > 0.11% [.] v8::internal::HeapObject::SizeFro
> > > 0.09% [.] tracked_objects::ThreadData::Tall
> > > ...
> >
> > To continue this monologue, perf doesn't even look at these entries. So some
> > hists__calc_col_len() calls seem to be missing for the "perf top --hierarchy"
> > case or hists__reset_col_len() is called too early or too often.
>
> Right, it missed to call the function for leaf entries. Could you
> please check below patch fixes the problem?
Yes, it does. Thank you.
Another issue: all entries vanish if one scrolls to the left two times.
--
Markus
On Mon, Oct 24, 2016 at 07:53:12AM +0200, Markus Trippelsdorf wrote:
> On 2016.10.24 at 13:55 +0900, Namhyung Kim wrote:
> > Hi,
> >
> > Sorry for late reply.
> >
> > On Mon, Oct 10, 2016 at 07:54:27PM +0200, Markus Trippelsdorf wrote:
> > > On 2016.10.08 at 13:21 +0200, Markus Trippelsdorf wrote:
> > > > On 2016.10.07 at 07:09 +0200, Markus Trippelsdorf wrote:
> > > > > On 2016.10.07 at 06:56 +0200, Markus Trippelsdorf wrote:
> > > > > > On 2016.10.07 at 06:32 +0200, Markus Trippelsdorf wrote:
> > > > > > And BTW symbols are currently always cut off at 60 characters in
> > > > > > expanded entries.
> > > > >
> > > > > Hmm, no. Sometimes they are cut off, sometimes they are not. I haven't
> > > > > figured out what triggered this strange behavior.
> > > >
> > > > Here is an example:
> > > >
> > > > % echo $COLUMNS
> > > > 179
> > > > % perf top --hierarchy
> > > > + 34.81% [kernel]
> > > > - 20.89% chrome
> > > > 0.51% [.] v8::internal::IncrementalMarking:
> > > > 0.43% [.] tc_malloc
> > > > 0.29% [.] sqlite3BtreeMovetoUnpacked
> > > > 0.28% [.] tc_free
> > > > 0.24% [.] v8::internal::BodyDescriptorBase:
> > > > 0.24% [.] sqlite3VdbeExec
> > > > 0.22% [.] v8::internal::MarkCompactCollecto
> > > > 0.19% [.] blink::SelectorChecker::checkOne
> > > > 0.19% [.] SkBlitRow::Color32
> > > > 0.18% [.] SkBlitLCD16OpaqueRow_SSE2
> > > > 0.17% [.] btreeInitPage.part.366
> > > > 0.16% [.] blink::SelectorChecker::matchSele
> > > > 0.15% [.] blink::ElementRuleCollector::coll
> > > > 0.15% [.] blink::CSSTokenizer::consumeName
> > > > 0.14% [.] sqlite3GetVarint
> > > > 0.13% [.] operator new[]
> > > > 0.12% [.] FPDFAPI_inflate_fast
> > > > 0.11% [.] v8::internal::HeapObject::SizeFro
> > > > 0.09% [.] tracked_objects::ThreadData::Tall
> > > > ...
> > >
> > > To continue this monologue, perf doesn't even look at these entries. So some
> > > hists__calc_col_len() calls seem to be missing for the "perf top --hierarchy"
> > > case or hists__reset_col_len() is called too early or too often.
> >
> > Right, it missed to call the function for leaf entries. Could you
> > please check below patch fixes the problem?
>
> Yes, it does. Thank you.
>
> Another issue: all entries vanish if one scrolls to the left two times.
Hmm.. Did you mean pressing RIGHT key two times?
Thanks,
Namhyung
On 2016.10.24 at 15:02 +0900, Namhyung Kim wrote:
> On Mon, Oct 24, 2016 at 07:53:12AM +0200, Markus Trippelsdorf wrote:
> > On 2016.10.24 at 13:55 +0900, Namhyung Kim wrote:
> > > Hi,
> > >
> > > Sorry for late reply.
> > >
> > > On Mon, Oct 10, 2016 at 07:54:27PM +0200, Markus Trippelsdorf wrote:
> > > > On 2016.10.08 at 13:21 +0200, Markus Trippelsdorf wrote:
> > > > > On 2016.10.07 at 07:09 +0200, Markus Trippelsdorf wrote:
> > > > > > On 2016.10.07 at 06:56 +0200, Markus Trippelsdorf wrote:
> > > > > > > On 2016.10.07 at 06:32 +0200, Markus Trippelsdorf wrote:
> > > > > > > And BTW symbols are currently always cut off at 60 characters in
> > > > > > > expanded entries.
> > > > > >
> > > > > > Hmm, no. Sometimes they are cut off, sometimes they are not. I haven't
> > > > > > figured out what triggered this strange behavior.
> > > > >
> > > > > Here is an example:
> > > > >
> > > > > % echo $COLUMNS
> > > > > 179
> > > > > % perf top --hierarchy
> > > > > + 34.81% [kernel]
> > > > > - 20.89% chrome
> > > > > 0.51% [.] v8::internal::IncrementalMarking:
> > > > > 0.43% [.] tc_malloc
> > > > > 0.29% [.] sqlite3BtreeMovetoUnpacked
> > > > > 0.28% [.] tc_free
> > > > > 0.24% [.] v8::internal::BodyDescriptorBase:
> > > > > 0.24% [.] sqlite3VdbeExec
> > > > > 0.22% [.] v8::internal::MarkCompactCollecto
> > > > > 0.19% [.] blink::SelectorChecker::checkOne
> > > > > 0.19% [.] SkBlitRow::Color32
> > > > > 0.18% [.] SkBlitLCD16OpaqueRow_SSE2
> > > > > 0.17% [.] btreeInitPage.part.366
> > > > > 0.16% [.] blink::SelectorChecker::matchSele
> > > > > 0.15% [.] blink::ElementRuleCollector::coll
> > > > > 0.15% [.] blink::CSSTokenizer::consumeName
> > > > > 0.14% [.] sqlite3GetVarint
> > > > > 0.13% [.] operator new[]
> > > > > 0.12% [.] FPDFAPI_inflate_fast
> > > > > 0.11% [.] v8::internal::HeapObject::SizeFro
> > > > > 0.09% [.] tracked_objects::ThreadData::Tall
> > > > > ...
> > > >
> > > > To continue this monologue, perf doesn't even look at these entries. So some
> > > > hists__calc_col_len() calls seem to be missing for the "perf top --hierarchy"
> > > > case or hists__reset_col_len() is called too early or too often.
> > >
> > > Right, it missed to call the function for leaf entries. Could you
> > > please check below patch fixes the problem?
> >
> > Yes, it does. Thank you.
> >
> > Another issue: all entries vanish if one scrolls to the left two times.
>
> Hmm.. Did you mean pressing RIGHT key two times?
Ah, yes, sorry.
--
Markus
On Mon, Oct 24, 2016 at 08:04:31AM +0200, Markus Trippelsdorf wrote:
> On 2016.10.24 at 15:02 +0900, Namhyung Kim wrote:
> > On Mon, Oct 24, 2016 at 07:53:12AM +0200, Markus Trippelsdorf wrote:
> > > Another issue: all entries vanish if one scrolls to the left two times.
> >
> > Hmm.. Did you mean pressing RIGHT key two times?
>
> Ah, yes, sorry.
Ok, will fix. Thanks for the report.
Thanks,
Namhyung
Hi, Namhyung and Arnaldo :)
On 10/24/2016 02:11 PM, Namhyung Kim wrote:
> Hi Arnaldo,
>
> Sorry for late reply.
>
> On Fri, Oct 07, 2016 at 11:35:45AM -0300, Arnaldo Carvalho de Melo wrote:
>> Em Fri, Oct 07, 2016 at 01:53:57PM +0900, Namhyung Kim escreveu:
>>> Cc-ing perf maintainers,
>>>
>>> On Fri, Oct 07, 2016 at 06:32:29AM +0200, Markus Trippelsdorf wrote:
>>>> On 2016.10.07 at 13:22 +0900, Namhyung Kim wrote:
>>>>> On Fri, Oct 07, 2016 at 05:51:18AM +0200, Markus Trippelsdorf wrote:
>>>>>> On 2016.10.07 at 10:17 +0900, Namhyung Kim wrote:
>>>>>>> On Thu, Oct 06, 2016 at 06:33:33PM +0200, Markus Trippelsdorf wrote:
>>>>>>>> Scrolling down is broken when using "perf top --hierarchy".
>>>>>>>> When it starts up everything is OK and one can scroll up and down to all
>>>>>>>> entries. But as further and further new entries get added to the list,
>>>>>>>> scrolling down is blocked (at the position of the last entry that was
>>>>>>>> shown directly after startup).
>>>>>>>
>>>>>>> I think below patch will fix the problem. Please check.
>>>>>>
>>>>>> Yes. It works fine now. Many thanks.
>>>>>
>>>>> Good. Can I add your Tested-by then?
>>>>
>>>> Sure.
>>>
>>> Ok, I'll send a formal patch with it.
>>>
>>>>
>>>> (And in the long run you should think of making "perf top --hierarchy"
>>>> the default for perf top, because it gives a much better (uncluttered)
>>>> overview of what is going on.)
>>>
>>> I think it's a matter of taste. Some people prefer to see the top
>>> single function or something (i.e. current behavior) while others
>>> prefer to see a higher-level view.
>>>
>>> But we can think again about the default at least for perf-top. I
>>> worried about changing default behavior because last time we did it
>>> for children mode many people complained about it. But I do think the
>>> hierarchy mode is useful for many people though.
>>
>> So, I think in such cases we could experiment with asking the user about
>> switching to the new mode by showing a popup message telling what it is
>> about, if the user says "yes, I want to try it" switch to it and if
>> another hotkey is pressed later, write what was chosen (yes, switch to
>> this new mode, no, I don't like it, don't pester me about it anymore) to
>> its ~/.perfconfig file so that next time it goes straight to this new
>> mode, else don't ask the user again and keep using whatever mode was
>> there already.
>>
>> What do you think?
>
> I think it's a flexible way to set the default behavior while it seems
> like a little bit complicated for implementation. Also I think it's
> better to popup another dialog at the end and asks for comfirmation
> (but it might not be appropriate for --stdio).
>
> And to do that, we need to have a (programmable) way of dealing with
> the configs.
>
> Taeung, is there an update on your config patchset (especially for
> write support)?
>
Is related this link with what you said ?
https://lkml.org/lkml/2016/1/11/495
Yes, the config patchset would be need to be updated.
Because the config patchset which has 'write' feature
don't use a recent 'struct perf_config_set' so I should change it
to use 'perf_config_set' like show_config() of builtin-config.c:36.
Do you need write support of perf-config command ?
If this feature is more necessary than a recent patchset about default
config array https://lkml.org/lkml/2016/9/5/17,
I'd remake config patchset for getting and setting features first. :)
Thanks,
Taeung
Hi Taeung,
On Mon, Oct 24, 2016 at 07:10:24PM +0900, Taeung Song wrote:
> Hi, Namhyung and Arnaldo :)
>
> On 10/24/2016 02:11 PM, Namhyung Kim wrote:
> > Hi Arnaldo,
> >
> > Sorry for late reply.
> >
> > On Fri, Oct 07, 2016 at 11:35:45AM -0300, Arnaldo Carvalho de Melo wrote:
> > > Em Fri, Oct 07, 2016 at 01:53:57PM +0900, Namhyung Kim escreveu:
> > > > Cc-ing perf maintainers,
> > > >
> > > > On Fri, Oct 07, 2016 at 06:32:29AM +0200, Markus Trippelsdorf wrote:
> > > > > On 2016.10.07 at 13:22 +0900, Namhyung Kim wrote:
> > > > > > On Fri, Oct 07, 2016 at 05:51:18AM +0200, Markus Trippelsdorf wrote:
> > > > > > > On 2016.10.07 at 10:17 +0900, Namhyung Kim wrote:
> > > > > > > > On Thu, Oct 06, 2016 at 06:33:33PM +0200, Markus Trippelsdorf wrote:
> > > > > > > > > Scrolling down is broken when using "perf top --hierarchy".
> > > > > > > > > When it starts up everything is OK and one can scroll up and down to all
> > > > > > > > > entries. But as further and further new entries get added to the list,
> > > > > > > > > scrolling down is blocked (at the position of the last entry that was
> > > > > > > > > shown directly after startup).
> > > > > > > >
> > > > > > > > I think below patch will fix the problem. Please check.
> > > > > > >
> > > > > > > Yes. It works fine now. Many thanks.
> > > > > >
> > > > > > Good. Can I add your Tested-by then?
> > > > >
> > > > > Sure.
> > > >
> > > > Ok, I'll send a formal patch with it.
> > > >
> > > > >
> > > > > (And in the long run you should think of making "perf top --hierarchy"
> > > > > the default for perf top, because it gives a much better (uncluttered)
> > > > > overview of what is going on.)
> > > >
> > > > I think it's a matter of taste. Some people prefer to see the top
> > > > single function or something (i.e. current behavior) while others
> > > > prefer to see a higher-level view.
> > > >
> > > > But we can think again about the default at least for perf-top. I
> > > > worried about changing default behavior because last time we did it
> > > > for children mode many people complained about it. But I do think the
> > > > hierarchy mode is useful for many people though.
> > >
> > > So, I think in such cases we could experiment with asking the user about
> > > switching to the new mode by showing a popup message telling what it is
> > > about, if the user says "yes, I want to try it" switch to it and if
> > > another hotkey is pressed later, write what was chosen (yes, switch to
> > > this new mode, no, I don't like it, don't pester me about it anymore) to
> > > its ~/.perfconfig file so that next time it goes straight to this new
> > > mode, else don't ask the user again and keep using whatever mode was
> > > there already.
> > >
> > > What do you think?
> >
> > I think it's a flexible way to set the default behavior while it seems
> > like a little bit complicated for implementation. Also I think it's
> > better to popup another dialog at the end and asks for comfirmation
> > (but it might not be appropriate for --stdio).
> >
> > And to do that, we need to have a (programmable) way of dealing with
> > the configs.
> >
> > Taeung, is there an update on your config patchset (especially for
> > write support)?
> >
>
> Is related this link with what you said ?
> https://lkml.org/lkml/2016/1/11/495
Yep, that kind of thing.
>
> Yes, the config patchset would be need to be updated.
> Because the config patchset which has 'write' feature
> don't use a recent 'struct perf_config_set' so I should change it
> to use 'perf_config_set' like show_config() of builtin-config.c:36.
>
> Do you need write support of perf-config command ?
> If this feature is more necessary than a recent patchset about default
> config array https://lkml.org/lkml/2016/9/5/17,
> I'd remake config patchset for getting and setting features first. :)
What I need is a way to add a config item with specific value. Maybe
I can just append a line into a config file, but it needs to check
possible conflict somehow. So I think it needs to process existing
config items properly and update with the new value.
Thanks,
Namhyung
Sorry for late reply, Namhyung
On 10/25/2016 01:37 AM, Namhyung Kim wrote:
> Hi Taeung,
>
> On Mon, Oct 24, 2016 at 07:10:24PM +0900, Taeung Song wrote:
>> Hi, Namhyung and Arnaldo :)
>>
>> On 10/24/2016 02:11 PM, Namhyung Kim wrote:
>>> Hi Arnaldo,
>>>
>>> Sorry for late reply.
>>>
>>> On Fri, Oct 07, 2016 at 11:35:45AM -0300, Arnaldo Carvalho de Melo wrote:
>>>> Em Fri, Oct 07, 2016 at 01:53:57PM +0900, Namhyung Kim escreveu:
>>>>> Cc-ing perf maintainers,
>>>>>
>>>>> On Fri, Oct 07, 2016 at 06:32:29AM +0200, Markus Trippelsdorf wrote:
>>>>>> On 2016.10.07 at 13:22 +0900, Namhyung Kim wrote:
>>>>>>> On Fri, Oct 07, 2016 at 05:51:18AM +0200, Markus Trippelsdorf wrote:
>>>>>>>> On 2016.10.07 at 10:17 +0900, Namhyung Kim wrote:
>>>>>>>>> On Thu, Oct 06, 2016 at 06:33:33PM +0200, Markus Trippelsdorf wrote:
>>>>>>>>>> Scrolling down is broken when using "perf top --hierarchy".
>>>>>>>>>> When it starts up everything is OK and one can scroll up and down to all
>>>>>>>>>> entries. But as further and further new entries get added to the list,
>>>>>>>>>> scrolling down is blocked (at the position of the last entry that was
>>>>>>>>>> shown directly after startup).
>>>>>>>>>
>>>>>>>>> I think below patch will fix the problem. Please check.
>>>>>>>>
>>>>>>>> Yes. It works fine now. Many thanks.
>>>>>>>
>>>>>>> Good. Can I add your Tested-by then?
>>>>>>
>>>>>> Sure.
>>>>>
>>>>> Ok, I'll send a formal patch with it.
>>>>>
>>>>>>
>>>>>> (And in the long run you should think of making "perf top --hierarchy"
>>>>>> the default for perf top, because it gives a much better (uncluttered)
>>>>>> overview of what is going on.)
>>>>>
>>>>> I think it's a matter of taste. Some people prefer to see the top
>>>>> single function or something (i.e. current behavior) while others
>>>>> prefer to see a higher-level view.
>>>>>
>>>>> But we can think again about the default at least for perf-top. I
>>>>> worried about changing default behavior because last time we did it
>>>>> for children mode many people complained about it. But I do think the
>>>>> hierarchy mode is useful for many people though.
>>>>
>>>> So, I think in such cases we could experiment with asking the user about
>>>> switching to the new mode by showing a popup message telling what it is
>>>> about, if the user says "yes, I want to try it" switch to it and if
>>>> another hotkey is pressed later, write what was chosen (yes, switch to
>>>> this new mode, no, I don't like it, don't pester me about it anymore) to
>>>> its ~/.perfconfig file so that next time it goes straight to this new
>>>> mode, else don't ask the user again and keep using whatever mode was
>>>> there already.
>>>>
>>>> What do you think?
>>>
>>> I think it's a flexible way to set the default behavior while it seems
>>> like a little bit complicated for implementation. Also I think it's
>>> better to popup another dialog at the end and asks for comfirmation
>>> (but it might not be appropriate for --stdio).
>>>
>>> And to do that, we need to have a (programmable) way of dealing with
>>> the configs.
>>>
>>> Taeung, is there an update on your config patchset (especially for
>>> write support)?
>>>
>>
>> Is related this link with what you said ?
>> https://lkml.org/lkml/2016/1/11/495
>
> Yep, that kind of thing.
>
>>
>> Yes, the config patchset would be need to be updated.
>> Because the config patchset which has 'write' feature
>> don't use a recent 'struct perf_config_set' so I should change it
>> to use 'perf_config_set' like show_config() of builtin-config.c:36.
>>
>> Do you need write support of perf-config command ?
>> If this feature is more necessary than a recent patchset about default
>> config array https://lkml.org/lkml/2016/9/5/17,
>> I'd remake config patchset for getting and setting features first. :)
>
> What I need is a way to add a config item with specific value. Maybe
> I can just append a line into a config file, but it needs to check
> possible conflict somehow. So I think it needs to process existing
> config items properly and update with the new value.
>
I got it. Understood what you said!
I'll send a patchset for this. :)
Thanks,
Taeung