Re: [PATCH 3/5] perf hists: Link hist entries before inserting to an output tree
Hi, On Thu, 6 Dec 2012 17:25:43 +0100, Jiri Olsa wrote: > On Fri, Dec 07, 2012 at 12:09:39AM +0900, Namhyung Kim wrote: >> From: Namhyung Kim >> >> For matching and/or linking hist entries, they need to be sorted by >> given sort keys. However current hists__match/link did this on the >> output trees, so that the entries in the output tree need to be resort >> before doing it. >> >> This looks not so good since we have trees for collecting or >> collapsing entries before passing them to an output tree and they're >> already sorted by the given sort keys. Since we don't need to print >> anything at the time of matching/linking, we can use these internal >> trees directly instead of bothering with double resort on the output >> tree. > > this patch also makes diff working over collapsed entries, > which was not possible before.. nice ;) > > outputs like: > > [jolsa@krava perf]$ ./perf diff -s comm > # Event 'cycles:u' > # > # BaselineDelta Command > # ... ... > # > 5.24% +68.96% firefox > 2.34% +5.66%X > 48.51% -41.53% mocp > 14.98% -11.53%skype > 18.01% -15.35% plugin-containe > 1.03% +1.48%xchat > 5.54% -4.61% gkrellm > 1.41% -0.93%xterm > +0.33% xmonad-x86_64-l > +0.23% vim > +0.07% xscreensaver > 0.19% -0.14% swapper > 1.00% -0.97% NetworkManager > 0.28% -0.25% ssh > 0.11% -0.09%sleep > 0.84% -0.83% dbus-daemon > 0.02% -0.01% perf > 0.40% -0.40% wpa_supplicant > 0.05% -0.05% gpm > 0.04% -0.04%crond > > > small nitpick below, otherwise > > Acked-by: Jiri Olsa Thanks! > > >> >> Its only user - at the time of this writing - perf diff can be easily >> converted to use the internal tree and can save some lines too by >> getting rid of unnecessary resorting codes. >> >> Cc: Jiri Olsa >> Cc: Stephane Eranian >> Signed-off-by: Namhyung Kim >> --- >> tools/perf/builtin-diff.c | 65 >> - >> tools/perf/util/hist.c| 49 +- >> 2 files changed, 54 insertions(+), 60 deletions(-) >> >> diff --git a/tools/perf/builtin-diff.c b/tools/perf/builtin-diff.c >> index b2e7d39f099b..044ad99dcc90 100644 >> --- a/tools/perf/builtin-diff.c >> +++ b/tools/perf/builtin-diff.c >> @@ -275,43 +275,6 @@ static struct perf_tool tool = { >> return NULL;A > > SNIP > >> } >> >> -static void perf_evlist__resort_hists(struct perf_evlist *evlist, bool name) >> +static void perf_evlist__resort_hists(struct perf_evlist *evlist) > > this could be called 'perf_evlist__collapse_resort' now Will change in the next spin. Thanks, Namhyung -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/5] perf hists: Link hist entries before inserting to an output tree
Hi, On Thu, 6 Dec 2012 17:25:43 +0100, Jiri Olsa wrote: On Fri, Dec 07, 2012 at 12:09:39AM +0900, Namhyung Kim wrote: From: Namhyung Kim namhyung@lge.com For matching and/or linking hist entries, they need to be sorted by given sort keys. However current hists__match/link did this on the output trees, so that the entries in the output tree need to be resort before doing it. This looks not so good since we have trees for collecting or collapsing entries before passing them to an output tree and they're already sorted by the given sort keys. Since we don't need to print anything at the time of matching/linking, we can use these internal trees directly instead of bothering with double resort on the output tree. this patch also makes diff working over collapsed entries, which was not possible before.. nice ;) outputs like: [jolsa@krava perf]$ ./perf diff -s comm # Event 'cycles:u' # # BaselineDelta Command # ... ... # 5.24% +68.96% firefox 2.34% +5.66%X 48.51% -41.53% mocp 14.98% -11.53%skype 18.01% -15.35% plugin-containe 1.03% +1.48%xchat 5.54% -4.61% gkrellm 1.41% -0.93%xterm +0.33% xmonad-x86_64-l +0.23% vim +0.07% xscreensaver 0.19% -0.14% swapper 1.00% -0.97% NetworkManager 0.28% -0.25% ssh 0.11% -0.09%sleep 0.84% -0.83% dbus-daemon 0.02% -0.01% perf 0.40% -0.40% wpa_supplicant 0.05% -0.05% gpm 0.04% -0.04%crond small nitpick below, otherwise Acked-by: Jiri Olsa jo...@redhat.com Thanks! Its only user - at the time of this writing - perf diff can be easily converted to use the internal tree and can save some lines too by getting rid of unnecessary resorting codes. Cc: Jiri Olsa jo...@redhat.com Cc: Stephane Eranian eran...@google.com Signed-off-by: Namhyung Kim namhy...@kernel.org --- tools/perf/builtin-diff.c | 65 - tools/perf/util/hist.c| 49 +- 2 files changed, 54 insertions(+), 60 deletions(-) diff --git a/tools/perf/builtin-diff.c b/tools/perf/builtin-diff.c index b2e7d39f099b..044ad99dcc90 100644 --- a/tools/perf/builtin-diff.c +++ b/tools/perf/builtin-diff.c @@ -275,43 +275,6 @@ static struct perf_tool tool = { return NULL;A SNIP } -static void perf_evlist__resort_hists(struct perf_evlist *evlist, bool name) +static void perf_evlist__resort_hists(struct perf_evlist *evlist) this could be called 'perf_evlist__collapse_resort' now Will change in the next spin. Thanks, Namhyung -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/5] perf hists: Link hist entries before inserting to an output tree
On Fri, Dec 07, 2012 at 12:09:39AM +0900, Namhyung Kim wrote: > From: Namhyung Kim > > For matching and/or linking hist entries, they need to be sorted by > given sort keys. However current hists__match/link did this on the > output trees, so that the entries in the output tree need to be resort > before doing it. > > This looks not so good since we have trees for collecting or > collapsing entries before passing them to an output tree and they're > already sorted by the given sort keys. Since we don't need to print > anything at the time of matching/linking, we can use these internal > trees directly instead of bothering with double resort on the output > tree. this patch also makes diff working over collapsed entries, which was not possible before.. nice ;) outputs like: [jolsa@krava perf]$ ./perf diff -s comm # Event 'cycles:u' # # BaselineDelta Command # ... ... # 5.24% +68.96% firefox 2.34% +5.66%X 48.51% -41.53% mocp 14.98% -11.53%skype 18.01% -15.35% plugin-containe 1.03% +1.48%xchat 5.54% -4.61% gkrellm 1.41% -0.93%xterm +0.33% xmonad-x86_64-l +0.23% vim +0.07% xscreensaver 0.19% -0.14% swapper 1.00% -0.97% NetworkManager 0.28% -0.25% ssh 0.11% -0.09%sleep 0.84% -0.83% dbus-daemon 0.02% -0.01% perf 0.40% -0.40% wpa_supplicant 0.05% -0.05% gpm 0.04% -0.04%crond small nitpick below, otherwise Acked-by: Jiri Olsa > > Its only user - at the time of this writing - perf diff can be easily > converted to use the internal tree and can save some lines too by > getting rid of unnecessary resorting codes. > > Cc: Jiri Olsa > Cc: Stephane Eranian > Signed-off-by: Namhyung Kim > --- > tools/perf/builtin-diff.c | 65 > - > tools/perf/util/hist.c| 49 +- > 2 files changed, 54 insertions(+), 60 deletions(-) > > diff --git a/tools/perf/builtin-diff.c b/tools/perf/builtin-diff.c > index b2e7d39f099b..044ad99dcc90 100644 > --- a/tools/perf/builtin-diff.c > +++ b/tools/perf/builtin-diff.c > @@ -275,43 +275,6 @@ static struct perf_tool tool = { > return NULL;A SNIP > } > > -static void perf_evlist__resort_hists(struct perf_evlist *evlist, bool name) > +static void perf_evlist__resort_hists(struct perf_evlist *evlist) this could be called 'perf_evlist__collapse_resort' now > { > struct perf_evsel *evsel; > > list_for_each_entry(evsel, >entries, node) { > struct hists *hists = >hists; > > - hists__output_resort(hists); > - > - if (name) > - hists__name_resort(hists); > + hists__collapse_resort(hists); > } > } SNIP -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 3/5] perf hists: Link hist entries before inserting to an output tree
From: Namhyung Kim For matching and/or linking hist entries, they need to be sorted by given sort keys. However current hists__match/link did this on the output trees, so that the entries in the output tree need to be resort before doing it. This looks not so good since we have trees for collecting or collapsing entries before passing them to an output tree and they're already sorted by the given sort keys. Since we don't need to print anything at the time of matching/linking, we can use these internal trees directly instead of bothering with double resort on the output tree. Its only user - at the time of this writing - perf diff can be easily converted to use the internal tree and can save some lines too by getting rid of unnecessary resorting codes. Cc: Jiri Olsa Cc: Stephane Eranian Signed-off-by: Namhyung Kim --- tools/perf/builtin-diff.c | 65 - tools/perf/util/hist.c| 49 +- 2 files changed, 54 insertions(+), 60 deletions(-) diff --git a/tools/perf/builtin-diff.c b/tools/perf/builtin-diff.c index b2e7d39f099b..044ad99dcc90 100644 --- a/tools/perf/builtin-diff.c +++ b/tools/perf/builtin-diff.c @@ -275,43 +275,6 @@ static struct perf_tool tool = { .ordering_requires_timestamps = true, }; -static void insert_hist_entry_by_name(struct rb_root *root, - struct hist_entry *he) -{ - struct rb_node **p = >rb_node; - struct rb_node *parent = NULL; - struct hist_entry *iter; - - while (*p != NULL) { - parent = *p; - iter = rb_entry(parent, struct hist_entry, rb_node); - if (hist_entry__cmp(he, iter) < 0) - p = &(*p)->rb_left; - else - p = &(*p)->rb_right; - } - - rb_link_node(>rb_node, parent, p); - rb_insert_color(>rb_node, root); -} - -static void hists__name_resort(struct hists *self) -{ - struct rb_root tmp = RB_ROOT; - struct rb_node *next = rb_first(>entries); - - while (next != NULL) { - struct hist_entry *n = rb_entry(next, struct hist_entry, rb_node); - - next = rb_next(>rb_node); - - rb_erase(>rb_node, >entries); - insert_hist_entry_by_name(, n); - } - - self->entries = tmp; -} - static struct perf_evsel *evsel_match(struct perf_evsel *evsel, struct perf_evlist *evlist) { @@ -324,30 +287,34 @@ static struct perf_evsel *evsel_match(struct perf_evsel *evsel, return NULL; } -static void perf_evlist__resort_hists(struct perf_evlist *evlist, bool name) +static void perf_evlist__resort_hists(struct perf_evlist *evlist) { struct perf_evsel *evsel; list_for_each_entry(evsel, >entries, node) { struct hists *hists = >hists; - hists__output_resort(hists); - - if (name) - hists__name_resort(hists); + hists__collapse_resort(hists); } } static void hists__baseline_only(struct hists *hists) { - struct rb_node *next = rb_first(>entries); + struct rb_root *root; + struct rb_node *next; + + if (sort__need_collapse) + root = >entries_collapsed; + else + root = hists->entries_in; + next = rb_first(root); while (next != NULL) { - struct hist_entry *he = rb_entry(next, struct hist_entry, rb_node); + struct hist_entry *he = rb_entry(next, struct hist_entry, rb_node_in); - next = rb_next(>rb_node); + next = rb_next(>rb_node_in); if (!hist_entry__next_pair(he)) { - rb_erase(>rb_node, >entries); + rb_erase(>rb_node_in, root); hist_entry__free(he); } } @@ -471,6 +438,8 @@ static void hists__process(struct hists *old, struct hists *new) else hists__link(new, old); + hists__output_resort(new); + if (sort_compute) { hists__precompute(new); hists__compute_resort(new); @@ -505,8 +474,8 @@ static int __cmd_diff(void) evlist_old = older->evlist; evlist_new = newer->evlist; - perf_evlist__resort_hists(evlist_old, true); - perf_evlist__resort_hists(evlist_new, false); + perf_evlist__resort_hists(evlist_old); + perf_evlist__resort_hists(evlist_new); list_for_each_entry(evsel, _new->entries, node) { struct perf_evsel *evsel_old; diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c index d4471c21ed17..b0c9952d7c3f 100644 --- a/tools/perf/util/hist.c +++ b/tools/perf/util/hist.c @@ -720,16 +720,24 @@ void hists__inc_nr_events(struct hists *hists, u32 type) static struct hist_entry *hists__add_dummy_entry(struct hists *hists,
[PATCH 3/5] perf hists: Link hist entries before inserting to an output tree
From: Namhyung Kim namhyung@lge.com For matching and/or linking hist entries, they need to be sorted by given sort keys. However current hists__match/link did this on the output trees, so that the entries in the output tree need to be resort before doing it. This looks not so good since we have trees for collecting or collapsing entries before passing them to an output tree and they're already sorted by the given sort keys. Since we don't need to print anything at the time of matching/linking, we can use these internal trees directly instead of bothering with double resort on the output tree. Its only user - at the time of this writing - perf diff can be easily converted to use the internal tree and can save some lines too by getting rid of unnecessary resorting codes. Cc: Jiri Olsa jo...@redhat.com Cc: Stephane Eranian eran...@google.com Signed-off-by: Namhyung Kim namhy...@kernel.org --- tools/perf/builtin-diff.c | 65 - tools/perf/util/hist.c| 49 +- 2 files changed, 54 insertions(+), 60 deletions(-) diff --git a/tools/perf/builtin-diff.c b/tools/perf/builtin-diff.c index b2e7d39f099b..044ad99dcc90 100644 --- a/tools/perf/builtin-diff.c +++ b/tools/perf/builtin-diff.c @@ -275,43 +275,6 @@ static struct perf_tool tool = { .ordering_requires_timestamps = true, }; -static void insert_hist_entry_by_name(struct rb_root *root, - struct hist_entry *he) -{ - struct rb_node **p = root-rb_node; - struct rb_node *parent = NULL; - struct hist_entry *iter; - - while (*p != NULL) { - parent = *p; - iter = rb_entry(parent, struct hist_entry, rb_node); - if (hist_entry__cmp(he, iter) 0) - p = (*p)-rb_left; - else - p = (*p)-rb_right; - } - - rb_link_node(he-rb_node, parent, p); - rb_insert_color(he-rb_node, root); -} - -static void hists__name_resort(struct hists *self) -{ - struct rb_root tmp = RB_ROOT; - struct rb_node *next = rb_first(self-entries); - - while (next != NULL) { - struct hist_entry *n = rb_entry(next, struct hist_entry, rb_node); - - next = rb_next(n-rb_node); - - rb_erase(n-rb_node, self-entries); - insert_hist_entry_by_name(tmp, n); - } - - self-entries = tmp; -} - static struct perf_evsel *evsel_match(struct perf_evsel *evsel, struct perf_evlist *evlist) { @@ -324,30 +287,34 @@ static struct perf_evsel *evsel_match(struct perf_evsel *evsel, return NULL; } -static void perf_evlist__resort_hists(struct perf_evlist *evlist, bool name) +static void perf_evlist__resort_hists(struct perf_evlist *evlist) { struct perf_evsel *evsel; list_for_each_entry(evsel, evlist-entries, node) { struct hists *hists = evsel-hists; - hists__output_resort(hists); - - if (name) - hists__name_resort(hists); + hists__collapse_resort(hists); } } static void hists__baseline_only(struct hists *hists) { - struct rb_node *next = rb_first(hists-entries); + struct rb_root *root; + struct rb_node *next; + + if (sort__need_collapse) + root = hists-entries_collapsed; + else + root = hists-entries_in; + next = rb_first(root); while (next != NULL) { - struct hist_entry *he = rb_entry(next, struct hist_entry, rb_node); + struct hist_entry *he = rb_entry(next, struct hist_entry, rb_node_in); - next = rb_next(he-rb_node); + next = rb_next(he-rb_node_in); if (!hist_entry__next_pair(he)) { - rb_erase(he-rb_node, hists-entries); + rb_erase(he-rb_node_in, root); hist_entry__free(he); } } @@ -471,6 +438,8 @@ static void hists__process(struct hists *old, struct hists *new) else hists__link(new, old); + hists__output_resort(new); + if (sort_compute) { hists__precompute(new); hists__compute_resort(new); @@ -505,8 +474,8 @@ static int __cmd_diff(void) evlist_old = older-evlist; evlist_new = newer-evlist; - perf_evlist__resort_hists(evlist_old, true); - perf_evlist__resort_hists(evlist_new, false); + perf_evlist__resort_hists(evlist_old); + perf_evlist__resort_hists(evlist_new); list_for_each_entry(evsel, evlist_new-entries, node) { struct perf_evsel *evsel_old; diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c index d4471c21ed17..b0c9952d7c3f 100644 --- a/tools/perf/util/hist.c +++ b/tools/perf/util/hist.c @@ -720,16 +720,24 @@ void
Re: [PATCH 3/5] perf hists: Link hist entries before inserting to an output tree
On Fri, Dec 07, 2012 at 12:09:39AM +0900, Namhyung Kim wrote: From: Namhyung Kim namhyung@lge.com For matching and/or linking hist entries, they need to be sorted by given sort keys. However current hists__match/link did this on the output trees, so that the entries in the output tree need to be resort before doing it. This looks not so good since we have trees for collecting or collapsing entries before passing them to an output tree and they're already sorted by the given sort keys. Since we don't need to print anything at the time of matching/linking, we can use these internal trees directly instead of bothering with double resort on the output tree. this patch also makes diff working over collapsed entries, which was not possible before.. nice ;) outputs like: [jolsa@krava perf]$ ./perf diff -s comm # Event 'cycles:u' # # BaselineDelta Command # ... ... # 5.24% +68.96% firefox 2.34% +5.66%X 48.51% -41.53% mocp 14.98% -11.53%skype 18.01% -15.35% plugin-containe 1.03% +1.48%xchat 5.54% -4.61% gkrellm 1.41% -0.93%xterm +0.33% xmonad-x86_64-l +0.23% vim +0.07% xscreensaver 0.19% -0.14% swapper 1.00% -0.97% NetworkManager 0.28% -0.25% ssh 0.11% -0.09%sleep 0.84% -0.83% dbus-daemon 0.02% -0.01% perf 0.40% -0.40% wpa_supplicant 0.05% -0.05% gpm 0.04% -0.04%crond small nitpick below, otherwise Acked-by: Jiri Olsa jo...@redhat.com Its only user - at the time of this writing - perf diff can be easily converted to use the internal tree and can save some lines too by getting rid of unnecessary resorting codes. Cc: Jiri Olsa jo...@redhat.com Cc: Stephane Eranian eran...@google.com Signed-off-by: Namhyung Kim namhy...@kernel.org --- tools/perf/builtin-diff.c | 65 - tools/perf/util/hist.c| 49 +- 2 files changed, 54 insertions(+), 60 deletions(-) diff --git a/tools/perf/builtin-diff.c b/tools/perf/builtin-diff.c index b2e7d39f099b..044ad99dcc90 100644 --- a/tools/perf/builtin-diff.c +++ b/tools/perf/builtin-diff.c @@ -275,43 +275,6 @@ static struct perf_tool tool = { return NULL;A SNIP } -static void perf_evlist__resort_hists(struct perf_evlist *evlist, bool name) +static void perf_evlist__resort_hists(struct perf_evlist *evlist) this could be called 'perf_evlist__collapse_resort' now { struct perf_evsel *evsel; list_for_each_entry(evsel, evlist-entries, node) { struct hists *hists = evsel-hists; - hists__output_resort(hists); - - if (name) - hists__name_resort(hists); + hists__collapse_resort(hists); } } SNIP -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/